From: Linus Torvalds <[email protected]>
commit 594cc251fdd0d231d342d88b2fdff4bc42fb0690 upstream.
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 <[email protected]>
Signed-off-by: Ashwin H <[email protected]>
---
arch/x86/include/asm/uaccess.h | 11 ++++++++++-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++--
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, 38 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 9718303..82cd874 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -711,7 +711,16 @@ 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(const bool 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(t, a, b) user_access_begin(t, a, b)
#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 f08c547..7110e8f 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1604,7 +1604,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,
@@ -2649,7 +2651,16 @@ 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 efe79c1..d55b68b 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 8e40efc..e4548a9 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 54c3269..894fca5 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1617,10 +1617,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);
@@ -1745,10 +1744,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 e304b54..b8570a1 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 184f80f..f5fa5b2 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.7.4
On Wed, May 13, 2020 at 07:19:21AM +0530, ashwin-h wrote:
> From: Linus Torvalds <[email protected]>
>
> commit 594cc251fdd0d231d342d88b2fdff4bc42fb0690 upstream.
>
> 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 <[email protected]>
> Signed-off-by: Ashwin H <[email protected]>
> ---
> arch/x86/include/asm/uaccess.h | 11 ++++++++++-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++--
> 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, 38 insertions(+), 20 deletions(-)
Are you wanting this merged to a specific stable kernel tree? If so, why?
thanks,
greg k-h
This patch fixes CVE-2018-20669 in 4.19 tree.
On 13/05/20, 11:36 AM, "Greg KH" <[email protected]> wrote:
On Wed, May 13, 2020 at 07:19:21AM +0530, ashwin-h wrote:
> From: Linus Torvalds <[email protected]>
>
> commit 594cc251fdd0d231d342d88b2fdff4bc42fb0690 upstream.
>
> 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 <[email protected]>
> Signed-off-by: Ashwin H <[email protected]>
> ---
> arch/x86/include/asm/uaccess.h | 11 ++++++++++-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 +++++++++++++--
> 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, 38 insertions(+), 20 deletions(-)
Are you wanting this merged to a specific stable kernel tree? If so, why?
thanks,
greg k-h
On Wed, May 13, 2020 at 06:13:17AM +0000, Ashwin H wrote:
> This patch fixes CVE-2018-20669 in 4.19 tree.
Ok, but what does that mean for us?
You need to say why you are sending a patch, otherwise we will guess
wrong.
greg k-h
> Ok, but what does that mean for us?
>
> You need to say why you are sending a patch, otherwise we will guess wrong.
In drivers/gpu/drm/i915/i915_gem_execbuffer.c, ioctl functions does user_access_begin() without doing access_ok(Checks if a user space pointer is valid) first.
A local attacker can craft a malicious ioctl function call to overwrite arbitrary kernel memory, resulting in a Denial of Service or privilege escalation (CVE-2018-20669)
This patch makes sure that user_access_begin always does access_ok.
user_access_begin has been modified to do access_ok internally.
Thanks,
Ashwin
On Wed, May 13, 2020 at 05:08:19PM +0000, Ashwin H wrote:
> > Ok, but what does that mean for us?
> >
> > You need to say why you are sending a patch, otherwise we will guess wrong.
>
> In drivers/gpu/drm/i915/i915_gem_execbuffer.c, ioctl functions does user_access_begin() without doing access_ok(Checks if a user space pointer is valid) first.
> A local attacker can craft a malicious ioctl function call to overwrite arbitrary kernel memory, resulting in a Denial of Service or privilege escalation (CVE-2018-20669)
>
> This patch makes sure that user_access_begin always does access_ok.
> user_access_begin has been modified to do access_ok internally.
I had this in the tree, but it broke the build on alpha, sh, and maybe a
few others :(
See:
https://lore.kernel.org/r/[email protected]
for the details.
Can you dig out all of the needed follow-on patches as well, and send
them all as a patch series for 4.19.y so that I can queue them all up at
once?
thanks,
greg k-h
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: Wednesday, May 27, 2020 9:02 PM
> To: Ashwin H <[email protected]>
> Cc: [email protected]; [email protected]; intel-
> [email protected]; [email protected]; [email protected];
> Srivatsa Bhat <[email protected]>; [email protected];
> [email protected]; Steven Rostedt <[email protected]>; Linus
> Torvalds <[email protected]>
> Subject: Re: [PATCH v4.19.x] make 'user_access_begin()' do 'access_ok()'
>
> On Wed, May 13, 2020 at 05:08:19PM +0000, Ashwin H wrote:
> > > Ok, but what does that mean for us?
> > >
> > > You need to say why you are sending a patch, otherwise we will guess
> wrong.
> >
> > In drivers/gpu/drm/i915/i915_gem_execbuffer.c, ioctl functions does
> user_access_begin() without doing access_ok(Checks if a user space pointer
> is valid) first.
> > A local attacker can craft a malicious ioctl function call to
> > overwrite arbitrary kernel memory, resulting in a Denial of Service or
> > privilege escalation (CVE-2018-20669)
> >
> > This patch makes sure that user_access_begin always does access_ok.
> > user_access_begin has been modified to do access_ok internally.
>
> I had this in the tree, but it broke the build on alpha, sh, and maybe a few
> others :(
>
Thanks Greg for including this patch.
I am sorry that this patch caused the failure. As I see this is not a build failure but tests have failed.
Build results:
total: 155 pass: 155 fail: 0
Qemu test results:
total: 421 pass: 390 fail: 31
Failed tests:
<all alpha>
<all sh>
<all sheb>
> See:
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Flore.kernel.org%2Fr%2F20200527140225.GA214763%40roeck-
> us.net&data=02%7C01%7Cashwinh%40vmware.com%7Cd8f60bb8a4584
> 7caa10f08d802530997%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7
> C637261902960990057&sdata=Vjv9v0QhebfcOGSq1UUDKshTDA%2FOV
> 4aKbqzKKJkEQxM%3D&reserved=0
> for the details.
>
> Can you dig out all of the needed follow-on patches as well, and send them
> all as a patch series for 4.19.y so that I can queue them all up at once?
>
I will check for follow-on patches and get back.
> thanks,
>
> greg k-h
Thanks,
Ashwin
> -----Original Message-----
> From: Ashwin H
> Sent: Thursday, May 28, 2020 1:01 PM
> To: Greg KH <[email protected]>
> Cc: [email protected]; [email protected]; intel-
> [email protected]; [email protected]; [email protected];
> Srivatsa Bhat <[email protected]>; [email protected];
> [email protected]; Steven Rostedt <[email protected]>; Linus
> Torvalds <[email protected]>
> Subject: RE: [PATCH v4.19.x] make 'user_access_begin()' do 'access_ok()'
>
>
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: Wednesday, May 27, 2020 9:02 PM
> > To: Ashwin H <[email protected]>
> > Cc: [email protected]; [email protected]; intel-
> > [email protected]; [email protected];
> > [email protected]; Srivatsa Bhat <[email protected]>;
> > [email protected]; [email protected]; Steven Rostedt
> > <[email protected]>; Linus Torvalds <[email protected]>
> > Subject: Re: [PATCH v4.19.x] make 'user_access_begin()' do 'access_ok()'
> >
> > On Wed, May 13, 2020 at 05:08:19PM +0000, Ashwin H wrote:
> > > > Ok, but what does that mean for us?
> > > >
> > > > You need to say why you are sending a patch, otherwise we will
> > > > guess
> > wrong.
> > >
> > > In drivers/gpu/drm/i915/i915_gem_execbuffer.c, ioctl functions does
> > user_access_begin() without doing access_ok(Checks if a user space
> > pointer is valid) first.
> > > A local attacker can craft a malicious ioctl function call to
> > > overwrite arbitrary kernel memory, resulting in a Denial of Service
> > > or privilege escalation (CVE-2018-20669)
> > >
> > > This patch makes sure that user_access_begin always does access_ok.
> > > user_access_begin has been modified to do access_ok internally.
> >
> > I had this in the tree, but it broke the build on alpha, sh, and maybe
> > a few others :(
> >
> Thanks Greg for including this patch.
> I am sorry that this patch caused the failure. As I see this is not a build failure
> but tests have failed.
> Build results:
> total: 155 pass: 155 fail: 0
> Qemu test results:
> total: 421 pass: 390 fail: 31
> Failed tests:
> <all alpha>
> <all sh>
> <all sheb>
>
> > See:
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F
> > %2Flore.kernel.org%2Fr%2F20200527140225.GA214763%40roeck-
> >
> us.net&data=02%7C01%7Cashwinh%40vmware.com%7Cd8f60bb8a4584
> >
> 7caa10f08d802530997%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7
> >
> C637261902960990057&sdata=Vjv9v0QhebfcOGSq1UUDKshTDA%2FOV
> > 4aKbqzKKJkEQxM%3D&reserved=0
> > for the details.
> >
> > Can you dig out all of the needed follow-on patches as well, and send
> > them all as a patch series for 4.19.y so that I can queue them all up at once?
> >
>
> I will check for follow-on patches and get back.
This seems to be the issue in alpha and SH
https://lore.kernel.org/lkml/[email protected]/#t
alpha and SH had buggy implementation of access_ok
Thanks,
Ashwin
>
> > thanks,
> >
> > greg k-h
>
> Thanks,
> Ashwin