Introduce the STACKLEAK feature and a test for it

This commit introduces the architecture-specific code filling the used part of the kernel stack with a poison value before returning to the userspace. Full STACKLEAK feature also contains the gcc plugin which comes in a separate commit.

 arch/x86/entry/entry_32.S        |  7 ++++++
 arch/x86/entry/entry_64.S        |  3 +++
 arch/x86/entry/entry_64_compat.S |  5 ++++
 arch/x86/entry/erase.c           | 54 ++++++++++++++++++++++++++++++++++++++++

Quoted 4 lines · Alexander Popov

This commit introduces the architecture-specific code filling the used part of the kernel stack with a poison value before returning to the userspace. Full STACKLEAK feature also contains the gcc plugin which comes in a separate commit.

Thanks for sending this again! And thanks for the updated reasoning for why this remains a valuable addition:

https://lkml.kernel.org/r/[email protected]

I, too, remain convinced this is a good protection to have, even as we slowly remove VLAs and try to improve the compiler's initialization of stack variables.

Dave, Ingo, Linus: how does this look? With the assembly rewritten into C, the entry changes are very small:

Quoted 4 lines

  arch/x86/entry/entry_32.S        |  7 ++++++
  arch/x86/entry/entry_64.S        |  3 +++
  arch/x86/entry/entry_64_compat.S |  5 ++++
  arch/x86/entry/erase.c           | 54 ++++++++++++++++++++++++++++++++++++++++

I'd really like to get people's Ack/Review. :)

Laura, can this C version work for arm64 as well?

Thanks,

-Kees

Dave, Ingo, Linus: how does this look? With the assembly rewritten into C, the entry changes are very small:

Quoted 2 lines · Kees Cook

Dave, Ingo, Linus: how does this look? With the assembly rewritten into C, the entry changes are very small:

The assembly looks very nice to me now. It is as minimally invasive as it can get. Definitely no objections from me on that part.

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Quoted 9 lines · Dave Hansen

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Thanks Dave!

Given this improvement and your review, I'm going to start carrying this for linux-next. Linus, if you're still opposed to this even after the changes here in v11, please let us know. I'd rather hash things out now instead of during a NAK in the 4.18 merge window. :)

Quoted 14 lines · Kees Cook

Quoted 9 lines · Dave Hansen

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Thanks Dave!

Quoted 4 lines

Given this improvement and your review, I'm going to start carrying this for linux-next. Linus, if you're still opposed to this even after the changes here in v11, please let us know. I'd rather hash things out now instead of during a NAK in the 4.18 merge window. :)

Kees, can we please route that x86/entry stuff through tip to avoid conflicts as there are other changes in that area on the horizon.

Quoted 21 lines · Thomas Gleixner

Quoted 18 lines · Kees Cook

Quoted 9 lines · Dave Hansen

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Thanks Dave!

Given this improvement and your review, I'm going to start carrying this for linux-next. Linus, if you're still opposed to this even after the changes here in v11, please let us know. I'd rather hash things out now instead of during a NAK in the 4.18 merge window. :)

Quoted 2 lines

Kees, can we please route that x86/entry stuff through tip to avoid conflicts as there are other changes in that area on the horizon.

Sure, let me figure out how best to split up the patches, since it touch x86/entry, gcc-plugins, and lkdtm. Thanks!

Quoted 22 lines · Kees Cook

Quoted 21 lines · Thomas Gleixner

Quoted 18 lines · Kees Cook

Quoted 9 lines · Dave Hansen

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Thanks Dave!

Given this improvement and your review, I'm going to start carrying this for linux-next. Linus, if you're still opposed to this even after the changes here in v11, please let us know. I'd rather hash things out now instead of during a NAK in the 4.18 merge window. :)

Quoted 5 lines

Quoted 2 lines

Kees, can we please route that x86/entry stuff through tip to avoid conflicts as there are other changes in that area on the horizon.

Sure, let me figure out how best to split up the patches, since it touch x86/entry, gcc-plugins, and lkdtm. Thanks!

Are they independent or do they carry dependencies?

Quoted 29 lines · Thomas Gleixner

Quoted 27 lines · Kees Cook

Quoted 16 lines · Thomas Gleixner

Quoted 14 lines · Kees Cook

Quoted 9 lines · Dave Hansen

Hi Alexander,

You can add:

Reviewed-by Dave Hansen <[email protected]>

for this patch if you like. I haven't taken a super close look at the rest, but this is certainly minimally invasive from my point of view for the entry code. Thanks, again for reworking it.

Thanks Dave!

Quoted 7 lines

Quoted 4 lines

Given this improvement and your review, I'm going to start carrying this for linux-next. Linus, if you're still opposed to this even after the changes here in v11, please let us know. I'd rather hash things out now instead of during a NAK in the 4.18 merge window. :)

Kees, can we please route that x86/entry stuff through tip to avoid conflicts as there are other changes in that area on the horizon.

Sure, let me figure out how best to split up the patches, since it touch x86/entry, gcc-plugins, and lkdtm. Thanks!

Quoted 1 line

Are they independent or do they carry dependencies?

They carry dependencies, as it interacts with the gcc plugin (and lkdtm). As I don't have other plugin changes for 4.18 queued, you could take the whole series for x86/entry if you want? Otherwise I can try to split out the x86 change so it's more self-contained.

On Wed, May 2, 2018 at 5:51 AM, Kees Cook <[email protected]> wrote:
I think it'd be better for this to go via my tree with your Ack (where I can carry the plugin, lkdtm, x86, and arm64 changes). How does that sound?

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

---
 arch/arm64/Kconfig                    |  1 +
 arch/arm64/include/asm/processor.h    |  6 ++++
 arch/arm64/kernel/Makefile            |  3 ++
 arch/arm64/kernel/entry.S             |  6 ++++
 arch/arm64/kernel/erase.c             | 55 +++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/process.c           | 16 ++++++++++
 drivers/firmware/efi/libstub/Makefile |  3 +-
 scripts/Makefile.gcc-plugins          |  5 +++-
 8 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/kernel/erase.c

arch/arm64/Kconfig +1 -0

index eb2cf4938f6d..b0221db95dc9 100644
@@ -92,6 +92,7 @@ config ARM64
        select HAVE_ARCH_MMAP_RND_BITS
        select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
        select HAVE_ARCH_SECCOMP_FILTER
+       select HAVE_ARCH_STACKLEAK
        select HAVE_ARCH_THREAD_STRUCT_WHITELIST
        select HAVE_ARCH_TRACEHOOK
        select HAVE_ARCH_TRANSPARENT_HUGEPAGE

arch/arm64/include/asm/processor.h +0 -0

index 767598932549..d31ab80ff647 100644

arch/arm64/include/asm/processor.h +0 -0

 static inline void arch_thread_struct_whitelist(unsigned long *offset,

arch/arm64/kernel/Makefile +3 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)         += crash_dump.o
 arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)  += sdei.o
 
+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o := n
+
 obj-y                                  += $(arm64-obj-y) vdso/ probes/
 obj-m                                  += $(arm64-obj-m)
 head-y                                 := head.o

arch/arm64/kernel/entry.S +5 -0

index ec2ee720e33e..3144f1ebdc18 100644
@@ -401,6 +401,11 @@ tsk        .req    x28             // current thread_info
 
        .text
 
+       .macro  ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       bl      erase_kstack
+#endif
+       .endm
 /*
  * Exception vectors.
  */

arch/arm64/kernel/erase.c new file +33 -0

new file mode 100644
index 000000000000..b8b5648d893b
@@ -0,0 +1,55 @@
+#include <linux/bug.h>
+#include <linux/sched.h>
+#include <asm/current.h>
+#include <asm/linkage.h>
+#include <asm/processor.h>
+
+asmlinkage void erase_kstack(void)
+{
+       unsigned long p = current->thread.lowest_stack;
+       unsigned long boundary = p & ~(THREAD_SIZE - 1);
+       unsigned long poison = 0;
+       const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+                                                       sizeof(unsigned long);
+
+       /*
+        * Let's search for the poison value in the stack.
+        * Start from the lowest_stack and go to the bottom.
+        */
+       while (p > boundary && poison <= check_depth) {
+               if (*(unsigned long *)p == STACKLEAK_POISON)
+                       poison++;
+               else
+                       poison = 0;
+
+               p -= sizeof(unsigned long);
+       }
+
+       /*
+        * One long int at the bottom of the thread stack is reserved and
+        * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+        */
+       if (p == boundary)
+               p += sizeof(unsigned long);

arch/arm64/kernel/erase.c new file +11 -0

+       BUG_ON(boundary - p >= THREAD_SIZE);
+
+       while (p < boundary) {
+               *(unsigned long *)p = STACKLEAK_POISON;
+               p += sizeof(unsigned long);
+       }
+
+       /* Reset the lowest_stack value for the next syscall */
+       current->thread.lowest_stack = current_stack_pointer;
+}
+

arch/arm64/kernel/process.c +5 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
        p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
        p->thread.cpu_context.sp = (unsigned long)childregs;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       p->thread.lowest_stack = (unsigned long)task_stack_page(p);
+#endif
        ptrace_hw_copy_thread(p);
 
        return 0;

@@ -493,3 +496,16 @@ void arch_setup_new_exec(void)
 {
        current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK

arch/arm64/kernel/process.c +1 -0

+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)  += -I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS                  := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
                                   -D__NO_FORTIFY \
                                   $(call cc-option,-ffreestanding) \
-                                  $(call cc-option,-fno-stack-protector)
+                                  $(call cc-option,-fno-stack-protector) \
+                                  $(DISABLE_STACKLEAK_PLUGIN)
 
 GCOV_PROFILE                   := n
 KASAN_SANITIZE                 := n

scripts/Makefile.gcc-plugins +4 -2

index 8d6070fc538f..6cc0e35d3324 100644
@@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS
 
   gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)    += stackleak_plugin.so
   gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)     += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+    DISABLE_STACKLEAK_PLUGIN           += -fplugin-arg-stackleak_plugin-disable
+  endif
 
   GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
   export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
-  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
+  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN
 
   ifneq ($(PLUGINCC),)
     # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.
--

2.14.3

Quoted 2 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Awesome! Notes below for both you and Alexander, since I think we can create a common code base instead of having near-duplicates in the arch/ trees...

Quoted 33 lines

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

---
  arch/arm64/Kconfig                    |  1 +
  arch/arm64/include/asm/processor.h    |  6 ++++
  arch/arm64/kernel/Makefile            |  3 ++
  arch/arm64/kernel/entry.S             |  6 ++++
  arch/arm64/kernel/erase.c             | 55 +++++++++++++++++++++++++++++++++++
  arch/arm64/kernel/process.c           | 16 ++++++++++
  drivers/firmware/efi/libstub/Makefile |  3 +-
  scripts/Makefile.gcc-plugins          |  5 +++-
  8 files changed, 93 insertions(+), 2 deletions(-)

create mode 100644 arch/arm64/kernel/erase.c

arch/arm64/Kconfig +1 -0

index eb2cf4938f6d..b0221db95dc9 100644
@@ -92,6 +92,7 @@ config ARM64
         select HAVE_ARCH_MMAP_RND_BITS
         select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
         select HAVE_ARCH_SECCOMP_FILTER
+       select HAVE_ARCH_STACKLEAK
         select HAVE_ARCH_THREAD_STRUCT_WHITELIST
         select HAVE_ARCH_TRACEHOOK
         select HAVE_ARCH_TRANSPARENT_HUGEPAGE

arch/arm64/include/asm/processor.h +0 -0

index 767598932549..d31ab80ff647 100644

arch/arm64/include/asm/processor.h struct thread_struct { · line 132

Quoted 9 lines

arch/arm64/include/asm/processor.h +6 -0

@@ -124,6 +124,12 @@ struct thread_struct {
         unsigned long           fault_address;  /* fault info */
         unsigned long           fault_code;     /* ESR_EL1 value */
         struct debug_info       debug;          /* debugging */
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       unsigned long           lowest_stack;
+#ifdef CONFIG_STACKLEAK_METRICS
+       unsigned long           prev_lowest_stack;
+#endif
+#endif

I wonder if x86 and arm64 could include a common struct here that was empty when the plugin is disabled... it would keep the ifdefs in one place. Maybe include/linux/stackleak.h could be:

---start---
/* Poison value points to the unused hole in the virtual memory map */ #define STACKLEAK_POISON -0xBEEF
#define STACKLEAK_POISON_CHECK_DEPTH 128

struct stackleak {

#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
        unsigned long           lowest;
#ifdef CONFIG_STACKLEAK_METRICS
        unsigned long           prev_lowest;

#endif
#endif
};

asmlinkage void erase_kstack(void);
---eof---

and arch/*/include/asm/processor.h could do:

arch/arm64/include/asm/processor.h +1 -0

@@ -124,6 +124,12 @@ struct thread_struct {
         unsigned long           fault_address;  /* fault info */
         unsigned long           fault_code;     /* ESR_EL1 value */
         struct debug_info       debug;          /* debugging */
+       struct stackleak         stackleak;
 

and arch/x86/entry/erase.c could move to maybe kernel/stackleak.c? (Oh, I notice this needs an SPDX line too.)

Quoted 32 lines

arch/arm64/include/asm/processor.h +0 -0

  static inline void arch_thread_struct_whitelist(unsigned long *offset,

arch/arm64/kernel/Makefile +3 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
  arm64-obj-$(CONFIG_CRASH_DUMP)         += crash_dump.o
  arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)  += sdei.o
 
+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o := n
+
  obj-y                                  += $(arm64-obj-y) vdso/ probes/
  obj-m                                  += $(arm64-obj-m)
  head-y                                 := head.o

arch/arm64/kernel/entry.S +5 -0

index ec2ee720e33e..3144f1ebdc18 100644
@@ -401,6 +401,11 @@ tsk        .req    x28             // current thread_info
 
         .text
 
+       .macro  ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       bl      erase_kstack
+#endif
+       .endm
  /*
   * Exception vectors.
   */

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 6 lines

arch/arm64/kernel/entry.S +1 -0

@@ -906,6 +911,7 @@ ret_to_user:
         cbnz    x2, work_pending
  finish_ret_to_user:
         enable_step_tsk x1, x2
+       ERASE_KSTACK
         kernel_exit 0
  ENDPROC(ret_to_user)

Nice. All of the return paths end up here (I went looking for ret_from_fork's path). :)

Quoted 40 lines

arch/arm64/kernel/erase.c new file +33 -0

new file mode 100644
index 000000000000..b8b5648d893b
@@ -0,0 +1,55 @@
+#include <linux/bug.h>
+#include <linux/sched.h>
+#include <asm/current.h>
+#include <asm/linkage.h>
+#include <asm/processor.h>
+
+asmlinkage void erase_kstack(void)
+{
+       unsigned long p = current->thread.lowest_stack;
+       unsigned long boundary = p & ~(THREAD_SIZE - 1);
+       unsigned long poison = 0;
+       const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+                                                       sizeof(unsigned long);
+
+       /*
+        * Let's search for the poison value in the stack.
+        * Start from the lowest_stack and go to the bottom.
+        */
+       while (p > boundary && poison <= check_depth) {
+               if (*(unsigned long *)p == STACKLEAK_POISON)
+                       poison++;
+               else
+                       poison = 0;
+
+               p -= sizeof(unsigned long);
+       }
+
+       /*
+        * One long int at the bottom of the thread stack is reserved and
+        * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+        */
+       if (p == boundary)
+               p += sizeof(unsigned long);

arch/arm64/kernel/erase.c line 43

Quoted 10 lines

arch/arm64/kernel/erase.c new file +10 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+       current->thread.prev_lowest_stack = p;
+#endif
+
+       /*
+        * So let's write the poison value to the kernel stack.
+        * Start from the address in p and move up till the new boundary.
+        */
+       boundary = current_stack_pointer;

This is the only difference between x86 and arm64 in this code. What do you think about implementing on_thread_stack() to match x86:

         if (on_thread_stack())
                 boundary = current_stack_pointer;
         else
                 boundary = current_top_of_stack();

then we could make this common code too instead of having two copies in arch/?

Quoted 36 lines

arch/arm64/kernel/erase.c new file +11 -0

+       BUG_ON(boundary - p >= THREAD_SIZE);
+
+       while (p < boundary) {
+               *(unsigned long *)p = STACKLEAK_POISON;
+               p += sizeof(unsigned long);
+       }
+
+       /* Reset the lowest_stack value for the next syscall */
+       current->thread.lowest_stack = current_stack_pointer;
+}
+

arch/arm64/kernel/process.c +10 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
         p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
         p->thread.cpu_context.sp = (unsigned long)childregs;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       p->thread.lowest_stack = (unsigned long)task_stack_page(p);
+#endif
         ptrace_hw_copy_thread(p);
 
         return 0;

@@ -493,3 +496,16 @@ void arch_setup_new_exec(void)
  {
         current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
  }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+       unsigned long sp, stack_left;
+
+       sp = current_stack_pointer;

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 510

Quoted 5 lines

arch/arm64/kernel/process.c +5 -0

+
+       stack_left = sp & (THREAD_SIZE - 1);
+       BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}
+EXPORT_SYMBOL(check_alloca);

This is pretty different from x86. Is this just an artifact of ORC, or something else?

Quoted 33 lines

arch/arm64/kernel/process.c +1 -0

+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)  += -I$(srctree)/scripts/dtc/libfdt
  KBUILD_CFLAGS                  := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
                                    -D__NO_FORTIFY \
                                    $(call cc-option,-ffreestanding) \
-                                  $(call cc-option,-fno-stack-protector)
+                                  $(call cc-option,-fno-stack-protector) \
+                                  $(DISABLE_STACKLEAK_PLUGIN)
 
  GCOV_PROFILE                   := n
  KASAN_SANITIZE                 := n

scripts/Makefile.gcc-plugins +4 -1

index 8d6070fc538f..6cc0e35d3324 100644
@@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS
 
    gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)    += stackleak_plugin.so
    gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)     += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+    DISABLE_STACKLEAK_PLUGIN           += -fplugin-arg-stackleak_plugin-disable
+  endif
 
    GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
    export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
-  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
+  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN
 

Quoted 5 lines

scripts/Makefile.gcc-plugins +0 -1

    ifneq ($(PLUGINCC),)
      # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.
--

2.14.3

-Kees

Quoted 9 lines · Kees Cook

struct stackleak {

#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
        unsigned long           lowest;
#ifdef CONFIG_STACKLEAK_METRICS
        unsigned long           prev_lowest;

#endif
#endif
};

Is this well defined across all compilers if the plugin is off? This seems to compile with gcc at least but 0 sized structs make me a little uneasy.

Quoted 8 lines

This is the only difference between x86 and arm64 in this code. What do you think about implementing on_thread_stack() to match x86:

         if (on_thread_stack())
                 boundary = current_stack_pointer;
         else
                 boundary = current_top_of_stack();

Quoted 3 lines

then we could make this common code too instead of having two copies in arch/?

The issue isn't on_thread_stack, it's current_top_of_stack which isn't defined on arm64. I agree it would be good if the code would be common but I'm not sure how much we want to start trying to force APIs.

Quoted 7 lines

Quoted 7 lines

arch/arm64/kernel/process.c +7 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+       unsigned long sp, stack_left;
+
+       sp = current_stack_pointer;
+

Quoted 9 lines

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 510

Quoted 4 lines

arch/arm64/kernel/process.c +4 -0

+       stack_left = sp & (THREAD_SIZE - 1);
+       BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}
+EXPORT_SYMBOL(check_alloca);

This is pretty different from x86. Is this just an artifact of ORC, or something else?

This was based on the earlier version of x86. I'll confess to not seeing how the current x86 version ended up with get_stack_info but I suspect it's either related to ORC unwinding or it's best practice.

Quoted 7 lines · Kees Cook

Quoted 2 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Awesome! Notes below for both you and Alexander, since I think we can create a common code base instead of having near-duplicates in the arch/ trees...

Quoted 51 lines

Quoted 32 lines

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

---
  arch/arm64/Kconfig                    |  1 +
  arch/arm64/include/asm/processor.h    |  6 ++++
  arch/arm64/kernel/Makefile            |  3 ++
  arch/arm64/kernel/entry.S             |  6 ++++
  arch/arm64/kernel/erase.c             | 55 +++++++++++++++++++++++++++++++++++
  arch/arm64/kernel/process.c           | 16 ++++++++++
  drivers/firmware/efi/libstub/Makefile |  3 +-
  scripts/Makefile.gcc-plugins          |  5 +++-
  8 files changed, 93 insertions(+), 2 deletions(-)

create mode 100644 arch/arm64/kernel/erase.c

arch/arm64/Kconfig +1 -0

index eb2cf4938f6d..b0221db95dc9 100644
@@ -92,6 +92,7 @@ config ARM64
         select HAVE_ARCH_MMAP_RND_BITS
         select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
         select HAVE_ARCH_SECCOMP_FILTER
+       select HAVE_ARCH_STACKLEAK
         select HAVE_ARCH_THREAD_STRUCT_WHITELIST
         select HAVE_ARCH_TRACEHOOK
         select HAVE_ARCH_TRANSPARENT_HUGEPAGE

arch/arm64/include/asm/processor.h +0 -0

index 767598932549..d31ab80ff647 100644

arch/arm64/include/asm/processor.h struct thread_struct { · line 132

Quoted 9 lines

arch/arm64/include/asm/processor.h +6 -0

@@ -124,6 +124,12 @@ struct thread_struct {
         unsigned long           fault_address;  /* fault info */
         unsigned long           fault_code;     /* ESR_EL1 value */
         struct debug_info       debug;          /* debugging */
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       unsigned long           lowest_stack;
+#ifdef CONFIG_STACKLEAK_METRICS
+       unsigned long           prev_lowest_stack;
+#endif
+#endif

I wonder if x86 and arm64 could include a common struct here that was empty when the plugin is disabled... it would keep the ifdefs in one place. Maybe include/linux/stackleak.h could be:

---start---
/* Poison value points to the unused hole in the virtual memory map */ #define STACKLEAK_POISON -0xBEEF
#define STACKLEAK_POISON_CHECK_DEPTH 128

Quoted 9 lines

struct stackleak {

#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
        unsigned long           lowest;
#ifdef CONFIG_STACKLEAK_METRICS
        unsigned long           prev_lowest;

#endif
#endif
};

Is this well defined across all compilers if the plugin is off? This seems to compile with gcc at least but 0 sized structs make me a little uneasy.

Quoted 5 lines

asmlinkage void erase_kstack(void);
---eof---

and arch/*/include/asm/processor.h could do:

Quoted 8 lines

arch/arm64/include/asm/processor.h +1 -0

@@ -124,6 +124,12 @@ struct thread_struct {
         unsigned long           fault_address;  /* fault info */
         unsigned long           fault_code;     /* ESR_EL1 value */
         struct debug_info       debug;          /* debugging */
+       struct stackleak         stackleak;
 

and arch/x86/entry/erase.c could move to maybe kernel/stackleak.c? (Oh, I notice this needs an SPDX line too.)

Quoted 96 lines

Quoted 32 lines

arch/arm64/include/asm/processor.h +0 -0

  static inline void arch_thread_struct_whitelist(unsigned long *offset,

arch/arm64/kernel/Makefile +3 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
  arm64-obj-$(CONFIG_CRASH_DUMP)         += crash_dump.o
  arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)  += sdei.o
 
+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o := n
+
  obj-y                                  += $(arm64-obj-y) vdso/ probes/
  obj-m                                  += $(arm64-obj-m)
  head-y                                 := head.o

arch/arm64/kernel/entry.S +5 -0

index ec2ee720e33e..3144f1ebdc18 100644
@@ -401,6 +401,11 @@ tsk        .req    x28             // current thread_info
 
         .text
 
+       .macro  ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       bl      erase_kstack
+#endif
+       .endm
  /*
   * Exception vectors.
   */

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 6 lines

arch/arm64/kernel/entry.S +1 -0

@@ -906,6 +911,7 @@ ret_to_user:
         cbnz    x2, work_pending
  finish_ret_to_user:
         enable_step_tsk x1, x2
+       ERASE_KSTACK
         kernel_exit 0
  ENDPROC(ret_to_user)

Nice. All of the return paths end up here (I went looking for ret_from_fork's path). :)

Quoted 40 lines

arch/arm64/kernel/erase.c new file +33 -0

new file mode 100644
index 000000000000..b8b5648d893b
@@ -0,0 +1,55 @@
+#include <linux/bug.h>
+#include <linux/sched.h>
+#include <asm/current.h>
+#include <asm/linkage.h>
+#include <asm/processor.h>
+
+asmlinkage void erase_kstack(void)
+{
+       unsigned long p = current->thread.lowest_stack;
+       unsigned long boundary = p & ~(THREAD_SIZE - 1);
+       unsigned long poison = 0;
+       const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+                                                       sizeof(unsigned long);
+
+       /*
+        * Let's search for the poison value in the stack.
+        * Start from the lowest_stack and go to the bottom.
+        */
+       while (p > boundary && poison <= check_depth) {
+               if (*(unsigned long *)p == STACKLEAK_POISON)
+                       poison++;
+               else
+                       poison = 0;
+
+               p -= sizeof(unsigned long);
+       }
+
+       /*
+        * One long int at the bottom of the thread stack is reserved and
+        * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+        */
+       if (p == boundary)
+               p += sizeof(unsigned long);

arch/arm64/kernel/erase.c line 43

Quoted 10 lines

arch/arm64/kernel/erase.c new file +10 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+       current->thread.prev_lowest_stack = p;
+#endif
+
+       /*
+        * So let's write the poison value to the kernel stack.
+        * Start from the address in p and move up till the new boundary.
+        */
+       boundary = current_stack_pointer;

This is the only difference between x86 and arm64 in this code. What do you think about implementing on_thread_stack() to match x86:

Quoted 7 lines

         if (on_thread_stack())
                 boundary = current_stack_pointer;
         else
                 boundary = current_top_of_stack();

then we could make this common code too instead of having two copies in arch/?

The issue isn't on_thread_stack, it's current_top_of_stack which isn't defined on arm64. I agree it would be good if the code would be common but I'm not sure how much we want to start trying to force APIs.

Quoted 19 lines

Quoted 19 lines

arch/arm64/kernel/erase.c new file +11 -0

+       BUG_ON(boundary - p >= THREAD_SIZE);
+
+       while (p < boundary) {
+               *(unsigned long *)p = STACKLEAK_POISON;
+               p += sizeof(unsigned long);
+       }
+
+       /* Reset the lowest_stack value for the next syscall */
+       current->thread.lowest_stack = current_stack_pointer;
+}
+

arch/arm64/kernel/process.c +0 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
         p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
         p->thread.cpu_context.sp = (unsigned long)childregs;
 

Quoted 3 lines

arch/arm64/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 369

Quoted 3 lines

arch/arm64/kernel/process.c +3 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+       p->thread.lowest_stack = (unsigned long)task_stack_page(p);
+#endif

Quoted 14 lines

Quoted 14 lines

arch/arm64/kernel/process.c +7 -0

         ptrace_hw_copy_thread(p);
 
         return 0;

@@ -493,3 +496,16 @@ void arch_setup_new_exec(void)
  {
         current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
  }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+       unsigned long sp, stack_left;
+
+       sp = current_stack_pointer;

Quoted 9 lines

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 510

Quoted 5 lines

arch/arm64/kernel/process.c +5 -0

+
+       stack_left = sp & (THREAD_SIZE - 1);
+       BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}
+EXPORT_SYMBOL(check_alloca);

This is pretty different from x86. Is this just an artifact of ORC, or something else?

This was based on the earlier version of x86. I'll confess to not seeing how the current x86 version ended up with get_stack_info but I suspect it's either related to ORC unwinding or it's best practice.

Quoted 33 lines

Quoted 33 lines

arch/arm64/kernel/process.c +1 -0

+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)  += -I$(srctree)/scripts/dtc/libfdt
  KBUILD_CFLAGS                  := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
                                    -D__NO_FORTIFY \
                                    $(call cc-option,-ffreestanding) \
-                                  $(call cc-option,-fno-stack-protector)
+                                  $(call cc-option,-fno-stack-protector) \
+                                  $(DISABLE_STACKLEAK_PLUGIN)
 
  GCOV_PROFILE                   := n
  KASAN_SANITIZE                 := n

scripts/Makefile.gcc-plugins +4 -1

index 8d6070fc538f..6cc0e35d3324 100644
@@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS
 
    gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)    += stackleak_plugin.so
    gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK)     += -DSTACKLEAK_PLUGIN -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+    DISABLE_STACKLEAK_PLUGIN           += -fplugin-arg-stackleak_plugin-disable
+  endif
 
    GCC_PLUGINS_CFLAGS := $(strip $(addprefix -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) $(gcc-plugin-cflags-y))
 
    export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
-  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
+  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN
 

Quoted 5 lines

Quoted 5 lines

scripts/Makefile.gcc-plugins +0 -1

    ifneq ($(PLUGINCC),)
      # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.
--

2.14.3

Hello Laura and Kees,

Quoted 5 lines · Laura Abbott

Quoted 4 lines · Kees Cook

Quoted 2 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Quoted 3 lines

Quoted 3 lines

Awesome! Notes below for both you and Alexander, since I think we can create a common code base instead of having near-duplicates in the arch/ trees...

Yes, sure.

I will extract the common part and send v12 for x86. Then Laura will be able to add arm64 support in a separate patch series. Is it fine?

Quoted 4 lines

This was based on the earlier version of x86. I'll confess to not seeing how the current x86 version ended up with get_stack_info but I suspect it's either related to ORC unwinding or it's best practice.

I've changed that in v4. Quote from the changelog:
- Fixed the surplus and erroneous code for calculating stack_left in check_alloca() on x86_64. That code repeats the work which is already done in get_stack_info() and it misses the fact that different exception stacks on x86_64 have different size.

http://www.openwall.com/lists/kernel-hardening/2017/10/04/68

We can see that in arch/x86/kernel/dumpstack_64.c.

Is it fine if check_alloca() would be arch-specific?

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

arch/arm64/kernel/Makefile +1 -0

+
 obj-y					+= $(arm64-obj-y) vdso/ probes/
 obj-m					+= $(arm64-obj-m)
 head-y					:= head.o

arch/arm64/kernel/entry.S +0 -0

index ec2ee720e33e..3144f1ebdc18 100644

arch/arm64/kernel/entry.S +17 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;

arch/arm64/kernel/process.c +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +0 -0

index a34e9290a699..25dd2a14560d 100644

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 10 lines

arch/arm64/kernel/Makefile +2 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)               += crash_dump.o
 arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)        += sdei.o
 
+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o       := n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

Quoted 9 lines

arch/arm64/kernel/Makefile +1 -0

+
 obj-y                                        += $(arm64-obj-y) vdso/ probes/
 obj-m                                        += $(arm64-obj-m)
 head-y                                       := head.o

arch/arm64/kernel/entry.S +0 -0

index ec2ee720e33e..3144f1ebdc18 100644

arch/arm64/kernel/entry.S tsk .req x28 // current thread_info · line 408

Quoted 8 lines

arch/arm64/kernel/entry.S +5 -0

@@ -401,6 +401,11 @@ tsk      .req    x28             // current thread_info
 
      .text
 
+     .macro  ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+     bl      erase_kstack
+#endif
+     .endm

Nit: The rest of our asm macros are lower-case -- can we stick to that here?

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 10 lines

arch/arm64/kernel/entry.S +1 -0

 /*
  * Exception vectors.
  */

@@ -906,6 +911,7 @@ ret_to_user:
      cbnz    x2, work_pending
 finish_ret_to_user:
      enable_step_tsk x1, x2
+     ERASE_KSTACK
      kernel_exit 0
 ENDPROC(ret_to_user)

I believe we also need this in ret_fast_syscall.

[...]

Quoted 17 lines

arch/arm64/kernel/entry.S +17 -0

+asmlinkage void erase_kstack(void)
+{
+     unsigned long p = current->thread.lowest_stack;
+     unsigned long boundary = p & ~(THREAD_SIZE - 1);
+     unsigned long poison = 0;
+     const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+                                                     sizeof(unsigned long);
+
+     /*
+      * Let's search for the poison value in the stack.
+      * Start from the lowest_stack and go to the bottom.
+      */
+     while (p > boundary && poison <= check_depth) {
+             if (*(unsigned long *)p == STACKLEAK_POISON)
+                     poison++;
+             else
+                     poison = 0;

arch/arm64/kernel/entry.S ret_to_user:

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+             p -= sizeof(unsigned long);
+     }
+
+     /*
+      * One long int at the bottom of the thread stack is reserved and
+      * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+      */
+     if (p == boundary)
+             p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

If we did, then IIUC we could do:

        unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

arch/arm64/kernel/entry.S

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+     current->thread.prev_lowest_stack = p;
+#endif
+
+     /*
+      * So let's write the poison value to the kernel stack.
+      * Start from the address in p and move up till the new boundary.
+      */
+     boundary = current_stack_pointer;

I worry a little that the compiler can move the SP during a function's lifetime, but maybe that's only the case when there are VLAs, or something like that?

arch/arm64/kernel/entry.S

Quoted 11 lines

arch/arm64/kernel/entry.S +11 -0

+
+     BUG_ON(boundary - p >= THREAD_SIZE);
+
+     while (p < boundary) {
+             *(unsigned long *)p = STACKLEAK_POISON;
+             p += sizeof(unsigned long);
+     }
+
+     /* Reset the lowest_stack value for the next syscall */
+     current->thread.lowest_stack = current_stack_pointer;
+}

Once this function returns, its data is left on the stack. Is that not a problem?

No strong feelings either way, but it might be worth mentioning in the commit message.

arch/arm64/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 368

Quoted 10 lines

arch/arm64/kernel/process.c +2 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
      p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
      p->thread.cpu_context.sp = (unsigned long)childregs;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+     p->thread.lowest_stack = (unsigned long)task_stack_page(p);

Nit: end_of_stack(p) would be slightly better semantically, even though currently equivalent to task_stack_page(p).

[...]

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 509

Quoted 10 lines

arch/arm64/kernel/process.c +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+     unsigned long sp, stack_left;
+
+     sp = current_stack_pointer;
+
+     stack_left = sp & (THREAD_SIZE - 1);
+     BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

Quoted 7 lines

arch/arm64/kernel/process.c +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +0 -0

index a34e9290a699..25dd2a14560d 100644

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 9 lines

drivers/firmware/efi/libstub/Makefile +2 -1

@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)        += -I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS                        := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
                                 -D__NO_FORTIFY \
                                 $(call cc-option,-ffreestanding) \
-                                $(call cc-option,-fno-stack-protector)
+                                $(call cc-option,-fno-stack-protector) \
+                                $(DISABLE_STACKLEAK_PLUGIN)
 
 GCOV_PROFILE                 := n
 KASAN_SANITIZE                       := n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Thanks,
Mark.

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

Quoted 8 lines

arch/arm64/kernel/Makefile +0 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
 arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)	+= sdei.o
 

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 2 lines

arch/arm64/kernel/Makefile +2 -0

+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o	:= n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

Quoted 9 lines

arch/arm64/kernel/Makefile +1 -0

+
 obj-y					+= $(arm64-obj-y) vdso/ probes/
 obj-m					+= $(arm64-obj-m)
 head-y					:= head.o

arch/arm64/kernel/entry.S +0 -0

index ec2ee720e33e..3144f1ebdc18 100644

arch/arm64/kernel/entry.S tsk .req x28 // current thread_info · line 408

Quoted 8 lines

arch/arm64/kernel/entry.S +5 -0

@@ -401,6 +401,11 @@ tsk	.req	x28		// current thread_info
 
 	.text
 
+	.macro	ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	erase_kstack
+#endif
+	.endm

Nit: The rest of our asm macros are lower-case -- can we stick to that here?

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 10 lines

arch/arm64/kernel/entry.S +1 -0

 /*
  * Exception vectors.
  */

@@ -906,6 +911,7 @@ ret_to_user:
 	cbnz	x2, work_pending
 finish_ret_to_user:
 	enable_step_tsk x1, x2
+	ERASE_KSTACK
 	kernel_exit 0
 ENDPROC(ret_to_user)

I believe we also need this in ret_fast_syscall.

[...]

Quoted 23 lines

arch/arm64/kernel/entry.S +23 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;
+
+		p -= sizeof(unsigned long);
+	}
+
+	/*
+	 * One long int at the bottom of the thread stack is reserved and

arch/arm64/kernel/entry.S

Quoted 4 lines

arch/arm64/kernel/entry.S +4 -0

+	 * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	if (p == boundary)
+		p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

If we did, then IIUC we could do:

	unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

Quoted 6 lines

arch/arm64/kernel/entry.S +6 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+	current->thread.prev_lowest_stack = p;
+#endif
+
+	/*

arch/arm64/kernel/entry.S

Quoted 4 lines

arch/arm64/kernel/entry.S +4 -0

+	 * So let's write the poison value to the kernel stack.
+	 * Start from the address in p and move up till the new boundary.
+	 */
+	boundary = current_stack_pointer;

I worry a little that the compiler can move the SP during a function's lifetime, but maybe that's only the case when there are VLAs, or something like that?

arch/arm64/kernel/entry.S

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+	BUG_ON(boundary - p >= THREAD_SIZE);
+
+	while (p < boundary) {
+		*(unsigned long *)p = STACKLEAK_POISON;
+		p += sizeof(unsigned long);
+	}
+
+	/* Reset the lowest_stack value for the next syscall */
+	current->thread.lowest_stack = current_stack_pointer;

Quoted 1 line

+}

Once this function returns, its data is left on the stack. Is that not a problem?

No strong feelings either way, but it might be worth mentioning in the commit message.

Quoted 8 lines

arch/arm64/kernel/process.c +0 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
 	p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
 	p->thread.cpu_context.sp = (unsigned long)childregs;
 

arch/arm64/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 368

Quoted 2 lines

arch/arm64/kernel/process.c +2 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	p->thread.lowest_stack = (unsigned long)task_stack_page(p);

Nit: end_of_stack(p) would be slightly better semantically, even though currently equivalent to task_stack_page(p).

[...]

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 509

Quoted 10 lines

arch/arm64/kernel/process.c +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+	unsigned long sp, stack_left;
+
+	sp = current_stack_pointer;
+
+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

Quoted 14 lines

arch/arm64/kernel/process.c +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
 				   -D__NO_FORTIFY \
 				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 2 lines

drivers/firmware/efi/libstub/Makefile +0 -0

 GCOV_PROFILE			:= n
 KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Hello Mark and Laura,

Let me join the discussion. Mark, thanks for your feedback!

Quoted 22 lines · Mark Rutland

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

Quoted 8 lines

arch/arm64/kernel/Makefile +0 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
  arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
  arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)	+= sdei.o
 

Quoted 5 lines

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 2 lines

arch/arm64/kernel/Makefile +2 -0

+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o	:= n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

I've disabled KASAN instrumentation for that file on x86 because erase_kstack() intentionally writes to the stack and causes KASAN false positive reports.

But I didn't see any conflicts with other types of instrumentation that you mentioned.

Quoted 58 lines

Quoted 9 lines

arch/arm64/kernel/Makefile +1 -0

+
  obj-y					+= $(arm64-obj-y) vdso/ probes/
  obj-m					+= $(arm64-obj-m)
  head-y					:= head.o

arch/arm64/kernel/entry.S +0 -0

index ec2ee720e33e..3144f1ebdc18 100644

arch/arm64/kernel/entry.S tsk .req x28 // current thread_info · line 408

Quoted 8 lines

arch/arm64/kernel/entry.S +5 -0

@@ -401,6 +401,11 @@ tsk	.req	x28		// current thread_info
 
  	.text
 
+	.macro	ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	erase_kstack
+#endif
+	.endm

Nit: The rest of our asm macros are lower-case -- can we stick to that here?

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 10 lines

arch/arm64/kernel/entry.S +1 -0

  /*
   * Exception vectors.
   */

@@ -906,6 +911,7 @@ ret_to_user:
  	cbnz	x2, work_pending
  finish_ret_to_user:
  	enable_step_tsk x1, x2
+	ERASE_KSTACK
  	kernel_exit 0
  ENDPROC(ret_to_user)

I believe we also need this in ret_fast_syscall.

[...]

Quoted 23 lines

arch/arm64/kernel/entry.S +23 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;
+
+		p -= sizeof(unsigned long);
+	}
+
+	/*
+	 * One long int at the bottom of the thread stack is reserved and

Quoted 8 lines

arch/arm64/kernel/entry.S

Quoted 4 lines

arch/arm64/kernel/entry.S +4 -0

+	 * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	if (p == boundary)
+		p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

I would be afraid to change the meaning of end_of_stack()... Currently it considers that magic long as usable (include/linux/sched/task_stack.h):

#define task_stack_end_corrupted(task) \
		(*(end_of_stack(task)) != STACK_END_MAGIC)

Quoted 5 lines

If we did, then IIUC we could do:

	unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

I should mention that erase_kstack() can be called from x86 trampoline stack. That's why the boundary is calculated from the lowest_stack.

Quoted 6 lines

Quoted 6 lines

arch/arm64/kernel/entry.S +6 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+	current->thread.prev_lowest_stack = p;
+#endif
+
+	/*

Quoted 8 lines

arch/arm64/kernel/entry.S

Quoted 4 lines

arch/arm64/kernel/entry.S +4 -0

+	 * So let's write the poison value to the kernel stack.
+	 * Start from the address in p and move up till the new boundary.
+	 */
+	boundary = current_stack_pointer;

I worry a little that the compiler can move the SP during a function's lifetime, but maybe that's only the case when there are VLAs, or something like that?

Oh, I don't know.

However, erase_kstack() doesn't call anything except simple inline functions. And as I see from its disasm on x86, the local variables reside in registers.

Quoted 10 lines

arch/arm64/kernel/entry.S

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+	BUG_ON(boundary - p >= THREAD_SIZE);
+
+	while (p < boundary) {
+		*(unsigned long *)p = STACKLEAK_POISON;
+		p += sizeof(unsigned long);
+	}
+
+	/* Reset the lowest_stack value for the next syscall */
+	current->thread.lowest_stack = current_stack_pointer;

Laura, that might be wrong and introduce huge performance impact.

I think, lowest_stack should be reset similarly to the original version.

Quoted 6 lines

Quoted 1 line

+}

Once this function returns, its data is left on the stack. Is that not a problem?

No strong feelings either way, but it might be worth mentioning in the commit message.

I managed to bypass that with "register" specifier. Although it doesn't give an absolute guarantee.

Quoted 8 lines

Quoted 8 lines

arch/arm64/kernel/process.c +0 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
  	p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
  	p->thread.cpu_context.sp = (unsigned long)childregs;
 

Quoted 5 lines

arch/arm64/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 368

Quoted 2 lines

arch/arm64/kernel/process.c +2 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	p->thread.lowest_stack = (unsigned long)task_stack_page(p);

Nit: end_of_stack(p) would be slightly better semantically, even though currently equivalent to task_stack_page(p).

Thanks, I agree, I'll fix it in v12.

Quoted 15 lines

[...]

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 509

Quoted 10 lines

arch/arm64/kernel/process.c +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+	unsigned long sp, stack_left;
+
+	sp = current_stack_pointer;
+
+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Quoted 1 line

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

Quoted 14 lines

Quoted 14 lines

arch/arm64/kernel/process.c +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
  KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
  				   -D__NO_FORTIFY \
  				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 

Quoted 4 lines

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 2 lines

drivers/firmware/efi/libstub/Makefile +0 -0

  GCOV_PROFILE			:= n
  KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Could you please give more details on that? Why STACKLEAK breaks it?

Thanks a lot!

Best regards,
Alexander

Quoted 7 lines · Alexander Popov

Quoted 3 lines · Mark Rutland

arch/arm64/kernel/erase.c line 53

Quoted 2 lines · Laura Abbott

arch/arm64/kernel/erase.c new file +2 -0

+	/* Reset the lowest_stack value for the next syscall */
+	current->thread.lowest_stack = current_stack_pointer;

Laura, that might be wrong and introduce huge performance impact.

Quoted 2 lines

I think, lowest_stack should be reset similarly to the original version.

Sorry, I'm not understanding here. What's the performance impact and what do you mean by original version?

Quoted 10 lines

Quoted 6 lines

Quoted 1 line

+}

Once this function returns, its data is left on the stack. Is that not a problem?

No strong feelings either way, but it might be worth mentioning in the commit message.

I managed to bypass that with "register" specifier. Although it doesn't give an absolute guarantee.

I guess I was assuming gcc would be smart enough not to spill stuff on the stack. I also intentionally removed the register keyword since it wasn't clear gcc does much with it on a modern system? I could be completely off base here though so please correct me if I'm wrong. It probably is worth documenting what we are assuming about the compiler here.

Quoted 17 lines

Quoted 16 lines

Quoted 5 lines

drivers/firmware/efi/libstub/Makefile +0 -0

index a34e9290a699..25dd2a14560d 100644

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 9 lines

drivers/firmware/efi/libstub/Makefile +2 -1

@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
  KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
  				   -D__NO_FORTIFY \
  				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 
  GCOV_PROFILE			:= n
  KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Quoted 2 lines

Could you please give more details on that? Why STACKLEAK breaks it?

For reference, I originally added this for the efistub because it would not compile.

I did compile this against my Fedora tree which has KVM enabled.

Hello Mark and Laura,

Let me join the discussion. Mark, thanks for your feedback!

Quoted 22 lines · Mark Rutland

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

Quoted 8 lines

arch/arm64/kernel/Makefile +0 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
 arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)	+= sdei.o
 

Quoted 5 lines

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 2 lines

arch/arm64/kernel/Makefile +2 -0

+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o	:= n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

I've disabled KASAN instrumentation for that file on x86 because erase_kstack() intentionally writes to the stack and causes KASAN false positive reports.

But I didn't see any conflicts with other types of instrumentation that you mentioned.

Quoted 23 lines

Quoted 23 lines

arch/arm64/kernel/Makefile +23 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;
+
+		p -= sizeof(unsigned long);
+	}
+
+	/*
+	 * One long int at the bottom of the thread stack is reserved and

Quoted 8 lines

arch/arm64/kernel/Makefile

Quoted 4 lines

arch/arm64/kernel/Makefile +4 -0

+	 * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	if (p == boundary)
+		p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

I would be afraid to change the meaning of end_of_stack()... Currently it considers that magic long as usable (include/linux/sched/task_stack.h):

#define task_stack_end_corrupted(task) \
		(*(end_of_stack(task)) != STACK_END_MAGIC)

Quoted 5 lines

If we did, then IIUC we could do:

	unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

I should mention that erase_kstack() can be called from x86 trampoline stack. That's why the boundary is calculated from the lowest_stack.

Quoted 13 lines

arch/arm64/kernel/Makefile

Quoted 10 lines

arch/arm64/kernel/Makefile +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+	unsigned long sp, stack_left;
+
+	sp = current_stack_pointer;
+
+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Quoted 1 line

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

Quoted 14 lines

Quoted 14 lines

arch/arm64/kernel/Makefile +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
 				   -D__NO_FORTIFY \
 				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 

Quoted 4 lines

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 2 lines

drivers/firmware/efi/libstub/Makefile +0 -0

 GCOV_PROFILE			:= n
 KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Could you please give more details on that? Why STACKLEAK breaks it?

Quoted 33 lines · Alexander Popov

Hello Mark and Laura,

Let me join the discussion. Mark, thanks for your feedback!

Quoted 22 lines · Mark Rutland

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

Quoted 8 lines

arch/arm64/kernel/Makefile +0 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
 arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
 arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)	+= sdei.o
 

Quoted 5 lines

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 2 lines

arch/arm64/kernel/Makefile +2 -0

+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o	:= n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

Quoted 5 lines

I've disabled KASAN instrumentation for that file on x86 because erase_kstack() intentionally writes to the stack and causes KASAN false positive reports.

But I didn't see any conflicts with other types of instrumentation that you mentioned.

The rationale is that any of these can result in implicit calls to C functions at arbitrary points during erase_kstack(). That could interfere with the search for poison, and/or leave data on the stack which is not erased.

They won't result in hard failures, as KASAN would, but we should probably avoid them regardless.

[...]

Quoted 45 lines

Quoted 23 lines

Quoted 23 lines

arch/arm64/kernel/Makefile +23 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;
+
+		p -= sizeof(unsigned long);
+	}
+
+	/*
+	 * One long int at the bottom of the thread stack is reserved and

Quoted 8 lines

arch/arm64/kernel/Makefile

Quoted 4 lines

arch/arm64/kernel/Makefile +4 -0

+	 * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	if (p == boundary)
+		p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

I would be afraid to change the meaning of end_of_stack()... Currently it considers that magic long as usable (include/linux/sched/task_stack.h):

#define task_stack_end_corrupted(task) \
		(*(end_of_stack(task)) != STACK_END_MAGIC)

Quoted 5 lines

If we did, then IIUC we could do:

	unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

Quoted 2 lines

I should mention that erase_kstack() can be called from x86 trampoline stack. That's why the boundary is calculated from the lowest_stack.

Ok. Under what circumstances does that happen?

It seems a little scary that curent::thread::lowest_stack might not be on current's task stack.

Is that reset when transitioning to/from the
trampoile stack?

[...]

Quoted 13 lines

Quoted 13 lines

arch/arm64/kernel/Makefile

Quoted 10 lines

arch/arm64/kernel/Makefile +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+	unsigned long sp, stack_left;
+
+	sp = current_stack_pointer;
+
+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Quoted 3 lines

Quoted 1 line

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

I'm just not sure I see the point in the offset, given things like VMAP_STACK exist. BUG_ON() handling will likely require *more* than 256 bytes of stack, so it seems superfluous, as we'd be relying on stack overflow detection at that point.

I can see that we should take the CONFIG_SCHED_STACK_END_CHECK offset into account, though.

Quoted 19 lines

Quoted 18 lines

Quoted 7 lines

arch/arm64/kernel/Makefile +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +0 -0

index a34e9290a699..25dd2a14560d 100644

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 9 lines

drivers/firmware/efi/libstub/Makefile +2 -1

@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
 				   -D__NO_FORTIFY \
 				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 
 GCOV_PROFILE			:= n
 KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Quoted 1 line

Could you please give more details on that? Why STACKLEAK breaks it?

In the hyp/EL2 exception level, we only map the hyp text, and not the rest of the kernel. So erase_kstack and check_alloca won't be mapped, and attempt to branch to them will fault.

Even if it were mapped, things like BUG_ON(), get_current(), etc do not work at hyp.

Additionally, the hyp code is mapped as a different virtual address from the rest of the kernel, so if any of the STACKLEAK code happens to use an absolute address, this will not work correctly.

Quoted 9 lines · Mark Rutland

Quoted 8 lines

Quoted 6 lines

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 508

Quoted 2 lines

arch/arm64/kernel/process.c +2 -0

+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

Quoted 7 lines

I'm just not sure I see the point in the offset, given things like VMAP_STACK exist. BUG_ON() handling will likely require *more* than 256 bytes of stack, so it seems superfluous, as we'd be relying on stack overflow detection at that point.

I can see that we should take the CONFIG_SCHED_STACK_END_CHECK offset into account, though.

Mark, thank you for such an important remark!

In Kconfig STACKLEAK implies but doesn't depend on VMAP_STACK. In fact x86_32 doesn't have VMAP_STACK at all but can have STACKLEAK.

[Adding Andy Lutomirski]

I've made some additional experiments: I exhaust the thread stack to have only (MIN_STACK_LEFT - 1) bytes left and then force alloca. If VMAP_STACK is disabled, BUG_ON() handling causes stack depth overflow, which is detected by SCHED_STACK_END_CHECK. If VMAP_STACK is enabled, the kernel hangs on BUG_ON() handling! Enabling CONFIG_PROVE_LOCKING gives the needed report from VMAP_STACK:

I can't say why VMAP_STACK report hangs during BUG_ON() handling on defconfig. Andy, can you give a clue?

I see that MIN_STACK_LEFT = 2048 is enough for BUG_ON() handling on both x86_64 and x86_32. So I'm going to:
- set MIN_STACK_LEFT to 2048;
- improve the lkdtm test to cover this case.

Mark, Kees, Laura, does it sound good?

Hello everyone,

Quoted 30 lines · Alexander Popov

Quoted 9 lines · Mark Rutland

Quoted 8 lines

Quoted 6 lines

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 508

Quoted 2 lines

arch/arm64/kernel/process.c +2 -0

+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

Quoted 7 lines

I'm just not sure I see the point in the offset, given things like VMAP_STACK exist. BUG_ON() handling will likely require *more* than 256 bytes of stack, so it seems superfluous, as we'd be relying on stack overflow detection at that point.

I can see that we should take the CONFIG_SCHED_STACK_END_CHECK offset into account, though.

Mark, thank you for such an important remark!

In Kconfig STACKLEAK implies but doesn't depend on VMAP_STACK. In fact x86_32 doesn't have VMAP_STACK at all but can have STACKLEAK.

[Adding Andy Lutomirski]

I've made some additional experiments: I exhaust the thread stack to have only (MIN_STACK_LEFT - 1) bytes left and then force alloca. If VMAP_STACK is disabled, BUG_ON() handling causes stack depth overflow, which is detected by SCHED_STACK_END_CHECK. If VMAP_STACK is enabled, the kernel hangs on BUG_ON() handling! Enabling CONFIG_PROVE_LOCKING gives the needed report from VMAP_STACK:

[...]

Quoted 8 lines

I can't say why VMAP_STACK report hangs during BUG_ON() handling on defconfig. Andy, can you give a clue?

I see that MIN_STACK_LEFT = 2048 is enough for BUG_ON() handling on both x86_64 and x86_32. So I'm going to:
- set MIN_STACK_LEFT to 2048;
- improve the lkdtm test to cover this case.

Quoted 1 line

Mark, Kees, Laura, does it sound good?

Could you have a look at the following changes in check_alloca() before I send the next version?

If VMAP_STACK is enabled and alloca causes stack depth overflow, I write to guard page below the thread stack to cause double fault and VMAP_STACK report.

If VMAP_STACK is disabled, I use MIN_STACK_LEFT = 2048, which seems to be enough for BUG_ON() handling both on x86_32 and x86_64. Unfortunately, I can't guarantee that it is always enough.

#ifdef CONFIG_GCC_PLUGIN_STACKLEAK

(quoted diff) +1 -1

-#define MIN_STACK_LEFT 256
+#define MIN_STACK_LEFT 2048
 
 void __used check_alloca(unsigned long size)
 {
        unsigned long sp = (unsigned long)&sp;
        struct stack_info stack_info = {0};
        unsigned long visit_mask = 0;
        unsigned long stack_left;
 
        BUG_ON(get_stack_info(&sp, current, &stack_info, &visit_mask));
 

        BUG_ON(stack_left < MIN_STACK_LEFT ||
                                size >= stack_left - MIN_STACK_LEFT);

Quoted 24 lines · Alexander Popov

Quoted 23 lines · Alexander Popov

Quoted 9 lines · Mark Rutland

Quoted 8 lines

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

It's just a reasonable number. We can introduce a macro for it.

Quoted 7 lines

I'm just not sure I see the point in the offset, given things like VMAP_STACK exist. BUG_ON() handling will likely require *more* than 256 bytes of stack, so it seems superfluous, as we'd be relying on stack overflow detection at that point.

I can see that we should take the CONFIG_SCHED_STACK_END_CHECK offset into account, though.

Mark, thank you for such an important remark!

In Kconfig STACKLEAK implies but doesn't depend on VMAP_STACK. In fact x86_32 doesn't have VMAP_STACK at all but can have STACKLEAK.

Quoted 7 lines

Quoted 7 lines

[Adding Andy Lutomirski]

I've made some additional experiments: I exhaust the thread stack to have only (MIN_STACK_LEFT - 1) bytes left and then force alloca. If VMAP_STACK is disabled, BUG_ON() handling causes stack depth overflow, which is detected by SCHED_STACK_END_CHECK. If VMAP_STACK is enabled, the kernel hangs on BUG_ON() handling! Enabling CONFIG_PROVE_LOCKING gives the needed report from VMAP_STACK:

I can't see why CONFIG_VMAP_STACK would only work in conjunction with CONFIG_PROVE_LOCKING.

On arm64 at least, if we overflow the stack while handling a BUG(), we *should* trigger the overflow handler as usual, and that should work, unless I'm missing something.

Maybe it gets part-way into panic(), sets up some state, stack-overflows, and we get wedged because we're already in a panic? Perhaps CONFIG_PROVE_LOCKING causes more stack to be used, so it dies a little earlier in panic(), before setting up some state that causes wedging.

... which sounds like something best fixed in those code paths, and not here.

Quoted 16 lines

[...]

Quoted 9 lines

I can't say why VMAP_STACK report hangs during BUG_ON() handling on defconfig. Andy, can you give a clue?

I see that MIN_STACK_LEFT = 2048 is enough for BUG_ON() handling on both x86_64 and x86_32. So I'm going to:
- set MIN_STACK_LEFT to 2048;
- improve the lkdtm test to cover this case.

Mark, Kees, Laura, does it sound good?

Could you have a look at the following changes in check_alloca() before I send the next version?

Quoted 2 lines

If VMAP_STACK is enabled and alloca causes stack depth overflow, I write to guard page below the thread stack to cause double fault and VMAP_STACK report.

On arm64 at least, writing to the guard page will not itself trigger a stack overflow, but will trigger a data abort. I suspect similar is true on x86, if the stack pointer is sufficiently far above the guard page.

Quoted 3 lines

If VMAP_STACK is disabled, I use MIN_STACK_LEFT = 2048, which seems to be enough for BUG_ON() handling both on x86_32 and x86_64. Unfortunately, I can't guarantee that it is always enough.

I don't think that we can choose something that's guaranteed to be sufficient for BUG() handling and also not wasting a tonne of space under normal operation.

Let's figure out what's going wrong on x86 in the case that you mention, and try to solve that.

Here I don't think we should reserve space at all -- it's completely arbitrary, and as above we can't guarantee that it's sufficient anyway.

Quoted 13 lines

#ifdef CONFIG_GCC_PLUGIN_STACKLEAK

(quoted diff) +1 -1

-#define MIN_STACK_LEFT 256
+#define MIN_STACK_LEFT 2048
 
 void __used check_alloca(unsigned long size)
 {
        unsigned long sp = (unsigned long)&sp;
        struct stack_info stack_info = {0};
        unsigned long visit_mask = 0;
        unsigned long stack_left;
 
        BUG_ON(get_stack_info(&sp, current, &stack_info, &visit_mask));
 

Review

Quoted 10 lines

(quoted diff) +9 -0

        stack_left = sp - (unsigned long)stack_info.begin;
+
+#ifdef CONFIG_VMAP_STACK
+       /*
+        * If alloca oversteps the thread stack boundary, we touch the guard
+        * page provided by VMAP_STACK to trigger handle_stack_overflow().
+        */
+       if (size >= stack_left)
+               *(stack_info.begin - 1) = 42;
+#else

On arm64, this won't trigger our stack overflow handler, unless the SP is already very close to the boundary.

Please just use BUG(). If there is an issue on x86, it would be good to solve that in the x86 code.

Quoted 2 lines

        BUG_ON(stack_left < MIN_STACK_LEFT ||
                                size >= stack_left - MIN_STACK_LEFT);

I really don't think we should bother with this arbitrary offset at all.

It seems that previously I was very "lucky" to accidentally have those MIN_STACK_LEFT, call trace depth and oops=panic together to experience a hang on stack overflow during BUG().

When I run my test in a loop _without_ VMAP_STACK, I manage to corrupt the neighbour processes with BUG() handling overstepping the stack boundary. It's a pity, but I have an idea.

In kernel/sched/core.c we already have:

#ifdef CONFIG_SCHED_STACK_END_CHECK
if (task_stack_end_corrupted(prev))

		panic("corrupted stack end detected inside scheduler\n");

#endif

So what would you think if I do the following in check_alloca():

	if (size >= stack_left) {
#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
		panic("alloca over the kernel stack boundary\n");
#else
		BUG();

#endif

Quoted 5 lines · Alexander Popov

It seems that previously I was very "lucky" to accidentally have those MIN_STACK_LEFT, call trace depth and oops=panic together to experience a hang on stack overflow during BUG().

Quoted 3 lines

When I run my test in a loop _without_ VMAP_STACK, I manage to corrupt the neighbour processes with BUG() handling overstepping the stack boundary. It's a pity, but I have an idea.

I think that in the absence of VMAP_STACK, there will always be cases where we *could* corrupt a neighbouring stack, but I agree that trying to minimize that possibility would be good.

Quoted 7 lines

In kernel/sched/core.c we already have:

#ifdef CONFIG_SCHED_STACK_END_CHECK
if (task_stack_end_corrupted(prev))

		panic("corrupted stack end detected inside scheduler\n");

#endif

Quoted 8 lines

So what would you think if I do the following in check_alloca():

	if (size >= stack_left) {
#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
		panic("alloca over the kernel stack boundary\n");
#else
		BUG();

#endif

Given this is already out-of-line, how about we always use panic(), regardless of VMAP_STACK and SCHED_STACK_END_CHECK? i.e. just

	if (unlikely(size >= stack_left))
		panic("alloca over the kernel stack boundary");

If we have VMAP_STACK selected, and overflow during the panic, it's the same as if we overflowed during the BUG(). It's likely that panic() will use less stack space than BUG(), and the compiler can put the call in a slow path that shouldn't affect most calls, so in all cases it's likely preferable.

Quoted 13 lines · Mark Rutland

Quoted 8 lines · Alexander Popov

So what would you think if I do the following in check_alloca():

	if (size >= stack_left) {
#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
		panic("alloca over the kernel stack boundary\n");
#else
		BUG();

#endif

Given this is already out-of-line, how about we always use panic(), regardless of VMAP_STACK and SCHED_STACK_END_CHECK? i.e. just

Quoted 7 lines

	if (unlikely(size >= stack_left))
		panic("alloca over the kernel stack boundary");

If we have VMAP_STACK selected, and overflow during the panic, it's the same as if we overflowed during the BUG(). It's likely that panic() will use less stack space than BUG(), and the compiler can put the call in a slow path that shouldn't affect most calls, so in all cases it's likely preferable.

I'm sure that maintainers and Linus will strongly dislike my patch if I always use panic() here. panic() kills the whole kernel and we shouldn't use it when we can safely continue to work.

Let me describe my logic. So let's have size >= stack_left on a thread stack.

1. If CONFIG_VMAP_STACK is enabled, we can safely use BUG(). Even if BUG() handling overflows the thread stack into the guard page, handle_stack_overflow() is called and the neighbour memory is not corrupted. The kernel can proceed to live.

2. If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt the neighbour kernel memory and cause the undefined behaviour of the whole kernel. I see it on my lkdtm test. That is a cogent reason for panic().

2.a. If CONFIG_SCHED_STACK_END_CHECK is enabled, the kernel already does panic() when STACK_END_MAGIC is corrupted. So we will _not_ break the safety policy if we do panic() in a similar situation in check_alloca().

2.b. If CONFIG_SCHED_STACK_END_CHECK is disabled, the user has some real reasons not to do panic() when the kernel stack is corrupted.

So we should not do it in check_alloca() as well, just use BUG() and hope for the best.

Quoted 26 lines · Alexander Popov

Quoted 13 lines · Mark Rutland

Quoted 8 lines · Alexander Popov

So what would you think if I do the following in check_alloca():

	if (size >= stack_left) {
#if !defined(CONFIG_VMAP_STACK) && defined(CONFIG_SCHED_STACK_END_CHECK)
		panic("alloca over the kernel stack boundary\n");
#else
		BUG();

#endif

Given this is already out-of-line, how about we always use panic(), regardless of VMAP_STACK and SCHED_STACK_END_CHECK? i.e. just

Quoted 7 lines

	if (unlikely(size >= stack_left))
		panic("alloca over the kernel stack boundary");

If we have VMAP_STACK selected, and overflow during the panic, it's the same as if we overflowed during the BUG(). It's likely that panic() will use less stack space than BUG(), and the compiler can put the call in a slow path that shouldn't affect most calls, so in all cases it's likely preferable.

I'm sure that maintainers and Linus will strongly dislike my patch if I always use panic() here. panic() kills the whole kernel and we shouldn't use it when we can safely continue to work.

Quoted 5 lines

Let me describe my logic. So let's have size >= stack_left on a thread stack.

1. If CONFIG_VMAP_STACK is enabled, we can safely use BUG(). Even if BUG() handling overflows the thread stack into the guard page, handle_stack_overflow() is called and the neighbour memory is not corrupted. The kernel can proceed to live.

On arm64 with CONFIG_VMAP_STACK, a stack overflow will result in a panic(). My understanding was that the same is true on x86.

Quoted 3 lines

2. If CONFIG_VMAP_STACK is disabled, BUG() handling can corrupt the neighbour kernel memory and cause the undefined behaviour of the whole kernel. I see it on my lkdtm test. That is a cogent reason for panic().

In this case, panic() can also corrupt the neighbour stack, and could also fail.

When CONFIG_VMAP_STACK is not selected, a stack overflow simply cannot be handled reliably -- while panic() may be more likely to succeed, it is not gauranteed to.

Quoted 3 lines

2.a. If CONFIG_SCHED_STACK_END_CHECK is enabled, the kernel already does panic() when STACK_END_MAGIC is corrupted. So we will _not_ break the safety policy if we do panic() in a similar situation in check_alloca().

Sure, I'm certainly happy with panic() here.

Quoted 2 lines

2.b. If CONFIG_SCHED_STACK_END_CHECK is disabled, the user has some real reasons not to do panic() when the kernel stack is corrupted.

I believe that CONFIG_SCHED_STACK_END_CHECK is seen as a debug feature, and hence people don't select it.

I strongly doubt that people have
reasons to disable it other than not wanting the overhead associated with debug features.

I think it is reasonable to panic() here even with CONFIG_VMAP_STACK selected.

Quoted 2 lines

So we should not do it in check_alloca() as well, just use BUG() and hope for the best.

Regardless of whether we BUG() or panic(), we're hoping for the best.

Consistently using panic() here will keep things simpler, so any failure reported will be easier to reason about, and easier to debug.

Quoted 2 lines · Mark Rutland

I think it is reasonable to panic() here even with CONFIG_VMAP_STACK selected.

It's too tough for CONFIG_VMAP_STACK on x86 - the system can proceed to live. Anyway, the check_alloca() code will not be shared between x86 and arm64, I've described the reasons in this thread. So I can have BUG() for CONFIG_VMAP_STACK on x86 and Laura can consistently use panic() on arm64.

Quoted 5 lines

Quoted 2 lines

So we should not do it in check_alloca() as well, just use BUG() and hope for the best.

Regardless of whether we BUG() or panic(), we're hoping for the best.

Quoted 2 lines

Consistently using panic() here will keep things simpler, so any failure reported will be easier to reason about, and easier to debug.

Let me keep BUG() for !CONFIG_SCHED_STACK_END_CHECK. I beware of using panic() by default, let distro/user decide this. I remember very well how I was shouted at, when this one was merged:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ce6fa91b93630396ca220c33dd38ffc62686d499

Hi Laura,

Quoted 6 lines · Laura Abbott

Implementation of stackleak based heavily on the x86 version

Signed-off-by Laura Abbott <[email protected]>

---
Now written in C instead of a bunch of assembly.

This looks neat!

I have a few minor comments below.

arch/arm64/kernel/Makefile arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o · line 59

Quoted 10 lines

arch/arm64/kernel/Makefile +2 -0

index bf825f38d206..0ceea613c65b 100644
@@ -55,6 +55,9 @@ arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o
  arm64-obj-$(CONFIG_CRASH_DUMP)		+= crash_dump.o
  arm64-obj-$(CONFIG_ARM_SDE_INTERFACE)	+= sdei.o
 
+arm64-obj-$(CONFIG_GCC_PLUGIN_STACKLEAK) += erase.o
+KASAN_SANITIZE_erase.o	:= n

I suspect we want to avoid the full set of instrumentation suspects here, e.g. GKOV, KASAN, UBSAN, and KCOV.

Quoted 9 lines

arch/arm64/kernel/Makefile +1 -0

+
  obj-y					+= $(arm64-obj-y) vdso/ probes/
  obj-m					+= $(arm64-obj-m)
  head-y					:= head.o

arch/arm64/kernel/entry.S +0 -0

index ec2ee720e33e..3144f1ebdc18 100644

arch/arm64/kernel/entry.S tsk .req x28 // current thread_info · line 408

Quoted 8 lines

arch/arm64/kernel/entry.S +5 -0

@@ -401,6 +401,11 @@ tsk	.req	x28		// current thread_info
 
  	.text
 
+	.macro	ERASE_KSTACK
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	bl	erase_kstack
+#endif
+	.endm

Nit: The rest of our asm macros are lower-case -- can we stick to that here?

arch/arm64/kernel/entry.S ret_to_user: · line 916

Quoted 10 lines

arch/arm64/kernel/entry.S +1 -0

  /*
   * Exception vectors.
   */

@@ -906,6 +911,7 @@ ret_to_user:
  	cbnz	x2, work_pending
  finish_ret_to_user:
  	enable_step_tsk x1, x2
+	ERASE_KSTACK
  	kernel_exit 0
  ENDPROC(ret_to_user)

I believe we also need this in ret_fast_syscall.

[...]

Quoted 17 lines

arch/arm64/kernel/entry.S +17 -0

+asmlinkage void erase_kstack(void)
+{
+	unsigned long p = current->thread.lowest_stack;
+	unsigned long boundary = p & ~(THREAD_SIZE - 1);
+	unsigned long poison = 0;
+	const unsigned long check_depth = STACKLEAK_POISON_CHECK_DEPTH /
+							sizeof(unsigned long);
+
+	/*
+	 * Let's search for the poison value in the stack.
+	 * Start from the lowest_stack and go to the bottom.
+	 */
+	while (p > boundary && poison <= check_depth) {
+		if (*(unsigned long *)p == STACKLEAK_POISON)
+			poison++;
+		else
+			poison = 0;

arch/arm64/kernel/entry.S

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+		p -= sizeof(unsigned long);
+	}
+
+	/*
+	 * One long int at the bottom of the thread stack is reserved and
+	 * should not be poisoned (see CONFIG_SCHED_STACK_END_CHECK).
+	 */
+	if (p == boundary)
+		p += sizeof(unsigned long);

I wonder if end_of_stack() should be taught about CONFIG_SCHED_STACK_END_CHECK, given that's supposed to return the last *usable* long on the stack, and we don't account for this elsewhere.

If we did, then IIUC we could do:

	unsigned long boundary = (unsigned long)end_of_stack(current);

... at the start of the function, and not have to worry about this explicitly.

arch/arm64/kernel/entry.S

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+#ifdef CONFIG_STACKLEAK_METRICS
+	current->thread.prev_lowest_stack = p;
+#endif
+
+	/*
+	 * So let's write the poison value to the kernel stack.
+	 * Start from the address in p and move up till the new boundary.
+	 */
+	boundary = current_stack_pointer;

I worry a little that the compiler can move the SP during a function's lifetime, but maybe that's only the case when there are VLAs, or something like that?

arch/arm64/kernel/entry.S

Quoted 11 lines

arch/arm64/kernel/entry.S +11 -0

+
+	BUG_ON(boundary - p >= THREAD_SIZE);
+
+	while (p < boundary) {
+		*(unsigned long *)p = STACKLEAK_POISON;
+		p += sizeof(unsigned long);
+	}
+
+	/* Reset the lowest_stack value for the next syscall */
+	current->thread.lowest_stack = current_stack_pointer;
+}

Once this function returns, its data is left on the stack. Is that not a problem?

No strong feelings either way, but it might be worth mentioning in the commit message.

arch/arm64/kernel/process.c int copy_thread(unsigned long clone_flags, unsigned long stack_start, · line 368

Quoted 10 lines

arch/arm64/kernel/process.c +2 -0

index f08a2ed9db0d..156fa0a0da19 100644
@@ -364,6 +364,9 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
  	p->thread.cpu_context.pc = (unsigned long)ret_from_fork;
  	p->thread.cpu_context.sp = (unsigned long)childregs;
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+	p->thread.lowest_stack = (unsigned long)task_stack_page(p);

Nit: end_of_stack(p) would be slightly better semantically, even though currently equivalent to task_stack_page(p).

[...]

arch/arm64/kernel/process.c void arch_setup_new_exec(void) · line 509

Quoted 10 lines

arch/arm64/kernel/process.c +10 -0

+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+void __used check_alloca(unsigned long size)
+{
+	unsigned long sp, stack_left;
+
+	sp = current_stack_pointer;
+
+	stack_left = sp & (THREAD_SIZE - 1);
+	BUG_ON(stack_left < 256 || size >= stack_left - 256);
+}

Is this arbitrary, or is there something special about 256?

Even if this is arbitrary, can we give it some mnemonic?

Quoted 14 lines

arch/arm64/kernel/process.c +2 -0

+EXPORT_SYMBOL(check_alloca);
+#endif

drivers/firmware/efi/libstub/Makefile +2 -1

index a34e9290a699..25dd2a14560d 100644
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)	+= -I$(srctree)/scripts/dtc/libfdt
  KBUILD_CFLAGS			:= $(cflags-y) -DDISABLE_BRANCH_PROFILING \
  				   -D__NO_FORTIFY \
  				   $(call cc-option,-ffreestanding) \
-				   $(call cc-option,-fno-stack-protector)
+				   $(call cc-option,-fno-stack-protector) \
+				   $(DISABLE_STACKLEAK_PLUGIN)
 

drivers/firmware/efi/libstub/Makefile cflags-$(CONFIG_EFI_ARMSTUB) += -I$(srctree)/scripts/dtc/libfdt · line 27

Quoted 2 lines

drivers/firmware/efi/libstub/Makefile +0 -0

  GCOV_PROFILE			:= n
  KASAN_SANITIZE			:= n

I believe we'll also need to do this for the KVM hyp code in arch/arm64/kvm/hyp/.

Thanks,
Mark.

Quoted 5 lines · Mark Rutland

Quoted 2 lines · Laura Abbott

arch/arm64/kernel/erase.c new file +2 -0

+asmlinkage void erase_kstack(void)
+{

Quoted 10 lines

arch/arm64/kernel/erase.c line 43

Quoted 6 lines

arch/arm64/kernel/erase.c new file +6 -0

+
+	/*
+	 * So let's write the poison value to the kernel stack.
+	 * Start from the address in p and move up till the new boundary.
+	 */
+	boundary = current_stack_pointer;

I worry a little that the compiler can move the SP during a function's lifetime, but maybe that's only the case when there are VLAs, or something like that?

I think that's true and a risk we take writing this in C. Here's the disassembly on gcc-7.3.1:

ffff00000809d4d8 <erase_kstack>:

ffff00000809d4d8:       a9bf7bfd        stp     x29, x30, [sp, #-16]!
ffff00000809d4dc:       d5384100        mrs     x0, sp_el0
ffff00000809d4e0:       910003fd        mov     x29, sp
ffff00000809d4e4:       f946e400        ldr     x0, [x0, #3528]
ffff00000809d4e8:       9272c404        and     x4, x0, #0xffffffffffffc000
ffff00000809d4ec:       eb04001f        cmp     x0, x4
ffff00000809d4f0:       540002c9        b.ls    ffff00000809d548 <erase_kstack+0x70>  // b.plast
ffff00000809d4f4:       d2800003        mov     x3, #0x0                        // #0
ffff00000809d4f8:       9297ddc5        mov     x5, #0xffffffffffff4111         // #-48879
ffff00000809d4fc:       14000008        b       ffff00000809d51c <erase_kstack+0x44>
ffff00000809d500:       d1002000        sub     x0, x0, #0x8
ffff00000809d504:       52800022        mov     w2, #0x1                        // #1
ffff00000809d508:       eb00009f        cmp     x4, x0
ffff00000809d50c:       d2800003        mov     x3, #0x0                        // #0
ffff00000809d510:       1a9f27e1        cset    w1, cc  // cc = lo, ul, last
ffff00000809d514:       6a01005f        tst     w2, w1
ffff00000809d518:       54000180        b.eq    ffff00000809d548 <erase_kstack+0x70>  // b.none
ffff00000809d51c:       f9400001        ldr     x1, [x0]
ffff00000809d520:       eb05003f        cmp     x1, x5
ffff00000809d524:       54fffee1        b.ne    ffff00000809d500 <erase_kstack+0x28>  // b.any
ffff00000809d528:       91000463        add     x3, x3, #0x1
ffff00000809d52c:       d1002000        sub     x0, x0, #0x8
ffff00000809d530:       f100407f        cmp     x3, #0x10
ffff00000809d534:       1a9f87e2        cset    w2, ls  // ls = plast
ffff00000809d538:       eb00009f        cmp     x4, x0
ffff00000809d53c:       1a9f27e1        cset    w1, cc  // cc = lo, ul, last
ffff00000809d540:       6a01005f        tst     w2, w1
ffff00000809d544:       54fffec1        b.ne    ffff00000809d51c <erase_kstack+0x44>  // b.any
ffff00000809d548:       eb00009f        cmp     x4, x0
ffff00000809d54c:       91002001        add     x1, x0, #0x8
ffff00000809d550:       9a800020        csel    x0, x1, x0, eq  // eq = none
ffff00000809d554:       910003e1        mov     x1, sp
ffff00000809d558:       d5384102        mrs     x2, sp_el0
ffff00000809d55c:       f906e840        str     x0, [x2, #3536]
ffff00000809d560:       cb000023        sub     x3, x1, x0
ffff00000809d564:       d287ffe2        mov     x2, #0x3fff                     // #16383
ffff00000809d568:       eb02007f        cmp     x3, x2
ffff00000809d56c:       540001a8        b.hi    ffff00000809d5a0 <erase_kstack+0xc8>  // b.pmore
ffff00000809d570:       9297ddc2        mov     x2, #0xffffffffffff4111         // #-48879
ffff00000809d574:       eb01001f        cmp     x0, x1
ffff00000809d578:       54000082        b.cs    ffff00000809d588 <erase_kstack+0xb0>  // b.hs, b.nlast
ffff00000809d57c:       f8008402        str     x2, [x0], #8
ffff00000809d580:       eb00003f        cmp     x1, x0
ffff00000809d584:       54ffffc8        b.hi    ffff00000809d57c <erase_kstack+0xa4>  // b.pmore
ffff00000809d588:       910003e1        mov     x1, sp
ffff00000809d58c:       d5384100        mrs     x0, sp_el0
ffff00000809d590:       f906e401        str     x1, [x0, #3528]
ffff00000809d594:       a8c17bfd        ldp     x29, x30, [sp], #16
ffff00000809d598:       d65f03c0        ret
ffff00000809d59c:       d503201f        nop
ffff00000809d5a0:       d4210000        brk     #0x800
ffff00000809d5a4:       00000000        .inst   0x00000000 ; undefined