Re: [PATCH v4] mm/gup_test: safely calculate GUP batch size

[David Hildenbrand (Arm)]

On 9/11/26 14:13, Sarthak Sharma wrote:
> Hi David!
> 
> On 9/7/26 8:20 PM, David Hildenbrand (Arm) wrote:
>> On 9/3/26 13:50, Sarthak Sharma wrote:
>>> __gup_test_ioctl() calculates the end of a GUP batch using:
>>>
>>> next = addr + nr * PAGE_SIZE;
>>>
>>> If nr is too large, it can cause next to overflow and wrap around.
>>> If it wraps, the next > end check is bypassed and a large value
>>> of nr is passed to the GUP call, even though the pages array was
>>> allocated according to gup->size. This can lead to out of bounds writes.
>>>
>>> Also, when fewer than PAGE_SIZE bytes remain, the calculated batch
>>> contains zero pages. The code still calls GUP functions with pages + i,
>>> which can point past the allocated array.
>>>
>>> Calculate nr by taking the minimum of the number of pages per call and
>>> the pages remaining in the address range. Stop processing when no
>>> pages remain and calculate the end of the batch using this bounded
>>> value of nr.
>>>
>>> Fixes: 64c349f4ae78 ("mm: add infrastructure for get_user_pages_fast() benchmarking")
>>> Signed-off-by: Sarthak Sharma <sarthak.sharma@xxxxxxx>
>>> ---
>>> Sashiko reported the issue fixed by this patch while reviewing the patch
>>> "mm/gup_test: report actual pinned bytes". That patch has been applied to
>>> mm-new. Since the two fixes are independent, this revision contains
>>> only the GUP batch size fix.
>>>
>>> Changes in v4:
>>> - Simplify batch clamping logic as suggested by Kiryl
>>> - Don't call GUP functions for an empty batch
>>> - Drop the pinned byte reporting patch since it has been applied to mm-new
>>>
>>> v3: https://lore.kernel.org/all/20260901083452.115365-1-sarthak.sharma@xxxxxxx/
>>>
>>>  mm/gup_test.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/mm/gup_test.c b/mm/gup_test.c
>>> index 185ba3bb8ed1..1b64e932c4c5 100644
>>> --- a/mm/gup_test.c
>>> +++ b/mm/gup_test.c
>>> @@ -139,11 +139,12 @@ static int __gup_test_ioctl(unsigned int cmd,
>>>  		if (nr != gup->nr_pages_per_call)
>>>  			break;
>>>  
>>> +		nr = min_t(unsigned long, gup->nr_pages_per_call,
>>> +			   (end - addr) / PAGE_SIZE);
>>> +		if (!nr)
>>> +			break;
>>
>> Can !nr actually happen?
> 
> Yes, it is possible in 2 cases:
> 
> 1. We calculate end as gup->addr + gup->size. So, if gup->size is not
> page aligned, then at some point, we can have (end - addr) < PAGE_SIZE
> and (end != addr). Then (end - addr) / PAGESIZE will be 0. Right now all
> callers seem to provide a page aligned gup->size, but any userspace
> program which can open /sys/kernel/debug/gup_test can provide a non page
> aligned gup->size.

Note that this is a pure testing interface. So we don't care about weird user
space doing stupid things.

Rather reject unaligned gup->size early in __gup_test_ioctl().

> 
> 2. If the userspace supplies nr_pages_per_call = 0 (we never validate
> nr_pages_per_call so it is possible).
> 

Same here, user space won't do that. If you want to be sure, rather reject
exactly that early in __gup_test_ioctl().

Something like

diff --git a/mm/gup_test.c b/mm/gup_test.c
index 185ba3bb8ed10..9e3e1df8dd526 100644
--- a/mm/gup_test.c
+++ b/mm/gup_test.c
@@ -116,7 +116,8 @@ static int __gup_test_ioctl(unsigned int cmd,
        bool needs_mmap_lock =
                cmd != GUP_FAST_BENCHMARK && cmd != PIN_FAST_BENCHMARK;

-       if (gup->addr > ULONG_MAX || gup->size > ULONG_MAX)
+       if (gup->addr > ULONG_MAX || gup->size > ULONG_MAX || !gup->size ||
+           !PAGE_ALIGNED(gup->size))
                return -EINVAL;
        if (check_add_overflow((unsigned long)gup->addr,
                               (unsigned long)gup->size, &end))

> Sorry for the delay, your email didn't reach my inbox due to some reason :)

No worries.

-- 
Cheers,

David