Re: [bug report] fs/proc/task_mmu: implement IOCTL to get and optionally clear info about PTEs

[Muhammad Usama Anjum <Usama.Anjum@collabora.com>]

On 9/12/24 11:36 AM, Muhammad Usama Anjum wrote:
> Hi Dan,
> 
> Thank you for reporting.
I've debugged more and found out that no changes are required as
access_ok() already deals well with the overflows. I've tested the
corner cases on x86_64 and there are no issue found.

I'll add more test cases in the selftest for this ioctl. Please share
your thoughts if I may have missed something.

> 
> On 9/11/24 3:21 PM, Dan Carpenter wrote:
>> Hello Muhammad Usama Anjum,
>>
>> Commit 52526ca7fdb9 ("fs/proc/task_mmu: implement IOCTL to get and
>> optionally clear info about PTEs") from Aug 21, 2023 (linux-next),
>> leads to the following Smatch static checker warning:
>>
>> 	fs/proc/task_mmu.c:2664 pagemap_scan_get_args()
>> 	warn: potential user controlled sizeof overflow 'arg->vec_len * 24' '0-u64max * 24' type='ullong'
>>
>> fs/proc/task_mmu.c
>>     2637 static int pagemap_scan_get_args(struct pm_scan_arg *arg,
>>     2638                                  unsigned long uarg)
>>     2639 {
>>     2640         if (copy_from_user(arg, (void __user *)uarg, sizeof(*arg)))
>>
>> arg comes from the user
>>
>>     2641                 return -EFAULT;
>>     2642 
>>     2643         if (arg->size != sizeof(struct pm_scan_arg))
>>     2644                 return -EINVAL;
>>     2645 
>>     2646         /* Validate requested features */
>>     2647         if (arg->flags & ~PM_SCAN_FLAGS)
>>     2648                 return -EINVAL;
>>     2649         if ((arg->category_inverted | arg->category_mask |
>>     2650              arg->category_anyof_mask | arg->return_mask) & ~PM_SCAN_CATEGORIES)
>>     2651                 return -EINVAL;
>>     2652 
>>     2653         arg->start = untagged_addr((unsigned long)arg->start);
>>     2654         arg->end = untagged_addr((unsigned long)arg->end);
>>     2655         arg->vec = untagged_addr((unsigned long)arg->vec);
>>     2656 
>>     2657         /* Validate memory pointers */
>>     2658         if (!IS_ALIGNED(arg->start, PAGE_SIZE))
>>     2659                 return -EINVAL;
>>
>> We should probably check ->end here as well.
>>
>>     2660         if (!access_ok((void __user *)(long)arg->start, arg->end - arg->start))
> I'll add check to verify that end is equal or greater than start.
> 
>>
>> Otherwise we're checking access_ok() and then making ->end larger.  Maybe move
>> the arg->end = ALIGN(arg->end, PAGE_SIZE) before the access_ok() check?
>>
>>     2661                 return -EFAULT;
>>     2662         if (!arg->vec && arg->vec_len)
>>     2663                 return -EINVAL;
>> --> 2664         if (arg->vec && !access_ok((void __user *)(long)arg->vec,
>>     2665                               arg->vec_len * sizeof(struct page_region)))
>>
>> This "arg->vec_len * sizeof(struct page_region)" multiply could have an integer
>> overflow.
> I'll check for overflow before calling access_ok().
> 
>>
>> arg->vec_len is a u64 so size_add() won't work on a 32bit system.  I wonder if
>> size_add() should check for sizes larger than SIZE_MAX?
>>
>>     2666                 return -EFAULT;
>>     2667 
>>     2668         /* Fixup default values */
>>     2669         arg->end = ALIGN(arg->end, PAGE_SIZE);
>>     2670         arg->walk_end = 0;
>>     2671         if (!arg->max_pages)
>>     2672                 arg->max_pages = ULONG_MAX;
>>     2673 
>>     2674         return 0;
>>     2675 }
> I'll send fix soon.
> 
>>
>> regards,
>> dan carpenter
> 

-- 
BR,
Muhammad Usama Anjum