We were checking one byte beyond the actual range that would be accessed.
Originally, valid_user_address would consider the user guard page to be
valid, so checks including the final accessible byte would still succeed.
However, after commit 86e6b1547b3d ("x86: fix user address masking
non-canonical speculation issue") this is no longer the case.

Update the logic to always consider the final address in the range.

Fixes: 86e6b1547b3d ("x86: fix user address masking non-canonical speculation issue")
Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
---
 arch/x86/include/asm/uaccess_64.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index b0a887209400..3e0eb72c036f 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -100,9 +100,11 @@ static inline bool __access_ok(const void __user *ptr, unsigned long size)
 	if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
 		return valid_user_address(ptr);
 	} else {
-		unsigned long sum = size + (__force unsigned long)ptr;
+		unsigned long end = (__force unsigned long)ptr;
 
-		return valid_user_address(sum) && sum >= (__force unsigned long)ptr;
+		if (size)
+			end += size - 1;
+		return valid_user_address(end) && end >= (__force unsigned long)ptr;
 	}
 }
 #define __access_ok __access_ok
-- 
2.47.0

From: Mikel Rychliski
> Sent: 09 November 2024 21:03
> 
> We were checking one byte beyond the actual range that would be accessed.
> Originally, valid_user_address would consider the user guard page to be
> valid, so checks including the final accessible byte would still succeed.

Did it allow the entire page or just the first byte?
The test for ignoring small constant sizes rather assumes that accesses
to the guard page are errored (or transfers start with the first byte).

> However, after commit 86e6b1547b3d ("x86: fix user address masking
> non-canonical speculation issue") this is no longer the case.
> 
> Update the logic to always consider the final address in the range.
> 
> Fixes: 86e6b1547b3d ("x86: fix user address masking non-canonical speculation issue")
> Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
> ---
>  arch/x86/include/asm/uaccess_64.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> index b0a887209400..3e0eb72c036f 100644
> --- a/arch/x86/include/asm/uaccess_64.h
> +++ b/arch/x86/include/asm/uaccess_64.h
> @@ -100,9 +100,11 @@ static inline bool __access_ok(const void __user *ptr, unsigned long size)
>  	if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
>  		return valid_user_address(ptr);
>  	} else {
> -		unsigned long sum = size + (__force unsigned long)ptr;
> +		unsigned long end = (__force unsigned long)ptr;
> 
> -		return valid_user_address(sum) && sum >= (__force unsigned long)ptr;
> +		if (size)
> +			end += size - 1;
> +		return valid_user_address(end) && end >= (__force unsigned long)ptr;

Why not:
	if (statically_true(size <= PAGE_SIZE) || !size)
		return vaid_user_address(ptr);
	end = ptr + size - 1;
	return ptr <= end && valid_user_address(end);

Although it is questionable whether a zero size should be allowed.
Also, if you assume that the actual copies are 'reasonably sequential',
it is valid to just ignore the length completely.

It also ought to be possible to get the 'size == 0' check out of the common path.
Maybe something like:
	if (statically_true(size <= PAGE_SIZE)
		return vaid_user_address(ptr);
	end = ptr + size - 1;
	return (ptr <= end || (end++, !size)) && valid_user_address(end);
You might want a likely() around the <=, but I suspect it makes little
difference on modern x86 (esp. Intel ones).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Hi David,

Thanks for the review:

On Sunday, November 10, 2024 2:36:49 P.M. EST David Laight wrote:
> From: Mikel Rychliski
> 
> > Sent: 09 November 2024 21:03
> > 
> > We were checking one byte beyond the actual range that would be accessed.
> > Originally, valid_user_address would consider the user guard page to be
> > valid, so checks including the final accessible byte would still succeed.
> 
> Did it allow the entire page or just the first byte?
> The test for ignoring small constant sizes rather assumes that accesses
> to the guard page are errored (or transfers start with the first byte).
> 

valid_user_address() allowed the whole guard page. __access_ok() was 
inconsistent about ranges including the guard page (and, as you mention, would 
continue to be with this change).

The problem is before 86e6b1547b3d, the off-by-one calculation just lead to 
another harmless inconsistency in checks including the guard page. Now it 
prohibits reads of the last mapped userspace byte.

> > diff --git a/arch/x86/include/asm/uaccess_64.h
> > b/arch/x86/include/asm/uaccess_64.h index b0a887209400..3e0eb72c036f
> > 100644
> > --- a/arch/x86/include/asm/uaccess_64.h
> > +++ b/arch/x86/include/asm/uaccess_64.h
> > @@ -100,9 +100,11 @@ static inline bool __access_ok(const void __user
> > *ptr, unsigned long size)> 
> >  	if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) 
{
> >  	
> >  		return valid_user_address(ptr);
> >  	
> >  	} else {
> > 
> > -		unsigned long sum = size + (__force unsigned long)ptr;
> > +		unsigned long end = (__force unsigned long)ptr;
> > 
> > -		return valid_user_address(sum) && sum >= (__force 
unsigned long)ptr;
> > +		if (size)
> > +			end += size - 1;
> > +		return valid_user_address(end) && end >= (__force 
unsigned long)ptr;
> 
> Why not:
> 	if (statically_true(size <= PAGE_SIZE) || !size)
> 		return vaid_user_address(ptr);
> 	end = ptr + size - 1;
> 	return ptr <= end && valid_user_address(end);

Sure, agree this works as well.

> Although it is questionable whether a zero size should be allowed.
> Also, if you assume that the actual copies are 'reasonably sequential',
> it is valid to just ignore the length completely.
> 
> It also ought to be possible to get the 'size == 0' check out of the common
> path. Maybe something like:
> 	if (statically_true(size <= PAGE_SIZE)
> 		return vaid_user_address(ptr);
> 	end = ptr + size - 1;
> 	return (ptr <= end || (end++, !size)) && valid_user_address(end);

The first issue I ran into with the size==0 is that __import_iovec() is 
checking access for vectors with io_len==0 (and the check needs to succeed, 
otherwise userspace will get a -EFAULT). Not sure if there are others.

Similarly, the iovec case is depending on access_ok(0, 0) succeeding. So with 
the example here, end underflows and gets rejected.

From: Mikel Rychliski
> Sent: 11 November 2024 18:33
> 
> Hi David,
> 
> Thanks for the review:
> 
> On Sunday, November 10, 2024 2:36:49 P.M. EST David Laight wrote:
> > From: Mikel Rychliski
> >
> > > Sent: 09 November 2024 21:03
> > >
> > > We were checking one byte beyond the actual range that would be accessed.
> > > Originally, valid_user_address would consider the user guard page to be
> > > valid, so checks including the final accessible byte would still succeed.
> >
> > Did it allow the entire page or just the first byte?
> > The test for ignoring small constant sizes rather assumes that accesses
> > to the guard page are errored (or transfers start with the first byte).
> >
> 
> valid_user_address() allowed the whole guard page. __access_ok() was
> inconsistent about ranges including the guard page (and, as you mention, would
> continue to be with this change).
> 
> The problem is before 86e6b1547b3d, the off-by-one calculation just lead to
> another harmless inconsistency in checks including the guard page. Now it
> prohibits reads of the last mapped userspace byte.

So if you could find code that didn't read the first byte of a short buffer
first you could access the first page of kernel memory.
(Ignoring the STAC/CLAC instructions.)
So that has always been wrong!

OTOH I suspect that all user accesses start with the first byte
and are either 'reasonably sequential' or recheck an updated pointer.
So an architecture with a guard page (not all do) need only check
the base address of a user buffer for being below/equal to the
guard page.

...
> > Why not:
> > 	if (statically_true(size <= PAGE_SIZE) || !size)
> > 		return vaid_user_address(ptr);
> > 	end = ptr + size - 1;
> > 	return ptr <= end && valid_user_address(end);
> 
> Sure, agree this works as well.

But is likely to replicate the valid_user_address() code.

> > Although it is questionable whether a zero size should be allowed.
> > Also, if you assume that the actual copies are 'reasonably sequential',
> > it is valid to just ignore the length completely.
> >
> > It also ought to be possible to get the 'size == 0' check out of the common
> > path. Maybe something like:
> > 	if (statically_true(size <= PAGE_SIZE)
> > 		return vaid_user_address(ptr);
> > 	end = ptr + size - 1;
> > 	return (ptr <= end || (end++, !size)) && valid_user_address(end);
> 
> The first issue I ran into with the size==0 is that __import_iovec() is
> checking access for vectors with io_len==0 (and the check needs to succeed,
> otherwise userspace will get a -EFAULT). Not sure if there are others.

I've looked at __import_iovec() in the past.
The API is horrid! and the 32bit compat version is actually faster.
It doesn't need to call access_ok() either the check is done later.

> Similarly, the iovec case is depending on access_ok(0, 0) succeeding. So with
> the example here, end underflows and gets rejected.

I've even wondered what the actual issue is with speculative kernel
reads from get_user().
The read itself can't be an issue (a valid user address will also displace
any cache lines), so I think the value read must be used to form an
address in order for any kernel data to be leaked.
You might find a compare (eg the length in import_iovec() but that can
only expose high bits of a byte - and probably requires i-cache timing.
But I'm not expert - and the experts hide the fine details.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

From: David Laight
> Sent: 10 November 2024 19:37
> 
> From: Mikel Rychliski
> > Sent: 09 November 2024 21:03
> >
> > We were checking one byte beyond the actual range that would be accessed.
> > Originally, valid_user_address would consider the user guard page to be
> > valid, so checks including the final accessible byte would still succeed.
> 
> Did it allow the entire page or just the first byte?
> The test for ignoring small constant sizes rather assumes that accesses
> to the guard page are errored (or transfers start with the first byte).
> 
> > However, after commit 86e6b1547b3d ("x86: fix user address masking
> > non-canonical speculation issue") this is no longer the case.
> >
> > Update the logic to always consider the final address in the range.
> >
> > Fixes: 86e6b1547b3d ("x86: fix user address masking non-canonical speculation issue")
> > Signed-off-by: Mikel Rychliski <mikel@mikelr.com>
> > ---
> >  arch/x86/include/asm/uaccess_64.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
> > index b0a887209400..3e0eb72c036f 100644
> > --- a/arch/x86/include/asm/uaccess_64.h
> > +++ b/arch/x86/include/asm/uaccess_64.h
> > @@ -100,9 +100,11 @@ static inline bool __access_ok(const void __user *ptr, unsigned long size)
> >  	if (__builtin_constant_p(size <= PAGE_SIZE) && size <= PAGE_SIZE) {
> >  		return valid_user_address(ptr);
> >  	} else {
> > -		unsigned long sum = size + (__force unsigned long)ptr;
> > +		unsigned long end = (__force unsigned long)ptr;
> >
> > -		return valid_user_address(sum) && sum >= (__force unsigned long)ptr;
> > +		if (size)
> > +			end += size - 1;
> > +		return valid_user_address(end) && end >= (__force unsigned long)ptr;
> 
> Why not:
> 	if (statically_true(size <= PAGE_SIZE) || !size)
> 		return vaid_user_address(ptr);
> 	end = ptr + size - 1;
> 	return ptr <= end && valid_user_address(end);

Thinking more that version probably bloats the code with two
copies of valid_user_address().

> Although it is questionable whether a zero size should be allowed.
> Also, if you assume that the actual copies are 'reasonably sequential',
> it is valid to just ignore the length completely.
> 
> It also ought to be possible to get the 'size == 0' check out of the common path.
> Maybe something like:
> 	if (statically_true(size <= PAGE_SIZE)
> 		return vaid_user_address(ptr);
> 	end = ptr + size - 1;
> 	return (ptr <= end || (end++, !size)) && valid_user_address(end);
> You might want a likely() around the <=, but I suspect it makes little
> difference on modern x86 (esp. Intel ones).

I can't actually remember Linus's final version and don't have it to hand.
But I'm sure I remember something about a 64bit address constant.
(Probably 'cmpi, sbb, or' and then test the sign or carry flag.)
It might just be enough to increase that by one so that buffers that
end at the boundary are ok.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)