Messages in this thread | | | Subject | Re: [PATCH] make 'user_access_begin()' do 'access_ok()' | From | Guenter Roeck <> | Date | Sun, 6 Jan 2019 12:24:11 -0800 |
| |
On 1/6/19 11:18 AM, Linus Torvalds wrote: > [ Re-sending the message because my first reply bounced - Guenther had > mis-typed the lkml address ] >
Sigh. That _always_ happens to me when typing fast. Sorry.
> On Sun, Jan 6, 2019 at 10:09 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> All alpha and sh4 (big and little endian) images fail to boot in qemu >> with this patch applied. Reverting it fixes the problem. > > Funky. 99% of that patch is a complete no-op on non-x86. > > The one exception is the strncpy_from_user() and strnlen_user() cases, > which didn't use to do access_ok() at all, and now essentially do. > > But I think I see what may be the problem. I think the alpha version > of "access_ok()" is buggy. > > Lookie here: > > #define __access_ok(addr, size) \ > ((get_fs().seg & (addr | size | (addr+size))) == 0) > > and what it basically tests is of any of the high bits get set (the > USER_DS value is 0xfffffc0000000000). > > And that's completely wrong for the "addr+size" check. It's off-by-one > for the case where we check to the very end of the user address space, > which is exactly what the strn*_user() functions do. > > Why? Because "addr+size" will be exactly the size of the address > space, so trying to access the last byte of the user address space > will *fail* the __access_ok() check, even though it shouldn't. > > So it's not really that that commit is buggy in itself, but it > triggers that off-by-one error in access_ok(). > > Side note: that alpha macro is buggy for another reason too: it > re-uses the arguments twice. > > And SH has almost the exact same bug: > > #define __addr_ok(addr) \ > ((unsigned long __force)(addr) < current_thread_info()->addr_limit.seg) > > so far so good: yes, a user address must be below the limit. But then: > > #define __access_ok(addr, size) \ > (__addr_ok((addr) + (size))) > > is wrong with the exact same off-by-one case: the case when > "addr+size" is exactly _equal_ to the limit is actually perfectly > fine. > > The SH version is actually seriously buggy in another way: it doesn't > actually check for overflow, even though it did copy the _comment_ > that talks about overflow. > > So it turns out that both SH and alpha actually have completely > buggered implementations of access_ok(), but they happened to work > (although the SH overflow one is a serious serious security bug, not > that anybody likely cares about SH security) > > Ho humm. > > Maybe something like the attached patch? Entirely untested, I don't > have a cross-build environment, much less a boot setup. > > It isn't trying to be clever, the end address is based on this logic: > > unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b; \ > > which basically says "subtract one unless the length was zero". > > For a lot of access_ok() users the length is a constant, so this isn't > actually as expensive as it initially looks. > > Does that fix things for you? >
Yes, it does, for both alpha and sh (little and big endian).
Tested-by: Guenter Roeck <linux@roeck-us.net>
Guenter
| |