Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444627 > unrolled thread

[PATCH v3 00/11] mm: Hardened usercopy

Started byKees Cook <keescook@chromium.org>
First post2016-07-15 23:50 +0200
Last post2016-07-20 19:50 +0200
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/11] mm: Hardened usercopy Kees Cook <keescook@chromium.org> - 2016-07-15 23:50 +0200
    [PATCH v3 07/11] powerpc/uaccess: Enable hardened usercopy Kees Cook <keescook@chromium.org> - 2016-07-15 23:50 +0200
    [PATCH v3 06/11] ia64/uaccess: Enable hardened usercopy Kees Cook <keescook@chromium.org> - 2016-07-15 23:50 +0200
    Re: [PATCH v3 00/11] mm: Hardened usercopy Balbir Singh <bsingharora@gmail.com> - 2016-07-18 10:30 +0200
    RE: [PATCH v3 00/11] mm: Hardened usercopy David Laight <David.Laight@ACULAB.COM> - 2016-07-20 12:00 +0200
      Re: [PATCH v3 00/11] mm: Hardened usercopy Kees Cook <keescook@chromium.org> - 2016-07-20 17:40 +0200
        RE: [PATCH v3 00/11] mm: Hardened usercopy David Laight <David.Laight@ACULAB.COM> - 2016-07-20 18:10 +0200
          Re: [PATCH v3 00/11] mm: Hardened usercopy Rik van Riel <riel@redhat.com> - 2016-07-20 18:30 +0200
          Re: [PATCH v3 00/11] mm: Hardened usercopy Kees Cook <keescook@chromium.org> - 2016-07-20 19:50 +0200

#1444627 — [PATCH v3 00/11] mm: Hardened usercopy

FromKees Cook <keescook@chromium.org>
Date2016-07-15 23:50 +0200
Subject[PATCH v3 00/11] mm: Hardened usercopy
Message-ID<rVj9E-3EM-7@gated-at.bofh.it>
Hi,

[I'm going to carry this series in my kspp -next tree now, though I'd
really love to have some explicit Acked-bys or Reviewed-bys. If you've
looked through it or tested it, please consider it. :) (I added Valdis
and mpe's Tested-bys where they seemed correct, thank you!)]

This is a start of the mainline port of PAX_USERCOPY[1]. After I started
writing tests (now in lkdtm in -next) for Casey's earlier port[2], I kept
tweaking things further and further until I ended up with a whole new
patch series. To that end, I took Rik and other people's feedback along
with other changes and clean-ups.

Based on my understanding, PAX_USERCOPY was designed to catch a
few classes of flaws (mainly bad bounds checking) around the use of
copy_to_user()/copy_from_user(). These changes don't touch get_user() and
put_user(), since these operate on constant sized lengths, and tend to be
much less vulnerable. There are effectively three distinct protections in
the whole series, each of which I've given a separate CONFIG, though this
patch set is only the first of the three intended protections. (Generally
speaking, PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY
(this) and CONFIG_HARDENED_USERCOPY_WHITELIST (future), and
PAX_USERCOPY_SLABS covers CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC
(future).)

This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
being copied to/from userspace meet certain criteria:
- if address is a heap object, the size must not exceed the object's
  allocated size. (This will catch all kinds of heap overflow flaws.)
- if address range is in the current process stack, it must be within the
  current stack frame (if such checking is possible) or at least entirely
  within the current process's stack. (This could catch large lengths that
  would have extended beyond the current process stack, or overflows if
  their length extends back into the original stack.)
- if the address range is part of kernel data, rodata, or bss, allow it.
- if address range is page-allocated, that it doesn't span multiple
  allocations.
- if address is within the kernel text, reject it.
- everything else is accepted

The patches in the series are:
- Support for arch-specific stack frame checking (which will likely be
  replaced in the future by Josh's more comprehensive unwinder):
        1- mm: Implement stack frame object validation
- The core copy_to/from_user() checks, without the slab object checks:
        2- mm: Hardened usercopy
- Per-arch enablement of the protection:
        3- x86/uaccess: Enable hardened usercopy
        4- ARM: uaccess: Enable hardened usercopy
        5- arm64/uaccess: Enable hardened usercopy
        6- ia64/uaccess: Enable hardened usercopy
        7- powerpc/uaccess: Enable hardened usercopy
        8- sparc/uaccess: Enable hardened usercopy
        9- s390/uaccess: Enable hardened usercopy
- The heap allocator implementation of object size checking:
       10- mm: SLAB hardened usercopy support
       11- mm: SLUB hardened usercopy support

Some notes:

- This is expected to apply on top of -next which contains fixes for the
  position of _etext on both arm and arm64, though it has minor conflicts
  with KASAN that are trivial to fix up. Living in -next are also tests
  for this protection in lkdtm, prefixed with USERCOPY_.

- I couldn't detect a measurable performance change with these features
  enabled. Kernel build times were unchanged, hackbench was unchanged,
  etc. I think we could flip this to "on by default" at some point, but
  for now, I'm leaving it off until I can get some more definitive
  measurements. I would love if someone with greater familiarity with
  perf could give this a spin and report results.

- The SLOB support extracted from grsecurity seems entirely broken. I
  have no idea what's going on there, I spent my time testing SLAB and
  SLUB. Having someone else look at SLOB would be nice, but this series
  doesn't depend on it.

Additional features that would be nice, but aren't blocking this series:

- Needs more architecture support for stack frame checking (only x86 now,
  but it seems Josh will have a good solution for this soon).


Thanks!

-Kees

[1] https://grsecurity.net/download.php "grsecurity - test kernel patch"
[2] http://www.openwall.com/lists/kernel-hardening/2016/05/19/5

v3:
- switch to using BUG for better Oops integration
- when checking page allocations, check each for Reserved
- use enums for the stack check return for readability

v2:
- added s390 support
- handle slub red zone
- disallow writes to rodata area
- stack frame walker now CONFIG-controlled arch-specific helper

[toc] | [next] | [standalone]

#1444628 — [PATCH v3 07/11] powerpc/uaccess: Enable hardened usercopy

FromKees Cook <keescook@chromium.org>
Date2016-07-15 23:50 +0200
Subject[PATCH v3 07/11] powerpc/uaccess: Enable hardened usercopy
Message-ID<rVj9F-3EM-39@gated-at.bofh.it>
In reply to#1444627
Enables CONFIG_HARDENED_USERCOPY checks on powerpc.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/Kconfig               |  1 +
 arch/powerpc/include/asm/uaccess.h | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 01f7464d9fea..b7a18b2604be 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -164,6 +164,7 @@ config PPC
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
 	select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_ARCH_HARDENED_USERCOPY
 
 config GENERIC_CSUM
 	def_bool CPU_LITTLE_ENDIAN
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index b7c20f0b8fbe..c1dc6c14deb8 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -310,10 +310,15 @@ static inline unsigned long copy_from_user(void *to,
 {
 	unsigned long over;
 
-	if (access_ok(VERIFY_READ, from, n))
+	if (access_ok(VERIFY_READ, from, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(to, n, false);
 		return __copy_tofrom_user((__force void __user *)to, from, n);
+	}
 	if ((unsigned long)from < TASK_SIZE) {
 		over = (unsigned long)from + n - TASK_SIZE;
+		if (!__builtin_constant_p(n - over))
+			check_object_size(to, n - over, false);
 		return __copy_tofrom_user((__force void __user *)to, from,
 				n - over) + over;
 	}
@@ -325,10 +330,15 @@ static inline unsigned long copy_to_user(void __user *to,
 {
 	unsigned long over;
 
-	if (access_ok(VERIFY_WRITE, to, n))
+	if (access_ok(VERIFY_WRITE, to, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(from, n, true);
 		return __copy_tofrom_user(to, (__force void __user *)from, n);
+	}
 	if ((unsigned long)to < TASK_SIZE) {
 		over = (unsigned long)to + n - TASK_SIZE;
+		if (!__builtin_constant_p(n))
+			check_object_size(from, n - over, true);
 		return __copy_tofrom_user(to, (__force void __user *)from,
 				n - over) + over;
 	}
@@ -372,6 +382,10 @@ static inline unsigned long __copy_from_user_inatomic(void *to,
 		if (ret == 0)
 			return 0;
 	}
+
+	if (!__builtin_constant_p(n))
+		check_object_size(to, n, false);
+
 	return __copy_tofrom_user((__force void __user *)to, from, n);
 }
 
@@ -398,6 +412,9 @@ static inline unsigned long __copy_to_user_inatomic(void __user *to,
 		if (ret == 0)
 			return 0;
 	}
+	if (!__builtin_constant_p(n))
+		check_object_size(from, n, true);
+
 	return __copy_tofrom_user(to, (__force const void __user *)from, n);
 }
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1444629 — [PATCH v3 06/11] ia64/uaccess: Enable hardened usercopy

FromKees Cook <keescook@chromium.org>
Date2016-07-15 23:50 +0200
Subject[PATCH v3 06/11] ia64/uaccess: Enable hardened usercopy
Message-ID<rVj9F-3EM-41@gated-at.bofh.it>
In reply to#1444627
Enables CONFIG_HARDENED_USERCOPY checks on ia64.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/ia64/Kconfig               |  1 +
 arch/ia64/include/asm/uaccess.h | 18 +++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index f80758cb7157..32a87ef516a0 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -53,6 +53,7 @@ config IA64
 	select MODULES_USE_ELF_RELA
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_HARDENED_USERCOPY
 	default y
 	help
 	  The Itanium Processor Family is Intel's 64-bit successor to
diff --git a/arch/ia64/include/asm/uaccess.h b/arch/ia64/include/asm/uaccess.h
index 2189d5ddc1ee..465c70982f40 100644
--- a/arch/ia64/include/asm/uaccess.h
+++ b/arch/ia64/include/asm/uaccess.h
@@ -241,12 +241,18 @@ extern unsigned long __must_check __copy_user (void __user *to, const void __use
 static inline unsigned long
 __copy_to_user (void __user *to, const void *from, unsigned long count)
 {
+	if (!__builtin_constant_p(count))
+		check_object_size(from, count, true);
+
 	return __copy_user(to, (__force void __user *) from, count);
 }
 
 static inline unsigned long
 __copy_from_user (void *to, const void __user *from, unsigned long count)
 {
+	if (!__builtin_constant_p(count))
+		check_object_size(to, count, false);
+
 	return __copy_user((__force void __user *) to, from, count);
 }
 
@@ -258,8 +264,11 @@ __copy_from_user (void *to, const void __user *from, unsigned long count)
 	const void *__cu_from = (from);							\
 	long __cu_len = (n);								\
 											\
-	if (__access_ok(__cu_to, __cu_len, get_fs()))					\
-		__cu_len = __copy_user(__cu_to, (__force void __user *) __cu_from, __cu_len);	\
+	if (__access_ok(__cu_to, __cu_len, get_fs())) {					\
+		if (!__builtin_constant_p(n))						\
+			check_object_size(__cu_from, __cu_len, true);			\
+		__cu_len = __copy_user(__cu_to, (__force void __user *)  __cu_from, __cu_len);	\
+	}										\
 	__cu_len;									\
 })
 
@@ -270,8 +279,11 @@ __copy_from_user (void *to, const void __user *from, unsigned long count)
 	long __cu_len = (n);								\
 											\
 	__chk_user_ptr(__cu_from);							\
-	if (__access_ok(__cu_from, __cu_len, get_fs()))					\
+	if (__access_ok(__cu_from, __cu_len, get_fs())) {				\
+		if (!__builtin_constant_p(n))						\
+			check_object_size(__cu_to, __cu_len, false);			\
 		__cu_len = __copy_user((__force void __user *) __cu_to, __cu_from, __cu_len);	\
+	}										\
 	__cu_len;									\
 })
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1445293

FromBalbir Singh <bsingharora@gmail.com>
Date2016-07-18 10:30 +0200
Message-ID<rWc66-3Ku-1@gated-at.bofh.it>
In reply to#1444627
On Fri, 2016-07-15 at 14:44 -0700, Kees Cook wrote:
> Hi,
> 
> [I'm going to carry this series in my kspp -next tree now, though I'd
> really love to have some explicit Acked-bys or Reviewed-bys. If you've
> looked through it or tested it, please consider it. :) (I added Valdis
> and mpe's Tested-bys where they seemed correct, thank you!)]
> 
> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I kept
> tweaking things further and further until I ended up with a whole new
> patch series. To that end, I took Rik and other people's feedback along
> with other changes and clean-ups.
> 
> Based on my understanding, PAX_USERCOPY was designed to catch a
> few classes of flaws (mainly bad bounds checking) around the use of
> copy_to_user()/copy_from_user(). These changes don't touch get_user() and
> put_user(), since these operate on constant sized lengths, and tend to be
> much less vulnerable. There are effectively three distinct protections in
> the whole series, each of which I've given a separate CONFIG, though this
> patch set is only the first of the three intended protections. (Generally
> speaking, PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY
> (this) and CONFIG_HARDENED_USERCOPY_WHITELIST (future), and
> PAX_USERCOPY_SLABS covers CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC
> (future).)
> 
> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
> being copied to/from userspace meet certain criteria:
> - if address is a heap object, the size must not exceed the object's
>   allocated size. (This will catch all kinds of heap overflow flaws.)
> - if address range is in the current process stack, it must be within the
>   current stack frame (if such checking is possible) or at least entirely
>   within the current process's stack. (This could catch large lengths that
>   would have extended beyond the current process stack, or overflows if
>   their length extends back into the original stack.)
> - if the address range is part of kernel data, rodata, or bss, allow it.
> - if address range is page-allocated, that it doesn't span multiple
>   allocations.
> - if address is within the kernel text, reject it.
> - everything else is accepted
> 
> The patches in the series are:
> - Support for arch-specific stack frame checking (which will likely be
>   replaced in the future by Josh's more comprehensive unwinder):
>         1- mm: Implement stack frame object validation
> - The core copy_to/from_user() checks, without the slab object checks:
>         2- mm: Hardened usercopy
> - Per-arch enablement of the protection:
>         3- x86/uaccess: Enable hardened usercopy
>         4- ARM: uaccess: Enable hardened usercopy
>         5- arm64/uaccess: Enable hardened usercopy
>         6- ia64/uaccess: Enable hardened usercopy
>         7- powerpc/uaccess: Enable hardened usercopy
>         8- sparc/uaccess: Enable hardened usercopy
>         9- s390/uaccess: Enable hardened usercopy
> - The heap allocator implementation of object size checking:
>        10- mm: SLAB hardened usercopy support
>        11- mm: SLUB hardened usercopy support
> 
> Some notes:
> 
> - This is expected to apply on top of -next which contains fixes for the
>   position of _etext on both arm and arm64, though it has minor conflicts
>   with KASAN that are trivial to fix up. Living in -next are also tests
>   for this protection in lkdtm, prefixed with USERCOPY_.
> 
> - I couldn't detect a measurable performance change with these features
>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>   etc. I think we could flip this to "on by default" at some point, but
>   for now, I'm leaving it off until I can get some more definitive
>   measurements. I would love if someone with greater familiarity with
>   perf could give this a spin and report results.
> 
> - The SLOB support extracted from grsecurity seems entirely broken. I
>   have no idea what's going on there, I spent my time testing SLAB and
>   SLUB. Having someone else look at SLOB would be nice, but this series
>   doesn't depend on it.
> 
> Additional features that would be nice, but aren't blocking this series:
> 
> - Needs more architecture support for stack frame checking (only x86 now,
>   but it seems Josh will have a good solution for this soon).
> 
> 
> Thanks!
> 
> -Kees
> 
> [1] https://grsecurity.net/download.php "grsecurity - test kernel patch"
> [2] http://www.openwall.com/lists/kernel-hardening/2016/05/19/5
> 
> v3:
> - switch to using BUG for better Oops integration
> - when checking page allocations, check each for Reserved
> - use enums for the stack check return for readability
>

Thanks looks good so far! I'll try and test it and report back

Balbir 

[toc] | [prev] | [next] | [standalone]

#1447136

FromDavid Laight <David.Laight@ACULAB.COM>
Date2016-07-20 12:00 +0200
Message-ID<rWWsn-8a2-11@gated-at.bofh.it>
In reply to#1444627
From: Kees Cook
> Sent: 15 July 2016 22:44
> This is a start of the mainline port of PAX_USERCOPY[1]. 
...
> - if address range is in the current process stack, it must be within the
>   current stack frame (if such checking is possible) or at least entirely
>   within the current process's stack.
...

That description doesn't seem quite right to me.
I presume the check is:
  Within the current process's stack and not crossing the ends of the
  current stack frame.

The 'current' stack frame is likely to be that of copy_to/from_user().
Even if you use the stack of the caller, any problematic buffers
are likely to have been passed in from a calling function.
So unless you are going to walk the stack (good luck on that)
I'm not sure checking the stack frames is worth it.

I'd also guess that a lot of copies are from the middle of structures
so cannot fail the tests you are adding.

	David

[toc] | [prev] | [next] | [standalone]

#1447304

FromKees Cook <keescook@chromium.org>
Date2016-07-20 17:40 +0200
Message-ID<rX1Lk-3cG-17@gated-at.bofh.it>
In reply to#1447136
On Wed, Jul 20, 2016 at 2:52 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Kees Cook
>> Sent: 15 July 2016 22:44
>> This is a start of the mainline port of PAX_USERCOPY[1].
> ...
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack.
> ...
>
> That description doesn't seem quite right to me.
> I presume the check is:
>   Within the current process's stack and not crossing the ends of the
>   current stack frame.

Actually, it's a bad description all around. :) The check is that the
range is within a valid stack frame (current or any prior caller's
frame). i.e. it does not cross a frame or touch the saved frame
pointer nor instruction pointer.

> The 'current' stack frame is likely to be that of copy_to/from_user().
> Even if you use the stack of the caller, any problematic buffers
> are likely to have been passed in from a calling function.
> So unless you are going to walk the stack (good luck on that)
> I'm not sure checking the stack frames is worth it.

Yup: that's exactly what it's doing: walking up the stack. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1447333

FromDavid Laight <David.Laight@ACULAB.COM>
Date2016-07-20 18:10 +0200
Message-ID<rX2em-3Bs-25@gated-at.bofh.it>
In reply to#1447304
From: Kees Cook
> Sent: 20 July 2016 16:32
...
> Yup: that's exactly what it's doing: walking up the stack. :)

Remind me to make sure all our customers run kernels with it disabled.

	David

[toc] | [prev] | [next] | [standalone]

#1447347

FromRik van Riel <riel@redhat.com>
Date2016-07-20 18:30 +0200
Message-ID<rX2xI-3I2-13@gated-at.bofh.it>
In reply to#1447333

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-07-20 at 16:02 +0000, David Laight wrote:
> From: Kees Cook
> > Sent: 20 July 2016 16:32
> ...
> > Yup: that's exactly what it's doing: walking up the stack. :)
> 
> Remind me to make sure all our customers run kernels with it
> disabled.

You want a single copy_from_user to write to data in
multiple stack frames?

-- 

All Rights Reversed.

[toc] | [prev] | [next] | [standalone]

#1447380

FromKees Cook <keescook@chromium.org>
Date2016-07-20 19:50 +0200
Message-ID<rX3N7-4tM-1@gated-at.bofh.it>
In reply to#1447333
On Wed, Jul 20, 2016 at 9:02 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Kees Cook
>> Sent: 20 July 2016 16:32
> ...
>> Yup: that's exactly what it's doing: walking up the stack. :)
>
> Remind me to make sure all our customers run kernels with it disabled.

What's your concern with stack walking?

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web