[PATCH v2] usercopy: Check valid lifetime via stack depth

[PATCH v2] usercopy: Check valid lifetime via stack depth

[Kees Cook]

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

RE: [PATCH v2] usercopy: Check valid lifetime via stack depth

[David Laight]

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)

Re: [PATCH v2] usercopy: Check valid lifetime via stack depth

[Kees Cook]

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