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

Mikel Rychliski
2024-11-11T18:33:18Z

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.