[PATCH] mm/gup_test: reject wrapped user ranges

[PATCH] mm/gup_test: reject wrapped user ranges

[Samuel Moelius]

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.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 mm/gup_test.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/mm/gup_test.c b/mm/gup_test.c
index 9dd48db897b9..eb4c9cda16ed 100644
--- 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))
 		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;
-		if (next > gup->addr + gup->size) {
-			next = gup->addr + gup->size;
+		if (next > end) {
+			next = end;
 			nr = (next - addr) / PAGE_SIZE;
 		}
 
-- 
2.43.0

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

[Andrew Morton]

On Tue,  9 Jun 2026 00:48:15 +0000 Samuel Moelius <sam.moelius@trailofbits.com> 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@trailofbits.com

> -		if (next > gup->addr + gup->size) {
> -			next = gup->addr + gup->size;
> +		if (next > end) {
> +			next = end;
>  			nr = (next - addr) / PAGE_SIZE;
>  		}

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@trailofbits.com> 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@trailofbits.com

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

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

[Samuel Moelius]

On Tue, Jun 9, 2026 at 9:27 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue,  9 Jun 2026 00:48:15 +0000 Samuel Moelius <sam.moelius@trailofbits.com> 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@trailofbits.com

Does the __u64 or Sashiko AI point require action from me?

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

[Andrew Morton]

On Wed, 10 Jun 2026 13:22:32 -0400 Samuel Moelius <sam.moelius@trailofbits.com> wrote:

> > >       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@trailofbits.com
> 
> Does the __u64 or Sashiko AI point require action from me?

Not really.  The patch makes the code better, that's what counts!

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

[David Hildenbrand (Arm)]

On 6/9/26 02:48, Samuel Moelius 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.

Yeah, it's only used for testing and (a) not expected to be included in
production kernels or of so (b) only accessible to root.

So we didn't particularly care about making this interface watertight.

> 
> Compute the end address once with overflow checking and use that checked
> end for the loop bounds.
> 
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
> ---
>  mm/gup_test.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/gup_test.c b/mm/gup_test.c
> index 9dd48db897b9..eb4c9cda16ed 100644
> --- 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))
>  		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;
> -		if (next > gup->addr + gup->size) {
> -			next = gup->addr + gup->size;
> +		if (next > end) {
> +			next = end;
>  			nr = (next - addr) / PAGE_SIZE;
>  		}
>  

LGTM

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David