* [PATCH v2] usercopy: Check valid lifetime via stack depth
@ 2022-02-24 6:03 Kees Cook
2022-02-24 8:58 ` David Laight
0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2022-02-24 6:03 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Kees Cook, Josh Poimboeuf, Andrew Morton, linux-mm,
Muhammad Usama Anjum, linux-kernel, x86, linux-arm-kernel,
linuxppc-dev, linux-s390, linux-sh, linux-hardening
Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
is not available (i.e. everything except x86 with FRAME_POINTER), check
a stack object as being at least "current depth valid", in the sense
that any object within the stack region but not between start-of-stack
and current_stack_pointer should be considered unavailable (i.e. its
lifetime is from a call no longer present on the stack).
Introduce ARCH_HAS_CURRENT_STACK_POINTER to track which architectures
have actually implemented the common global register alias.
Additionally report usercopy bounds checking failures with an offset
from current_stack_pointer, which may assist with diagnosing failures.
The LKDTM USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests
(once slightly adjusted in a separate patch) will pass again with
this fixed.
Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v1: https://lore.kernel.org/all/20220216201449.2087956-1-keescook@chromium.org/
v2: adjust for only some archs having current_stack_pointer
---
arch/arm/Kconfig | 1 +
arch/arm64/Kconfig | 1 +
arch/powerpc/Kconfig | 1 +
arch/s390/Kconfig | 1 +
arch/sh/Kconfig | 1 +
arch/x86/Kconfig | 1 +
mm/usercopy.c | 41 ++++++++++++++++++++++++++++++++++++++---
7 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4c97cb40eebb..a7a09eef1852 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -5,6 +5,7 @@ config ARM
select ARCH_32BIT_OFF_T
select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE if HAVE_KRETPROBES && FRAME_POINTER && !ARM_UNWIND
select ARCH_HAS_BINFMT_FLAT
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL if MMU
select ARCH_HAS_DMA_WRITE_COMBINE if !ARM_DMA_MEM_BUFFERABLE
select ARCH_HAS_ELF_RANDOMIZE
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index f2b5a4abef21..b8ab790555c8 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -18,6 +18,7 @@ config ARM64
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
select ARCH_ENABLE_THP_MIGRATION if TRANSPARENT_HUGEPAGE
select ARCH_HAS_CACHE_LINE_SIZE
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DMA_PREP_COHERENT
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index b779603978e1..7e7387bd7d53 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -108,6 +108,7 @@ config PPC
select ARCH_ENABLE_MEMORY_HOTPLUG
select ARCH_ENABLE_MEMORY_HOTREMOVE
select ARCH_HAS_COPY_MC if PPC64
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEBUG_WX if STRICT_KERNEL_RWX
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index be9f39fd06df..4845ab549dd1 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -60,6 +60,7 @@ config S390
select ARCH_ENABLE_MEMORY_HOTPLUG if SPARSEMEM
select ARCH_ENABLE_MEMORY_HOTREMOVE
select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VM_PGTABLE
select ARCH_HAS_DEBUG_WX
select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 2474a04ceac4..1c2b53bf3093 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -7,6 +7,7 @@ config SUPERH
select ARCH_HAVE_CUSTOM_GPIO_H
select ARCH_HAVE_NMI_SAFE_CMPXCHG if (GUSA_RB || CPU_SH4A)
select ARCH_HAS_BINFMT_FLAT if !MMU
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_GIGANTIC_PAGE
select ARCH_HAS_GCOV_PROFILE_ALL
select ARCH_HAS_PTE_SPECIAL
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9f5bd41bf660..90494fba3620 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -69,6 +69,7 @@ config X86
select ARCH_ENABLE_THP_MIGRATION if X86_64 && TRANSPARENT_HUGEPAGE
select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI
select ARCH_HAS_CACHE_LINE_SIZE
+ select ARCH_HAS_CURRENT_STACK_POINTER
select ARCH_HAS_DEBUG_VIRTUAL
select ARCH_HAS_DEBUG_VM_PGTABLE if !X86_PAE
select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/mm/usercopy.c b/mm/usercopy.c
index d0d268135d96..5d28725af95f 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -22,6 +22,30 @@
#include <asm/sections.h>
#include "slab.h"
+/*
+ * Only called if obj is within stack/stackend bounds. Determine if within
+ * current stack depth.
+ */
+static inline int check_stack_object_depth(const void *obj,
+ unsigned long len)
+{
+#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
+#ifndef CONFIG_STACK_GROWSUP
+ const void * const high = stackend;
+ const void * const low = (void *)current_stack_pointer;
+#else
+ const void * const high = (void *)current_stack_pointer;
+ const void * const low = stack;
+#endif
+
+ /* Reject: object not within current stack depth. */
+ if (obj < low || high < obj + len)
+ return BAD_STACK;
+
+#endif
+ return GOOD_STACK;
+}
+
/*
* Checks if a given pointer and length is contained by the current
* stack frame (if possible).
@@ -29,7 +53,7 @@
* Returns:
* NOT_STACK: not at all on the stack
* GOOD_FRAME: fully within a valid stack frame
- * GOOD_STACK: fully on the stack (when can't do frame-checking)
+ * GOOD_STACK: within the current stack (when can't frame-check exactly)
* BAD_STACK: error condition (invalid stack position or bad stack frame)
*/
static noinline int check_stack_object(const void *obj, unsigned long len)
@@ -55,7 +79,8 @@ static noinline int check_stack_object(const void *obj, unsigned long len)
if (ret)
return ret;
- return GOOD_STACK;
+ /* Finally, check stack depth if possible. */
+ return check_stack_object_depth(obj, len);
}
/*
@@ -280,7 +305,17 @@ void __check_object_size(const void *ptr, unsigned long n, bool to_user)
*/
return;
default:
- usercopy_abort("process stack", NULL, to_user, 0, n);
+ usercopy_abort("process stack", NULL, to_user,
+#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
+# ifndef CONFIG_STACK_GROWSUP
+ (void *)current_stack_pointer - ptr,
+# else
+ ptr - (void *)current_stack_pointer,
+# endif
+#else
+ 0,
+#endif
+ n);
}
/* Check for bad heap object. */
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* RE: [PATCH v2] usercopy: Check valid lifetime via stack depth
2022-02-24 6:03 [PATCH v2] usercopy: Check valid lifetime via stack depth Kees Cook
@ 2022-02-24 8:58 ` David Laight
2022-02-25 4:47 ` Kees Cook
0 siblings, 1 reply; 3+ messages in thread
From: David Laight @ 2022-02-24 8:58 UTC (permalink / raw)
To: 'Kees Cook', Matthew Wilcox
Cc: Josh Poimboeuf, Andrew Morton, linux-mm, Muhammad Usama Anjum,
linux-kernel, x86, linux-arm-kernel, linuxppc-dev, linux-s390,
linux-sh, linux-hardening
From: Kees Cook
> Sent: 24 February 2022 06:04
>
> Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
> is not available (i.e. everything except x86 with FRAME_POINTER), check
> a stack object as being at least "current depth valid", in the sense
> that any object within the stack region but not between start-of-stack
> and current_stack_pointer should be considered unavailable (i.e. its
> lifetime is from a call no longer present on the stack).
>
...
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index d0d268135d96..5d28725af95f 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -22,6 +22,30 @@
> #include <asm/sections.h>
> #include "slab.h"
>
> +/*
> + * Only called if obj is within stack/stackend bounds. Determine if within
> + * current stack depth.
> + */
> +static inline int check_stack_object_depth(const void *obj,
> + unsigned long len)
> +{
> +#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
> +#ifndef CONFIG_STACK_GROWSUP
Pointless negation
> + const void * const high = stackend;
> + const void * const low = (void *)current_stack_pointer;
> +#else
> + const void * const high = (void *)current_stack_pointer;
> + const void * const low = stack;
> +#endif
> +
> + /* Reject: object not within current stack depth. */
> + if (obj < low || high < obj + len)
> + return BAD_STACK;
> +
> +#endif
> + return GOOD_STACK;
> +}
If the comment at the top of the function is correct then
only a single test for the correct end of the buffer against
the current stack pointer is needed.
Something like:
#ifdef CONFIG_STACK_GROWSUP
if ((void *)current_stack_pointer < obj + len)
return BAD_STACK;
#else
if (obj < (void *)current_stack_pointer)
return BAD_STACK;
#endif
return GOOD_STACK;
Although it may depend on exactly where the stack pointer
points to - especially for GROWSUP.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] usercopy: Check valid lifetime via stack depth
2022-02-24 8:58 ` David Laight
@ 2022-02-25 4:47 ` Kees Cook
0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2022-02-25 4:47 UTC (permalink / raw)
To: David Laight
Cc: Matthew Wilcox, Josh Poimboeuf, Andrew Morton, linux-mm,
Muhammad Usama Anjum, linux-kernel, x86, linux-arm-kernel,
linuxppc-dev, linux-s390, linux-sh, linux-hardening
On Thu, Feb 24, 2022 at 08:58:20AM +0000, David Laight wrote:
> From: Kees Cook
> > Sent: 24 February 2022 06:04
> >
> > Under CONFIG_HARDENED_USERCOPY=y, when exact stack frame boundary checking
> > is not available (i.e. everything except x86 with FRAME_POINTER), check
> > a stack object as being at least "current depth valid", in the sense
> > that any object within the stack region but not between start-of-stack
> > and current_stack_pointer should be considered unavailable (i.e. its
> > lifetime is from a call no longer present on the stack).
> >
> ...
> > diff --git a/mm/usercopy.c b/mm/usercopy.c
> > index d0d268135d96..5d28725af95f 100644
> > --- a/mm/usercopy.c
> > +++ b/mm/usercopy.c
> > @@ -22,6 +22,30 @@
> > #include <asm/sections.h>
> > #include "slab.h"
> >
> > +/*
> > + * Only called if obj is within stack/stackend bounds. Determine if within
> > + * current stack depth.
> > + */
> > +static inline int check_stack_object_depth(const void *obj,
> > + unsigned long len)
> > +{
> > +#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER
> > +#ifndef CONFIG_STACK_GROWSUP
>
> Pointless negation
>
> > + const void * const high = stackend;
> > + const void * const low = (void *)current_stack_pointer;
> > +#else
> > + const void * const high = (void *)current_stack_pointer;
> > + const void * const low = stack;
> > +#endif
> > +
> > + /* Reject: object not within current stack depth. */
> > + if (obj < low || high < obj + len)
> > + return BAD_STACK;
> > +
> > +#endif
> > + return GOOD_STACK;
> > +}
>
> If the comment at the top of the function is correct then
> only a single test for the correct end of the buffer against
> the current stack pointer is needed.
> Something like:
> #ifdef CONFIG_STACK_GROWSUP
> if ((void *)current_stack_pointer < obj + len)
> return BAD_STACK;
> #else
> if (obj < (void *)current_stack_pointer)
> return BAD_STACK;
> #endif
> return GOOD_STACK;
Oh, yeah, excellent point. I suspect the compiler would probably
optimize it all away, but yes, this is, in fact, easier to read, and
short enough I should probably just not bother with a separate function.
Thanks!
-Kees
>
> Although it may depend on exactly where the stack pointer
> points to - especially for GROWSUP.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
--
Kees Cook
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-25 4:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-24 6:03 [PATCH v2] usercopy: Check valid lifetime via stack depth Kees Cook
2022-02-24 8:58 ` David Laight
2022-02-25 4:47 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).