Message-ID: <6a4fe075-a644-1b06-305b-9e55b8c9575b@roeck-us.net>
Date:   Sun, 6 Jan 2019 12:24:11 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Matt Turner <mattst88@...il.com>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>
Cc:     Linux List Kernel Mailing <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] make 'user_access_begin()' do 'access_ok()'

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@...ck-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@...ck-us.net>

Guenter

Powered by blists - more mailing lists