Message ID | d29f87f3f3abb4e496866253bd170faad976f687.1589305630.git.ashwinh@vmware.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v4.19.x] make 'user_access_begin()' do 'access_ok()' | expand |
On Wed, May 13, 2020 at 07:19:21AM +0530, ashwin-h wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > 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 <torvalds@linux-foundation.org> > Signed-off-by: Ashwin H <ashwinh@vmware.com> > --- > 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" <gregkh@linuxfoundation.org> wrote: On Wed, May 13, 2020 at 07:19:21AM +0530, ashwin-h wrote: > From: Linus Torvalds <torvalds@linux-foundation.org> > > 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 <torvalds@linux-foundation.org> > Signed-off-by: Ashwin H <ashwinh@vmware.com> > --- > 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/20200527140225.GA214763@roeck-us.net 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 <gregkh@linuxfoundation.org> > Sent: Wednesday, May 27, 2020 9:02 PM > To: Ashwin H <ashwinh@vmware.com> > Cc: x86@kernel.org; dri-devel@lists.freedesktop.org; intel- > gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; stable@kernel.org; > Srivatsa Bhat <srivatsab@vmware.com>; srivatsa@csail.mit.edu; > rostedt@goodmis.org; Steven Rostedt <srostedt@vmware.com>; Linus > Torvalds <torvalds@linux-foundation.org> > 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 <gregkh@linuxfoundation.org> > Cc: x86@kernel.org; dri-devel@lists.freedesktop.org; intel- > gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; stable@kernel.org; > Srivatsa Bhat <srivatsab@vmware.com>; srivatsa@csail.mit.edu; > rostedt@goodmis.org; Steven Rostedt <srostedt@vmware.com>; Linus > Torvalds <torvalds@linux-foundation.org> > Subject: RE: [PATCH v4.19.x] make 'user_access_begin()' do 'access_ok()' > > > > > -----Original Message----- > > From: Greg KH <gregkh@linuxfoundation.org> > > Sent: Wednesday, May 27, 2020 9:02 PM > > To: Ashwin H <ashwinh@vmware.com> > > Cc: x86@kernel.org; dri-devel@lists.freedesktop.org; intel- > > gfx@lists.freedesktop.org; linux-kernel@vger.kernel.org; > > stable@kernel.org; Srivatsa Bhat <srivatsab@vmware.com>; > > srivatsa@csail.mit.edu; rostedt@goodmis.org; Steven Rostedt > > <srostedt@vmware.com>; Linus Torvalds <torvalds@linux-foundation.org> > > 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/6a4fe075-a644-1b06-305b-9e55b8c9575b@roeck-us.net/#t alpha and SH had buggy implementation of access_ok Thanks, Ashwin > > > thanks, > > > > greg k-h > > Thanks, > Ashwin
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; }