| Message ID | 20220225173345.3358109-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 2792d84e6da5e0fd7d3b22fd70bc69b7ee263609 |
| Headers | show |
| Series | [v3] usercopy: Check valid lifetime via stack depth | expand |
On Fri, 25 Feb 2022 09:33:45 -0800 Kees Cook <keescook@chromium.org> wrote: > 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. Again, what does this actually do? > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> A link to that report would shed some light. But actually describing the user-visible impact right there in the changelog is preferable. It sounds like a selftest is newly failing, which makes it a userspace-visible regression, perhaps? If so, do we have a Fixes: and is a cc:stable warranted?
On Fri, Feb 25, 2022 at 04:01:57PM -0800, Andrew Morton wrote: > On Fri, 25 Feb 2022 09:33:45 -0800 Kees Cook <keescook@chromium.org> wrote: > > > 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. > > Again, what does this actually do? One of the things that CONFIG_HARDENED_USERCOPY checks is whether an object is overlapping the stack at all. If it is, it performs a number of inexpensive bounds checks. One of the finer-grained checks is whether an object cross stack frame within the stack region. Doing this with CONFIG_FRAME_POINTER was cheap/easy. Doing it with ORC is too heavy, and was left out (a while ago), leaving the courser whole-stack check. The LKDTM tests try to exercise the cross-frame cases to validate the defense. They have been failing every since (which was expected). More below... > > > Reported-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > A link to that report would shed some light. But actually describing > the user-visible impact right there in the changelog is preferable. Yes, good point. The bug[1] involves multiple LKDTM tests and their failure modes, so it wasn't a very clean pointer. But I will include it. [1] https://github.com/kernelci/kernelci-project/issues/84 > It sounds like a selftest is newly failing, which makes it a > userspace-visible regression, perhaps? No, it's been failing since ORC was introduced, but the regression in coverage (due to switching from FRAME_POINTER to ORC unwinder) was minimal. While discussing this with Muhammad, I realized we did, actually, have something that could be tested that was less than "the entire stack area" and "each specific frame", and that was current stack depth, so we gain back a little coverage. > If so, do we have a Fixes: and is a cc:stable warranted? I don't think it's warranted; it is technically a new feature. -Kees
On Fri, 25 Feb 2022 17:35:49 -0800 Kees Cook <keescook@chromium.org> wrote: > On Fri, Feb 25, 2022 at 04:01:57PM -0800, Andrew Morton wrote: > > On Fri, 25 Feb 2022 09:33:45 -0800 Kees Cook <keescook@chromium.org> wrote: > > > > > 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. > > > > Again, what does this actually do? > > [answers] > OK, thanks. I think a new changelog is warranted? What's your preferred path for upstreaming this change?
On Fri, Feb 25, 2022 at 05:46:57PM -0800, Andrew Morton wrote: > On Fri, 25 Feb 2022 17:35:49 -0800 Kees Cook <keescook@chromium.org> wrote: > > > On Fri, Feb 25, 2022 at 04:01:57PM -0800, Andrew Morton wrote: > > > On Fri, 25 Feb 2022 09:33:45 -0800 Kees Cook <keescook@chromium.org> wrote: > > > > > > > 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. > > > > > > Again, what does this actually do? > > > > [answers] > > > > OK, thanks. I think a new changelog is warranted? Yup, I've cut/pasted most of that into the new changelog: usercopy: Check valid lifetime via stack depth One of the things that CONFIG_HARDENED_USERCOPY sanity-checks is whether an object that is about to be copied to/from userspace is overlapping the stack at all. If it is, it performs a number of inexpensive bounds checks. One of the finer-grained checks is whether an object crosses stack frames within the stack region. Doing this on x86 with CONFIG_FRAME_POINTER was cheap/easy. Doing it with ORC was deemed too heavy, and was left out (a while ago), leaving the courser whole-stack check. The LKDTM tests USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM try to exercise these cross-frame cases to validate the defense is working. They have been failing ever since ORC was added (which was expected). While Muhammad was investigating various LKDTM failures[1], he asked me for additional details on them, and I realized that when exact stack frame boundary checking is not available (i.e. everything except x86 with FRAME_POINTER), it could check if a stack object is 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) pass again with this fixed. [1] https://github.com/kernelci/kernelci-project/issues/84 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/lkml/20220216201449.2087956-1-keescook@chromium.org v2: https://lore.kernel.org/lkml/20220224060342.1855457-1-keescook@chromium.org v3: https://lore.kernel.org/lkml/20220225173345.3358109-1-keescook@chromium.org v4: - improve commit log (akpm) > What's your preferred path for upstreaming this change? I figured I would take it via my for-next/hardening tree; I have 2 arch changes ready to go (Acked by maintainers) there too (to add current_stack_pointer). Thanks for the review!
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/Kconfig b/mm/Kconfig index 3326ee3903f3..c349599601f8 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -744,6 +744,15 @@ config IDLE_PAGE_TRACKING config ARCH_HAS_CACHE_LINE_SIZE bool +config ARCH_HAS_CURRENT_STACK_POINTER + bool + help + In support of HARDENED_USERCOPY performing stack variable lifetime + checking, an architecture-agnostic way to find the stack pointer + is needed. Once an architecture defines an unsigned long global + register alias named "current_stack_pointer", this config can be + selected. + config ARCH_HAS_PTE_DEVMAP bool diff --git a/mm/usercopy.c b/mm/usercopy.c index d0d268135d96..5d34c40c16c2 100644 --- a/mm/usercopy.c +++ b/mm/usercopy.c @@ -29,7 +29,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,6 +55,17 @@ static noinline int check_stack_object(const void *obj, unsigned long len) if (ret) return ret; + /* Finally, check stack depth if possible. */ +#ifdef CONFIG_ARCH_HAS_CURRENT_STACK_POINTER + if (IS_ENABLED(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; } @@ -280,7 +291,15 @@ 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 + IS_ENABLED(CONFIG_STACK_GROWSUP) ? + ptr - (void *)current_stack_pointer : + (void *)current_stack_pointer - ptr, +#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/lkml/20220216201449.2087956-1-keescook@chromium.org v2: https://lore.kernel.org/lkml/20220224060342.1855457-1-keescook@chromium.org v3: - simplify bounds check (David) - add missed Kconfig declaration --- 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/Kconfig | 9 +++++++++ mm/usercopy.c | 23 +++++++++++++++++++++-- 8 files changed, 36 insertions(+), 2 deletions(-)