Hi Dan,
Thank you for reporting.
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
