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

[Sarthak Sharma]

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.

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

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