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