Messages in this thread | | | Date | Mon, 31 Aug 2020 12:31:12 -0500 | From | Josh Poimboeuf <> | Subject | Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation |
| |
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
| |