Re: [PATCH] mm/gup_test: reject wrapped user ranges

[David Hildenbrand (Arm)]

On 6/10/26 03:27, Andrew Morton wrote:
> On Tue,  9 Jun 2026 00:48:15 +0000 Samuel Moelius <sam.moelius@xxxxxxxxxxxxxxx> wrote:
> 
>> gup_test accepts an address and size from the debugfs ioctl and
>> repeatedly compares against addr + size. If that addition wraps, the
>> loop can be skipped and the ioctl returns success with size rewritten to
>> zero.
>>
>> Compute the end address once with overflow checking and use that checked
>> end for the loop bounds.
>>
> 
> Looks sane, thanks.
> 
>> --- a/mm/gup_test.c
>> +++ b/mm/gup_test.c
>> @@ -105,11 +105,15 @@ static int __gup_test_ioctl(unsigned int cmd,
>>  	unsigned long i, nr_pages, addr, next;
>>  	long nr;
>>  	struct page **pages;
>> +	unsigned long end;
>>  	int ret = 0;
>>  	bool needs_mmap_lock =
>>  		cmd != GUP_FAST_BENCHMARK && cmd != PIN_FAST_BENCHMARK;
>>  
>> -	if (gup->size > ULONG_MAX)
>> +	if (gup->addr > ULONG_MAX || gup->size > ULONG_MAX)
>> +		return -EINVAL;
>> +	if (check_add_overflow((unsigned long)gup->addr,
>> +			       (unsigned long)gup->size, &end))
> 
> I wonder why those fields were made __u64 instead of ulong.
> 
>>  		return -EINVAL;
>>  
>>  	nr_pages = gup->size / PAGE_SIZE;
>> @@ -125,13 +129,13 @@ static int __gup_test_ioctl(unsigned int cmd,
>>  	i = 0;
>>  	nr = gup->nr_pages_per_call;
>>  	start_time = ktime_get();
>> -	for (addr = gup->addr; addr < gup->addr + gup->size; addr = next) {
>> +	for (addr = gup->addr; addr < end; addr = next) {
>>  		if (nr != gup->nr_pages_per_call)
>>  			break;
>>  
>>  		next = addr + nr * PAGE_SIZE;
> 
> Sashiko AI review identified a pre-existing possible overflow here on
> 32-bit machines.
> 
> https://sashiko.dev/#/patchset/20260609004814.1240586.6294d614ac80.gup-test-range-end-wrap@xxxxxxxxxxxxxxx

I mean, we don't really care for gup_test.c about any of that. The patch here
looks like a decent cleanup, so I think we should take it.

Fixing odd things on 32bit that will never happen? Not so sure.

-- 
Cheers,

David