Hi,
I suggest to include the commit: 594cc251fdd0 make 'user_access_begin()'
do 'access_ok()' for CVE-2018-20669.
stable version to apply to: kernel-4.14.y and kernel-4.19.y.
From the discussion below, I checked the latest kernel and found that we
should also apply other 4 patches. (total 5 patches)
https://lkml.org/lkml/2020/5/12/943
patch list:
commit ab10ae1c3bef lib: Reduce user_access_begin() boundaries in
strncpy_from_user() and strnlen_user()
commit 6e693b3ffecb x86: uaccess: Inhibit speculation past access_ok()
in user_access_begin()
commit 9cb2feb4d21d arch/openrisc: Fix issues with access_ok()
commit 94bd8a05cd4d Fix 'acccess_ok()' on alpha and SH
commit 594cc251fdd0 make 'user_access_begin()' do 'access_ok()'
Where only commit 6e693b3ffecb does not need backport modifications.
I attach my backport patches in this email.
I merged the patches with kernel-4.19.127 and kernel-4.14.183 without
conflicts.
Build with arm64 defconfig and bootup on arm64 QEMU environment.
cheers,
Miles
From ac351de9ddd86ef717a3f89236dc5f6b2a108cc7 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Fri, 4 Jan 2019 12:56:09 -0800
Subject: [PATCH] BACKPORT: make 'user_access_begin()' do 'access_ok()'
upstream commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
Originally, the rule used to be that you'd have to do access_ok()
separately, and then user_access_begin() before actually doing the
direct (optimized) user access.
But experience has shown that people then decide not to do access_ok()
at all, and instead rely on it being implied by other operations or
similar. Which makes it very hard to verify that the access has
actually been range-checked.
If you use the unsafe direct user accesses, hardware features (either
SMAP - Supervisor Mode Access Protection - on x86, or PAN - Privileged
Access Never - on ARM) do force you to use user_access_begin(). But
nothing really forces the range check.
By putting the range check into user_access_begin(), we actually force
people to do the right thing (tm), and the range check vill be visible
near the actual accesses. We have way too long a history of people
trying to avoid them.
Bug: 135368228
Change-Id: I4ca0e4566ea080fa148c5e768bb1a0b6f7201c01
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
---
arch/x86/include/asm/uaccess.h | 12 +++++++++++-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 16 ++++++++++++++--
include/linux/uaccess.h | 2 +-
kernel/compat.c | 6 ++----
kernel/exit.c | 6 ++----
lib/strncpy_from_user.c | 9 +++++----
lib/strnlen_user.c | 9 +++++----
7 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 4111edb3188e..4128e6fdfcc4 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -709,7 +709,17 @@ extern struct movsl_mask {
* checking before using them, but you have to surround them with the
* user_access_begin/end() pair.
*/
-#define user_access_begin() __uaccess_begin()
+static __must_check inline bool user_access_begin(int type,
+ const void __user *ptr,
+ size_t len)
+{
+ if (unlikely(!access_ok(type, ptr, len)))
+ return 0;
+ __uaccess_begin();
+ return 1;
+}
+
+#define user_access_begin(a, b, c) user_access_begin(a, b, c)
#define user_access_end() __uaccess_end()
#define unsafe_put_user(x, ptr, err_label) \
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 679bbae52945..04188e5168a2 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1602,7 +1602,9 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb)
* happened we would make the mistake of assuming that the
* relocations were valid.
*/
- user_access_begin();
+ if (!user_access_begin(VERIFY_WRITE, urelocs, size))
+ goto end_user;
+
for (copied = 0; copied < nreloc; copied++)
unsafe_put_user(-1,
&urelocs[copied].presumed_offset,
@@ -2601,7 +2603,17 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data,
unsigned int i;
/* Copy the new buffer offsets back to the user's exec list. */
- user_access_begin();
+ /*
+ * Note: count * sizeof(*user_exec_list) does not overflow,
+ * because we checked 'count' in check_buffer_count().
+ *
+ * And this range already got effectively checked earlier
+ * when we did the "copy_from_user()" above.
+ */
+ if (!user_access_begin(VERIFY_WRITE, user_exec_list,
+ count * sizeof(*user_exec_list)))
+ goto end_user;
+
for (i = 0; i < args->buffer_count; i++) {
if (!(exec2_list[i].offset & UPDATE))
continue;
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..d55b68b113de 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -267,7 +267,7 @@ extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
probe_kernel_read(&retval, addr, sizeof(retval))
#ifndef user_access_begin
-#define user_access_begin() do { } while (0)
+#define user_access_begin(type, ptr, len) access_ok(type, ptr, len)
#define user_access_end() do { } while (0)
#define unsafe_get_user(x, ptr, err) do { if (unlikely(__get_user(x, ptr))) goto err; } while (0)
#define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0)
diff --git a/kernel/compat.c b/kernel/compat.c
index 8e40efc2928a..e4548a9e9c52 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -354,10 +354,9 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask,
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!access_ok(VERIFY_READ, umask, bitmap_size / 8))
+ if (!user_access_begin(VERIFY_READ, umask, bitmap_size / 8))
return -EFAULT;
- user_access_begin();
while (nr_compat_longs > 1) {
compat_ulong_t l1, l2;
unsafe_get_user(l1, umask++, Efault);
@@ -384,10 +383,9 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask,
bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG);
nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size);
- if (!access_ok(VERIFY_WRITE, umask, bitmap_size / 8))
+ if (!user_access_begin(VERIFY_WRITE, umask, bitmap_size / 8))
return -EFAULT;
- user_access_begin();
while (nr_compat_longs > 1) {
unsigned long m = *mask++;
unsafe_put_user((compat_ulong_t)m, umask++, Efault);
diff --git a/kernel/exit.c b/kernel/exit.c
index e6d8ba1cd2e2..213ba451dcb3 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1614,10 +1614,9 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *,
if (!infop)
return err;
- if (!access_ok(VERIFY_WRITE, infop, sizeof(*infop)))
+ if (!user_access_begin(VERIFY_WRITE, infop, sizeof(*infop)))
return -EFAULT;
- user_access_begin();
unsafe_put_user(signo, &infop->si_signo, Efault);
unsafe_put_user(0, &infop->si_errno, Efault);
unsafe_put_user(info.cause, &infop->si_code, Efault);
@@ -1742,10 +1741,9 @@ COMPAT_SYSCALL_DEFINE5(waitid,
if (!infop)
return err;
- if (!access_ok(VERIFY_WRITE, infop, sizeof(*infop)))
+ if (!user_access_begin(VERIFY_WRITE, infop, sizeof(*infop)))
return -EFAULT;
- user_access_begin();
unsafe_put_user(signo, &infop->si_signo, Efault);
unsafe_put_user(0, &infop->si_errno, Efault);
unsafe_put_user(info.cause, &infop->si_code, Efault);
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index e304b54c9c7d..b8570a11776d 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -115,10 +115,11 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
kasan_check_write(dst, count);
check_object_size(dst, count, false);
- user_access_begin();
- retval = do_strncpy_from_user(dst, src, count, max);
- user_access_end();
- return retval;
+ if (user_access_begin(VERIFY_READ, src, max)) {
+ retval = do_strncpy_from_user(dst, src, count, max);
+ user_access_end();
+ return retval;
+ }
}
return -EFAULT;
}
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 184f80f7bacf..f5fa5b266ea2 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -114,10 +114,11 @@ long strnlen_user(const char __user *str, long count)
unsigned long max = max_addr - src_addr;
long retval;
- user_access_begin();
- retval = do_strnlen_user(str, count, max);
- user_access_end();
- return retval;
+ if (user_access_begin(VERIFY_READ, str, max)) {
+ retval = do_strnlen_user(str, count, max);
+ user_access_end();
+ return retval;
+ }
}
return 0;
}
--
2.18.0
From 9f2bf975bc5f8a9bf873e5a5534fca1b2eadb073 Mon Sep 17 00:00:00 2001
From: Miles Chen <miles.chen@xxxxxxxxxxxx>
Date: Wed, 10 Jun 2020 23:58:37 +0800
Subject: [PATCH] BACKPORT: Fix 'access_ok()' on alpha and SH
Commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
broke both alpha and SH booting in qemu, as noticed by Guenter Roeck.
It turns out that the bug wasn't actually in that commit itself (which
would have been surprising: it was mostly a no-op), but in how the
addition of access_ok() to the strncpy_from_user() and strnlen_user()
functions now triggered the case where those functions would test the
access of the very last byte of the user address space.
The string functions actually did that user range test before too, but
they did it manually by just comparing against user_addr_max(). But
with user_access_begin() doing the check (using "access_ok()"), it now
exposed problems in the architecture implementations of that function.
For example, on alpha, the access_ok() helper macro looked like this:
#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 masking value is 0xfffffc0000000000).
And that's completely wrong for the "addr+size" check. Because 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. As a result, the
user string accessor functions failed consistently - because they
literally don't know how long the string is going to be, and the max
access is going to be that last byte of the user address space.
Side note: that alpha macro is buggy for another reason too - it re-uses
the arguments twice.
And SH has another version of 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 (think "one
byte access at the last address of the user address space")
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 buggy
implementations of access_ok(), but they happened to work in practice
(although the SH overflow one is a serious serious security bug, not
that anybody likely cares about SH security).
This fixes the problems by using a similar macro on both alpha and SH.
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 "add start and length, and then subtract one unless
the length was zero". We can't subtract one for a zero length, or we'd
just hit an underflow instead.
For a lot of access_ok() users the length is a constant, so this isn't
actually as expensive as it initially looks.
Reported-and-tested-by: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: Matt Turner <mattst88@xxxxxxxxx>
Cc: Yoshinori Sato <ysato@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Miles Chen <miles.chen@xxxxxxxxxxxx>
---
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 87d8c4f0307d..7295967b5028 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(type, addr, size) \
({ \
diff --git a/arch/sh/include/asm/uaccess.h b/arch/sh/include/asm/uaccess.h
index 32eb56e00c11..6e7816360a75 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(type, addr, size) \
(__chk_user_ptr(addr), \
__access_ok((unsigned long __force)(addr), (size)))
--
2.18.0
From 5f9bbadfd9ecd4b640c2504162e7c319b704a73a Mon Sep 17 00:00:00 2001
From: Miles Chen <miles.chen@xxxxxxxxxxxx>
Date: Thu, 11 Jun 2020 00:38:38 +0800
Subject: [PATCH] BACKPORT: arch/openrisc: Fix issues with access_ok()
The commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
exposed incorrect implementations of access_ok() macro in several
architectures. This change fixes 2 issues found in OpenRISC.
OpenRISC was not properly using parenthesis for arguments and also using
arguments twice. This patch fixes those 2 issues.
I test booted this patch with v5.0-rc1 on qemu and it's working fine.
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Stafford Horne <shorne@xxxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Miles Chen <miles.chen@xxxxxxxxxxxx>
---
arch/openrisc/include/asm/uaccess.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/openrisc/include/asm/uaccess.h b/arch/openrisc/include/asm/uaccess.h
index bbf5c79cce7a..8b204cd1f531 100644
--- a/arch/openrisc/include/asm/uaccess.h
+++ b/arch/openrisc/include/asm/uaccess.h
@@ -58,8 +58,12 @@
/* Ensure that addr is below task's addr_limit */
#define __addr_ok(addr) ((unsigned long) addr < get_fs())
-#define access_ok(type, addr, size) \
- __range_ok((unsigned long)addr, (unsigned long)size)
+#define access_ok(type, addr, size) \
+({ \
+ unsigned long __ao_addr = (unsigned long)(addr); \
+ unsigned long __ao_size = (unsigned long)(size); \
+ __range_ok(__ao_addr, __ao_size); \
+})
/*
* These are the main single-value transfer routines. They automatically
--
2.18.0
From 6e693b3ffecb0b478c7050b44a4842854154f715 Mon Sep 17 00:00:00 2001
From: Will Deacon <will.deacon@xxxxxxx>
Date: Sat, 19 Jan 2019 21:56:05 +0000
Subject: [PATCH] x86: uaccess: Inhibit speculation past access_ok() in
user_access_begin()
Commit 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
makes the access_ok() check part of the user_access_begin() preceding a
series of 'unsafe' accesses. This has the desirable effect of ensuring
that all 'unsafe' accesses have been range-checked, without having to
pick through all of the callsites to verify whether the appropriate
checking has been made.
However, the consolidated range check does not inhibit speculation, so
it is still up to the caller to ensure that they are not susceptible to
any speculative side-channel attacks for user addresses that ultimately
fail the access_ok() check.
This is an oversight, so use __uaccess_begin_nospec() to ensure that
speculation is inhibited until the access_ok() check has passed.
Reported-by: Julien Thierry <julien.thierry@xxxxxxx>
Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
---
arch/x86/include/asm/uaccess.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index a77445d1b034..780f2b42c8ef 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -711,7 +711,7 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
{
if (unlikely(!access_ok(ptr,len)))
return 0;
- __uaccess_begin();
+ __uaccess_begin_nospec();
return 1;
}
#define user_access_begin(a,b) user_access_begin(a,b)
--
2.18.0
From 31e6d213e901f1d4c24c358cd8af0d0d6dff9933 Mon Sep 17 00:00:00 2001
From: Miles Chen <miles.chen@xxxxxxxxxxxx>
Date: Thu, 11 Jun 2020 00:43:11 +0800
Subject: [PATCH] BACKPORT: lib: Reduce user_access_begin() boundaries in
strncpy_from_user() and strnlen_user()
The range passed to user_access_begin() by strncpy_from_user() and
strnlen_user() starts at 'src' and goes up to the limit of userspace
although reads will be limited by the 'count' param.
On 32 bits powerpc (book3s/32) access has to be granted for each
256Mbytes segment and the cost increases with the number of segments to
unlock.
Limit the range with 'count' param.
Fixes: 594cc251fdd0 ("make 'user_access_begin()' do 'access_ok()'")
Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Miles Chen <miles.chen@xxxxxxxxxxxx>
---
lib/strncpy_from_user.c | 14 +++++++-------
lib/strnlen_user.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index b8570a11776d..fc5b1e2d997d 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -29,13 +29,6 @@ static inline long do_strncpy_from_user(char *dst, const char __user *src,
const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
unsigned long res = 0;
- /*
- * Truncate 'max' to the user-specified limit, so that
- * we only have one limit we need to check in the loop
- */
- if (max > count)
- max = count;
-
if (IS_UNALIGNED(src, dst))
goto byte_at_a_time;
@@ -113,6 +106,13 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
unsigned long max = max_addr - src_addr;
long retval;
+ /*
+ * Truncate 'max' to the user-specified limit, so that
+ * we only have one limit we need to check in the loop
+ */
+ if (max > count)
+ max = count;
+
kasan_check_write(dst, count);
check_object_size(dst, count, false);
if (user_access_begin(VERIFY_READ, src, max)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index f5fa5b266ea2..0bf7c06ebdad 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -31,13 +31,6 @@ static inline long do_strnlen_user(const char __user *src, unsigned long count,
unsigned long align, res = 0;
unsigned long c;
- /*
- * Truncate 'max' to the user-specified limit, so that
- * we only have one limit we need to check in the loop
- */
- if (max > count)
- max = count;
-
/*
* Do everything aligned. But that means that we
* need to also expand the maximum..
@@ -114,6 +107,13 @@ long strnlen_user(const char __user *str, long count)
unsigned long max = max_addr - src_addr;
long retval;
+ /*
+ * Truncate 'max' to the user-specified limit, so that
+ * we only have one limit we need to check in the loop
+ */
+ if (max > count)
+ max = count;
+
if (user_access_begin(VERIFY_READ, str, max)) {
retval = do_strnlen_user(str, count, max);
user_access_end();
--
2.18.0
