Re: [PATCH] x86: Fix off-by-one error in __access_ok

[Mikel Rychliski <mikel@mikelr.com>]

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.