| Message ID | 20220224060342.1855457-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usercopy: Check valid lifetime via stack depth | expand |
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)
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) >
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. */
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(-)