Re: [PATCH] make 'user_access_begin()' do 'access_ok()'

[Linus Torvalds]

[ Re-sending the message because my first reply bounced - Guenther had
mis-typed the lkml address ]

On Sun, Jan 6, 2019 at 10:09 AM Guenter Roeck <linux@xxxxxxxxxxxx> 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?

                   Linus
 arch/alpha/include/asm/uaccess.h | 8 +++++---
 arch/sh/include/asm/uaccess.h    | 7 +++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/alpha/include/asm/uaccess.h b/arch/alpha/include/asm/uaccess.h
index e69c4e13c328..cf4ac791a592 100644
--- a/arch/alpha/include/asm/uaccess.h
+++ b/arch/alpha/include/asm/uaccess.h
@@ -30,11 +30,13 @@
  * Address valid if:
  *  - "addr" doesn't have any high-bits set
  *  - AND "size" doesn't have any high-bits set
- *  - AND "addr+size" doesn't have any high-bits set
+ *  - AND "addr+size-(size != 0)" doesn't have any high-bits set
  *  - OR we are in kernel mode.
  */
-#define __access_ok(addr, size) \
-	((get_fs().seg & (addr | size | (addr+size))) == 0)
+#define __access_ok(addr, size) ({				\
+	unsigned long __ao_a = (addr), __ao_b = (size);		\
+	unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b;	\
+	(get_fs().seg & (__ao_a | __ao_b | __ao_end)) == 0; })
 
 #define access_ok(addr, size)				\
 ({							\
diff --git a/arch/sh/include/asm/uaccess.h b/arch/sh/include/asm/uaccess.h
index deebbfab5342..5fe751ad7582 100644
--- a/arch/sh/include/asm/uaccess.h
+++ b/arch/sh/include/asm/uaccess.h
@@ -16,8 +16,11 @@
  * sum := addr + size;  carry? --> flag = true;
  * if (sum >= addr_limit) flag = true;
  */
-#define __access_ok(addr, size)		\
-	(__addr_ok((addr) + (size)))
+#define __access_ok(addr, size)	({				\
+	unsigned long __ao_a = (addr), __ao_b = (size);		\
+	unsigned long __ao_end = __ao_a + __ao_b - !!__ao_b;	\
+	__ao_end >= __ao_a && __addr_ok(__ao_end); })
+
 #define access_ok(addr, size)	\
 	(__chk_user_ptr(addr),		\
 	 __access_ok((unsigned long __force)(addr), (size)))