On Wed, 2020-06-10 at 20:02 +0200, Greg KH wrote:
> On Thu, Jun 11, 2020 at 01:58:20AM +0800, Miles Chen wrote:
> > 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>
>
> No need for "Bug:" or "Change-Id:" for patches for stable trees.
>
> Also, can you please sign off on these as well?
>
> Can you fix that up and resend? I'll be glad to queue them up then.
>
> thanks,
Remove the "Bug/Change-Id" from
0001-BACKPORT-make-user_access_begin-do-access_ok.patch.
Actually, I got the patch from
https://android-review.googlesource.com/c/kernel/common/+/1114632
Todd backported the patch but there is no Todd's signed-off-by in his
patch. Should I add "Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>" as
well?
cheers
Miles
>
> greg k-h
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.
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Miles Chen <miles.chen@xxxxxxxxxxxx>
---
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
