diff mbox series

[v3] usercopy: Check valid lifetime via stack depth

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

Commit Message

Kees Cook Feb. 25, 2022, 5:33 p.m. UTC
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(-)

Comments

Andrew Morton Feb. 26, 2022, 12:01 a.m. UTC | #1
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?
Kees Cook Feb. 26, 2022, 1:35 a.m. UTC | #2
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
Andrew Morton Feb. 26, 2022, 1:46 a.m. UTC | #3
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?
Kees Cook Feb. 26, 2022, 2:22 a.m. UTC | #4
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 mbox series

Patch

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. */