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

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

[Muhammad Usama Anjum]

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

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

[Dan Carpenter]

On Thu, Sep 12, 2024 at 02:34:54PM +0500, Muhammad Usama Anjum wrote:
> 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.
> 

I don't understand what you are saying.  We are discussing three issues:

1)  We check the size and then make the size larger:

    check: if (!access_ok((void __user *)(long)arg->start, arg->end - arg->start))
    larger: arg->end = ALIGN(arg->end, PAGE_SIZE);

The ALIGN() can't make ->end go beyond the end of the page so it's possible that
if you have access_ok to the start of the page then you have access to the whole
page.  It just seems ugly to rely on that.  (I don't know mm very well).

2) Passing negative sizes to access_ok().

The access_ok() check will treat negative sizes as very large positive sizes and
it will be rejected.  So far as I can see this is fine.  Plus there is a check
at the start of walk_page_range() which says if (start >= end) return -EINVAL;

3) Integer overflow:
	access_ok((void __user *)(long)arg->vec, arg->vec_len * sizeof(struct page_region))
                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This multiplication can overflow so access_ok() checks a smaller size than
intended.  It will absolutely return success when it shouldn't.  To determine
the impact then we have to look at do_pagemap_scan() but I don't know the code
well enough to say if it's harmless or what the impact is.

Integer overflows to access_ok() are always treated as a bug though even if it's
harmless.

regards,
dan carpenter