Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Sat, Aug 29, 2020 at 07:31:20PM +0000, David Laight wrote:
> > Rereading the patch it looks like a lot of bloat (as well as a
> > lot of changes).
> > Does the array_mask even work on 32bit archs where the kernel
> > base address is 0xc0000000?

Why wouldn't it on work on 32-bit?  My patch does have a minor compile
bug on 32-bit, but otherwise it seems to work (i.e., the asm looks ok,
and it boots).

> > I'm sure there is something much simpler.
> > 
> > If access_ok() generates ~0u or 0 without a conditional then
> > the address can be masked with the result.
> > So you probably need to change access_ok() to take the address
> > of the user pointer - so the callers become like:
> > 	if (access_ok(&user_buffer, len))
> > 		return -EFAULT
> > 	__put_user(user_buffer, value);
> > 
> > It would be easier if NULL were guaranteed to be an invalid
> > user address (is it?).
> > Then access_ok() could return the modified pointer.
> > So you get something like:
> > 	user_buffer = access_ok(user_buffer, len);
> > 	if (!user_buffer)
> > 		return -EFAULT.
> > 
> > Provided the 'last' user page is never allocated (it can't
> > be on i386 due to cpu prefetch issues) something like:
> > (and with the asm probably all broken)
> > 
> > static inline void __user * access_ok(void __user *b, size_t len)
> > {
> > 	unsigned long x = (long)b | (long)(b + len);
> > 	unsigned long lim = 64_bit ? 1u << 63 : 0x40000000;
> > 	asm volatile (" add %1, %0\n"
> > 			" sbb $0, %0", "=r" (x), "r" (lim));
> > 	return (void __user *)(long)b & ~x);
> > }
> 
> Actually, thinking further, if:
> 1) the access_ok() immediately precedes the user copy (as it should).
> 2) the user-copies use a sensible 'increasing address' copy.
> and
> 3) there is a 'guard page' between valid user and kernel addresses.
> Then access_ok() only need check the base address of the user buffer.

Yes, it would make sense to put the masking in access_ok() somehow.  But
to do it properly, I think we'd first need to make access_ok() generic.
Maybe that's do-able, but it would be a much bigger patch set.

First I'd prefer to just fix x86, like my patch does.  Then we could do
an access_ok() rework.

-- 
Josh