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
