[PATCH 0/8] stackleak: fixes and rework

[PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

This series reworks the stackleak code. The first patch fixes some
latent issues on arm64, and the subsequent patches improve the code to
improve clarity and permit better code generation.

I started working on this as a tangent from rework to arm64's stacktrace
code. Looking at users of the `on_*_stack()` helpers I noticed that the
assembly generated for stackleak was particularly awful as it performed
a lot of redundant work and also called instrumentable code, which isn't
sound.

The first patch fixes the major issues on arm64, and is Cc'd to stable
for backporting.

The second patch is a trivial optimization for when stackleak is
dynamically disabled.

The subsequent patches rework the way stackleak manipulates the stack
boundary values. This is partically for clarity (e.g. with separate
'low' and 'high' boundary variables), and also permits the compiler to
generate more optimal assembly by generating the high and low bounds
from the same base.

Patch 5 changes the way that `current->lowest_stack` is reset prior to
return to userspace. The existing code uses an undocumented offset
relative to the top of the stack which doesn't make much sense (as thie
sometimes falls within the task's pt_regs, or sometimes adds 600+ bytes
to erase upon the next exit to userspace). For now I've removed the
offset entirely.

Patch 7 adds stackleak_erase_on_task_stack() and
stackleak_erase_off_task_stack() that can be used when a caller knows
they're always on or off the task stack respectively, avoiding redundant
logic to check this and generate the high boundary value. On arm64 we
always call stackleak_erase() while on the task stack, so this is used
in patch 8.

Testing the series on arm64 with a QEMU HVF VM on an M1 Macbook Pro with
a few microbenchmarks shows a small but measureable improvement when
stackleak is enabled (relative to v5.18-rc1):

* Calling getpid 1^22 times in a loop (avg 50 runs)
  
  Before: 0.652099387 seconds ( +-  0.13% )
  After:  0.641005661 seconds ( +-  0.13% )

  ~1.7% time decrease

* perf bench sched pipe (single run)

  Before: 2.138 seconds total
  After:  2.118 seconds total

  ~0.93% time decrease

I also tested "perf bench sched messaging" but the noise outweighed the
difference.

While the improvement is small, I think the improvement to clarity and
code generation is a win regardless.

Thanks,
Mark.

Mark Rutland (8):
  arm64: stackleak: fix current_top_of_stack()
  stackleak: move skip_erasing() check earlier
  stackleak: rework stack low bound handling
  stackleak: clarify variable names
  stackleak: rework stack high bound handling
  stackleak: remove redundant check
  stackleak: add on/off stack variants
  arm64: entry: use stackleak_erase_on_task_stack()

 arch/arm64/include/asm/processor.h | 10 ++-
 arch/arm64/kernel/entry.S          |  2 +-
 include/linux/stackleak.h          | 29 ++++++++-
 kernel/stackleak.c                 | 99 ++++++++++++++++++++----------
 4 files changed, 98 insertions(+), 42 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] arm64: stackleak: fix current_top_of_stack()

[Mark Rutland]

Due to some historical confusion, arm64's current_top_of_stack() isn't
what the stackleak code expects. This could in theory result in a number
of problems, and practically results in an unnecessary performance hit.
We can avoid this by aligning the arm64 implementation with the x86
implementation.

The arm64 implementation of current_top_of_stack() was added
specifically for stackleak in commit:

  0b3e336601b82c6a ("arm64: Add support for STACKLEAK gcc plugin")

This was intended to be equivalent to the x86 implementation, but the
implementation, semantics, and performance characteristics differ
wildly:

* On x86, current_top_of_stack() returns the top of the current task's
  task stack, regardless of which stack is in active use.

  The implementation accesses a percpu variable which the x86 entry code
  maintains, and returns the location immediately above the pt_regs on
  the task stack (above which x86 has some padding).

* On arm64 current_top_of_stack() returns the top of the stack in active
  use (i.e. the one which is currently being used).

  The implementation checks the SP against a number of
  potentially-accessible stacks, and will BUG() if no stack is found.

The core stackleak_erase() code determines the upper bound of stack to
erase with:

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

On arm64 stackleak_erase() is always called on a task stack, and
on_thread_stack() should always be true. On x86, stackleak_erase() is
mostly called on a trampoline stack, and is sometimes called on a task
stack.

Currently, this results in a lot of unnecessary code being generated for
arm64 for the impossible !on_thread_stack() case. Some of this is
inlined, bloating stackleak_erase(), while portions of this are left
out-of-line and permitted to be instrumented (which would be a
functional problem if that code were reachable).

As a first step towards improving this, this patch aligns arm64's
implementation of current_top_of_stack() with x86's, always returning
the top of the current task's stack. With GCC 11.1.0 this results in the
bulk of the unnecessary code being removed, including all of the
out-of-line instrumentable code.

While I don't believe there's a functional problem in practice I've
marked this as a fix since the semantic was clearly wrong, the fix
itself is simple, and other code might rely upon this in future.

Fixes: 0b3e336601b82c6a ("arm64: Add support for STACKLEAK gcc plugin")
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/processor.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 73e38d9a540ce..6b1a12c23fe77 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -381,12 +381,10 @@ long get_tagged_addr_ctrl(struct task_struct *task);
  * of header definitions for the use of task_stack_page.
  */
 
-#define current_top_of_stack()								\
-({											\
-	struct stack_info _info;							\
-	BUG_ON(!on_accessible_stack(current, current_stack_pointer, 1, &_info));	\
-	_info.high;									\
-})
+/*
+ * The top of the current task's task stack
+ */
+#define current_top_of_stack()	((unsigned long)current->stack + THREAD_SIZE)
 #define on_thread_stack()	(on_task_stack(current, current_stack_pointer, 1, NULL))
 
 #endif /* __ASSEMBLY__ */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] stackleak: move skip_erasing() check earlier

[Mark Rutland]

In stackleak_erase() we check skip_erasing() after accessing some fields
from current. As generating the address of current uses asm which
hazards with the static branch asm, this work is always performed, even
when the static branch is patched to jump to the return a the end of the
function.

This patch avoids this redundant work by moving the skip_erasing() check
earlier.

To avoid complicating initialization within stackleak_erase(), the body
of the function is split out into a __stackleak_erase() helper, with the
check left in a wrapper function. The __stackleak_erase() helper is
marked __always_inline to ensure that this is inlined into
stackleak_erase() and not instrumented.

Before this patch, on x86-64 w/ GCC 11.1.0 the start of the function is:

<stackleak_erase>:
   65 48 8b 04 25 00 00    mov    %gs:0x0,%rax
   00 00
   48 8b 48 20             mov    0x20(%rax),%rcx
   48 8b 80 98 0a 00 00    mov    0xa98(%rax),%rax
   66 90                   xchg   %ax,%ax  <------------ static branch
   48 89 c2                mov    %rax,%rdx
   48 29 ca                sub    %rcx,%rdx
   48 81 fa ff 3f 00 00    cmp    $0x3fff,%rdx

After this patch, on x86-64 w/ GCC 11.1.0 the start of the function is:

<stackleak_erase>:
   0f 1f 44 00 00          nopl   0x0(%rax,%rax,1)  <--- static branch
   65 48 8b 04 25 00 00    mov    %gs:0x0,%rax
   00 00
   48 8b 48 20             mov    0x20(%rax),%rcx
   48 8b 80 98 0a 00 00    mov    0xa98(%rax),%rax
   48 89 c2                mov    %rax,%rdx
   48 29 ca                sub    %rcx,%rdx
   48 81 fa ff 3f 00 00    cmp    $0x3fff,%rdx

Before this patch, on arm64 w/ GCC 11.1.0 the start of the function is:

<stackleak_erase>:
   d503245f        bti     c
   d5384100        mrs     x0, sp_el0
   f9401003        ldr     x3, [x0, #32]
   f9451000        ldr     x0, [x0, #2592]
   d503201f        nop  <------------------------------- static branch
   d503233f        paciasp
   cb030002        sub     x2, x0, x3
   d287ffe1        mov     x1, #0x3fff
   eb01005f        cmp     x2, x1

After this patch, on arm64 w/ GCC 11.1.0 the start of the function is:

<stackleak_erase>:
   d503245f        bti     c
   d503201f        nop  <------------------------------- static branch
   d503233f        paciasp
   d5384100        mrs     x0, sp_el0
   f9401003        ldr     x3, [x0, #32]
   d287ffe1        mov     x1, #0x3fff
   f9451000        ldr     x0, [x0, #2592]
   cb030002        sub     x2, x0, x3
   eb01005f        cmp     x2, x1

While this may not be a huge win on its own, moving the static branch
will permit further optimization of the body of the function in
subsequent patches.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/stackleak.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index ddb5a7f48d69e..753eab797a04d 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -70,7 +70,7 @@ late_initcall(stackleak_sysctls_init);
 #define skip_erasing()	false
 #endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */
 
-asmlinkage void noinstr stackleak_erase(void)
+static __always_inline void __stackleak_erase(void)
 {
 	/* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */
 	unsigned long kstack_ptr = current->lowest_stack;
@@ -78,9 +78,6 @@ asmlinkage void noinstr stackleak_erase(void)
 	unsigned int poison_count = 0;
 	const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
 
-	if (skip_erasing())
-		return;
-
 	/* Check that 'lowest_stack' value is sane */
 	if (unlikely(kstack_ptr - boundary >= THREAD_SIZE))
 		kstack_ptr = boundary;
@@ -125,6 +122,14 @@ asmlinkage void noinstr stackleak_erase(void)
 	current->lowest_stack = current_top_of_stack() - THREAD_SIZE/64;
 }
 
+asmlinkage void noinstr stackleak_erase(void)
+{
+	if (skip_erasing())
+		return;
+
+	__stackleak_erase();
+}
+
 void __used __no_caller_saved_registers noinstr stackleak_track_stack(void)
 {
 	unsigned long sp = current_stack_pointer;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] stackleak: rework stack low bound handling

[Mark Rutland]

In stackleak_task_init(), stackleak_track_stack(), and
__stackleak_erase(), we open-code skipping the STACK_END_MAGIC at the
bottom of the stack. Each case is implemented slightly differently, and
only the __stackleak_erase() case is commented.

In stackleak_task_init() and stackleak_track_stack() we unconditionally
add sizeof(unsigned long) to the lowest stack address. In
stackleak_task_init() we use end_of_stack() for this, and in
stackleak_track_stack() we use task_stack_page(). In __stackleak_erase()
we handle this by detecting if `kstack_ptr` has hit the stack end
boundary, and if so, conditionally moving it above the magic.

This patch adds a new stackleak_task_low_bound() helper which is used in
all three cases, which unconditionally adds sizeof(unsigned long) to the
lowest address on the task stack, with commentary as to why. This uses
end_of_stack() as stackleak_task_init() did prior to this patch, as this
is consistent with the code in kernel/fork.c which initializes the
STACK_END_MAGIC value.

In __stackleak_erase() we no longer need to check whether we've spilled
into the STACK_END_MAGIC value, as stackleak_track_stack() ensures that
`current->lowest_stack` stops immediately above this, and similarly the
poison scan will stop immediately above this.

For stackleak_task_init() and stackleak_track_stack() this results in no
change to code generation. For __stackleak_erase() the generated
assembly is slightly simpler and shorter.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 include/linux/stackleak.h | 15 ++++++++++++++-
 kernel/stackleak.c        | 14 ++++----------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
index ccaab2043fcd5..67430faa5c518 100644
--- a/include/linux/stackleak.h
+++ b/include/linux/stackleak.h
@@ -15,9 +15,22 @@
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
 #include <asm/stacktrace.h>
 
+/*
+ * The lowest address on tsk's stack which we can plausibly erase.
+ */
+static __always_inline unsigned long
+stackleak_task_low_bound(const struct task_struct *tsk)
+{
+	/*
+	 * The lowest unsigned long on the task stack contains STACK_END_MAGIC,
+	 * which we must not corrupt.
+	 */
+	return (unsigned long)end_of_stack(tsk) + sizeof(unsigned long);
+}
+
 static inline void stackleak_task_init(struct task_struct *t)
 {
-	t->lowest_stack = (unsigned long)end_of_stack(t) + sizeof(unsigned long);
+	t->lowest_stack = stackleak_task_low_bound(t);
 # ifdef CONFIG_STACKLEAK_METRICS
 	t->prev_lowest_stack = t->lowest_stack;
 # endif
diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index 753eab797a04d..0472956d9a2ce 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -72,9 +72,11 @@ late_initcall(stackleak_sysctls_init);
 
 static __always_inline void __stackleak_erase(void)
 {
+	const unsigned long task_stack_low = stackleak_task_low_bound(current);
+
 	/* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */
 	unsigned long kstack_ptr = current->lowest_stack;
-	unsigned long boundary = (unsigned long)end_of_stack(current);
+	unsigned long boundary = task_stack_low;
 	unsigned int poison_count = 0;
 	const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
 
@@ -92,13 +94,6 @@ static __always_inline void __stackleak_erase(void)
 		kstack_ptr -= 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=y).
-	 */
-	if (kstack_ptr == boundary)
-		kstack_ptr += sizeof(unsigned long);
-
 #ifdef CONFIG_STACKLEAK_METRICS
 	current->prev_lowest_stack = kstack_ptr;
 #endif
@@ -144,8 +139,7 @@ void __used __no_caller_saved_registers noinstr stackleak_track_stack(void)
 	/* 'lowest_stack' should be aligned on the register width boundary */
 	sp = ALIGN(sp, sizeof(unsigned long));
 	if (sp < current->lowest_stack &&
-	    sp >= (unsigned long)task_stack_page(current) +
-						sizeof(unsigned long)) {
+	    sp >= stackleak_task_low_bound(current)) {
 		current->lowest_stack = sp;
 	}
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] stackleak: clarify variable names

[Mark Rutland]

The logic within __stackleak_erase() can be a little hard to follow, as
`boundary` switches from being the low bound to the high bound mid way
through the function, and `kstack_ptr` is used to represent the start of
the region to erase while `boundary` represents the end of the region to
erase.

Make this a little clearer by consistently using clearer variable names.
The `boundary` variable is removed, the bounds of the region to erase
are described by `erase_low` and `erase_high`, and bounds of the task
stack are described by `task_stack_low` and `task_stck_high`.

As the same time, remove the comment above the variables, since it is
unclear whether it's intended as rationale, a complaint, or a TODO, and
is more confusing than helpful.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/stackleak.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index 0472956d9a2ce..8fbc1e4c21435 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -73,44 +73,42 @@ late_initcall(stackleak_sysctls_init);
 static __always_inline void __stackleak_erase(void)
 {
 	const unsigned long task_stack_low = stackleak_task_low_bound(current);
-
-	/* It would be nice not to have 'kstack_ptr' and 'boundary' on stack */
-	unsigned long kstack_ptr = current->lowest_stack;
-	unsigned long boundary = task_stack_low;
+	unsigned long erase_low = current->lowest_stack;
+	unsigned long erase_high;
 	unsigned int poison_count = 0;
 	const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
 
 	/* Check that 'lowest_stack' value is sane */
-	if (unlikely(kstack_ptr - boundary >= THREAD_SIZE))
-		kstack_ptr = boundary;
+	if (unlikely(erase_low - task_stack_low >= THREAD_SIZE))
+		erase_low = task_stack_low;
 
 	/* Search for the poison value in the kernel stack */
-	while (kstack_ptr > boundary && poison_count <= depth) {
-		if (*(unsigned long *)kstack_ptr == STACKLEAK_POISON)
+	while (erase_low > task_stack_low && poison_count <= depth) {
+		if (*(unsigned long *)erase_low == STACKLEAK_POISON)
 			poison_count++;
 		else
 			poison_count = 0;
 
-		kstack_ptr -= sizeof(unsigned long);
+		erase_low -= sizeof(unsigned long);
 	}
 
 #ifdef CONFIG_STACKLEAK_METRICS
-	current->prev_lowest_stack = kstack_ptr;
+	current->prev_lowest_stack = erase_low;
 #endif
 
 	/*
-	 * Now write the poison value to the kernel stack. Start from
-	 * 'kstack_ptr' and move up till the new 'boundary'. We assume that
-	 * the stack pointer doesn't change when we write poison.
+	 * Now write the poison value to the kernel stack between 'erase_low'
+	 * and 'erase_high'. We assume that the stack pointer doesn't change
+	 * when we write poison.
 	 */
 	if (on_thread_stack())
-		boundary = current_stack_pointer;
+		erase_high = current_stack_pointer;
 	else
-		boundary = current_top_of_stack();
+		erase_high = current_top_of_stack();
 
-	while (kstack_ptr < boundary) {
-		*(unsigned long *)kstack_ptr = STACKLEAK_POISON;
-		kstack_ptr += sizeof(unsigned long);
+	while (erase_low < erase_high) {
+		*(unsigned long *)erase_low = STACKLEAK_POISON;
+		erase_low += sizeof(unsigned long);
 	}
 
 	/* Reset the 'lowest_stack' value for the next syscall */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] stackleak: rework stack high bound handling

[Mark Rutland]

Prior to returning to userpace, we reset current->lowest_stack to a
reasonable high bound. Currently we do this by subtracting the arbitrary
value `THREAD_SIZE/64` from the top of the stack, for reasons lost to
history.

Looking at configurations today:

* On i386 where THREAD_SIZE is 8K, the bound will be 128 bytes. The
  pt_regs at the top of the stack is 68 bytes (with 0 to 16 bytes of
  padding above), and so this covers an additional portion of 44 to 60
  bytes.

* On x86_64 where THREAD_SIZE is at least 16K (up to 32K with KASAN) the
  bound will be at least 256 bytes (up to 512 with KASAN). The pt_regs
  at the top of the stack is 168 bytes, and so this cover an additional
  88 bytes of stack (up to 344 with KASAN).

* On arm64 where THREAD_SIZE is at least 16K (up to 64K with 64K pages
  and VMAP_STACK), the bound will be at least 256 bytes (up to 1024 with
  KASAN). The pt_regs at the top of the stack is 336 bytes, so this can
  fall within the pt_regs, or can cover an additional 688 bytes of
  stack.

Clearly the `THREAD_SIZE/64` value doesn't make much sense -- in the
worst case, this will cause more than 600 bytes of stack to be erased
for every syscall, even if actual stack usage were substantially
smaller.

This patches makes this slightly less nonsensical by consistently
resetting current->lowest_stack to the base of the task pt_regs. For
clarity and for consistency with the handling of the low bound, the
generation of the high bound is split into a helper with commentary
explaining why.

Since the pt_regs at the top of the stack will be clobbered upon the
next exception entry, we don't need to poison these at exception exit.
By using task_pt_regs() as the high stack boundary instead of
current_top_of_stack() we avoid some redundant poisoning, and the
compiler can share the address generation between the poisoning and
restting of `current->lowest_stack`, making the generated code more
optimal.

It's not clear to me whether the existing `THREAD_SIZE/64` offset was a
dodgy heuristic to skip the pt_regs, or whether it was attempting to
minimize the number of times stackleak_check_stack() would have to
update `current->lowest_stack` when stack usage was shallow at the cost
of unconditionally poisoning a small portion of the stack for every exit
to userspace.

For now I've simply removed the offset, and if we need/want to minimize
updates for shallow stack usage it should be easy to add a better
heuristic atop, with appropriate commentary so we know what's going on.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 include/linux/stackleak.h | 14 ++++++++++++++
 kernel/stackleak.c        | 19 ++++++++++++++-----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
index 67430faa5c518..467661aeb4136 100644
--- a/include/linux/stackleak.h
+++ b/include/linux/stackleak.h
@@ -28,6 +28,20 @@ stackleak_task_low_bound(const struct task_struct *tsk)
 	return (unsigned long)end_of_stack(tsk) + sizeof(unsigned long);
 }
 
+/*
+ * The address immediately after the highest address on tsk's stack which we
+ * can plausibly erase.
+ */
+static __always_inline unsigned long
+stackleak_task_high_bound(const struct task_struct *tsk)
+{
+	/*
+	 * The task's pt_regs lives at the top of the task stack and will be
+	 * overwritten by exception entry, so there's no need to erase them.
+	 */
+	return (unsigned long)task_pt_regs(tsk);
+}
+
 static inline void stackleak_task_init(struct task_struct *t)
 {
 	t->lowest_stack = stackleak_task_low_bound(t);
diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index 8fbc1e4c21435..f597d3323729d 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -73,6 +73,7 @@ late_initcall(stackleak_sysctls_init);
 static __always_inline void __stackleak_erase(void)
 {
 	const unsigned long task_stack_low = stackleak_task_low_bound(current);
+	const unsigned long task_stack_high = stackleak_task_high_bound(current);
 	unsigned long erase_low = current->lowest_stack;
 	unsigned long erase_high;
 	unsigned int poison_count = 0;
@@ -97,14 +98,22 @@ static __always_inline void __stackleak_erase(void)
 #endif
 
 	/*
-	 * Now write the poison value to the kernel stack between 'erase_low'
-	 * and 'erase_high'. We assume that the stack pointer doesn't change
-	 * when we write poison.
+	 * Write poison to the task's stack between 'erase_low' and
+	 * 'erase_high'.
+	 *
+	 * If we're running on a different stack (e.g. an entry trampoline
+	 * stack) we can erase everything below the pt_regs at the top of the
+	 * task stack.
+	 *
+	 * If we're running on the task stack itself, we must not clobber any
+	 * stack used by this function and its caller. We assume that this
+	 * function has a fixed-size stack frame, and the current stack pointer
+	 * doesn't change while we write poison.
 	 */
 	if (on_thread_stack())
 		erase_high = current_stack_pointer;
 	else
-		erase_high = current_top_of_stack();
+		erase_high = task_stack_high;
 
 	while (erase_low < erase_high) {
 		*(unsigned long *)erase_low = STACKLEAK_POISON;
@@ -112,7 +121,7 @@ static __always_inline void __stackleak_erase(void)
 	}
 
 	/* Reset the 'lowest_stack' value for the next syscall */
-	current->lowest_stack = current_top_of_stack() - THREAD_SIZE/64;
+	current->lowest_stack = task_stack_high;
 }
 
 asmlinkage void noinstr stackleak_erase(void)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] stackleak: remove redundant check

[Mark Rutland]

In __stackleak_erase() we check that the `erase_low` value derived from
`current->lowest_stack` is above the lowest legitimate stack pointer
value, but this is already enforced by stackleak_track_stack() when
recording the lowest stack value.

Remove the redundant check.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/stackleak.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index f597d3323729d..ba346d46218f5 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -79,10 +79,6 @@ static __always_inline void __stackleak_erase(void)
 	unsigned int poison_count = 0;
 	const unsigned int depth = STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
 
-	/* Check that 'lowest_stack' value is sane */
-	if (unlikely(erase_low - task_stack_low >= THREAD_SIZE))
-		erase_low = task_stack_low;
-
 	/* Search for the poison value in the kernel stack */
 	while (erase_low > task_stack_low && poison_count <= depth) {
 		if (*(unsigned long *)erase_low == STACKLEAK_POISON)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] stackleak: add on/off stack variants

[Mark Rutland]

The stackleak_erase() code dynamically handles being on a task stack or
another stack. In most cases, this is a fixed property of the caller,
which the caller is aware of, as an architecture might always return
using the task stack, or might always return using a trampoline stack.

This patch adds stackleak_erase_on_task_stack() and
stackleak_erase_off_task_stack() functions which callers can use to
avoid on_thread_stack() check and associated redundant work when the
calling stack is known. The existing stackleak_erase() is retained as a
safe default.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/stackleak.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/kernel/stackleak.c b/kernel/stackleak.c
index ba346d46218f5..4c3f82066d7fb 100644
--- a/kernel/stackleak.c
+++ b/kernel/stackleak.c
@@ -70,7 +70,7 @@ late_initcall(stackleak_sysctls_init);
 #define skip_erasing()	false
 #endif /* CONFIG_STACKLEAK_RUNTIME_DISABLE */
 
-static __always_inline void __stackleak_erase(void)
+static __always_inline void __stackleak_erase(bool on_task_stack)
 {
 	const unsigned long task_stack_low = stackleak_task_low_bound(current);
 	const unsigned long task_stack_high = stackleak_task_high_bound(current);
@@ -106,7 +106,7 @@ static __always_inline void __stackleak_erase(void)
 	 * function has a fixed-size stack frame, and the current stack pointer
 	 * doesn't change while we write poison.
 	 */
-	if (on_thread_stack())
+	if (on_task_stack)
 		erase_high = current_stack_pointer;
 	else
 		erase_high = task_stack_high;
@@ -120,12 +120,41 @@ static __always_inline void __stackleak_erase(void)
 	current->lowest_stack = task_stack_high;
 }
 
+/*
+ * Erase and poison the portion of the task stack used since the last erase.
+ * Can be called from the task stack or an entry stack when the task stack is
+ * no longer in use.
+ */
 asmlinkage void noinstr stackleak_erase(void)
 {
 	if (skip_erasing())
 		return;
 
-	__stackleak_erase();
+	__stackleak_erase(on_thread_stack());
+}
+
+/*
+ * Erase and poison the portion of the task stack used since the last erase.
+ * Can only be called from the task stack.
+ */
+asmlinkage void noinstr stackleak_erase_on_task_stack(void)
+{
+	if (skip_erasing())
+		return;
+
+	__stackleak_erase(true);
+}
+
+/*
+ * Erase and poison the portion of the task stack used since the last erase.
+ * Can only be called from a stack other than the task stack.
+ */
+asmlinkage void noinstr stackleak_erase_off_task_stack(void)
+{
+	if (skip_erasing())
+		return;
+
+	__stackleak_erase(false);
 }
 
 void __used __no_caller_saved_registers noinstr stackleak_track_stack(void)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] arm64: entry: use stackleak_erase_on_task_stack()

[Mark Rutland]

On arm64 we always call stackleak_erase() on a task stack, and never
call it on another stack. We can avoid some redundant work by using
stackleak_erase_on_task_stack(), telling the stackleak code that it's
being called on a task stack.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Popov <alex.popov@linux.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/entry.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ede028dee81b0..5b82b92924005 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -596,7 +596,7 @@ SYM_CODE_START_LOCAL(ret_to_user)
 	ldr	x19, [tsk, #TSK_TI_FLAGS]	// re-check for single-step
 	enable_step_tsk x19, x2
 #ifdef CONFIG_GCC_PLUGIN_STACKLEAK
-	bl	stackleak_erase
+	bl	stackleak_erase_on_task_stack
 #endif
 	kernel_exit 0
 SYM_CODE_END(ret_to_user)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Kees Cook]

On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> This series reworks the stackleak code. The first patch fixes some
> latent issues on arm64, and the subsequent patches improve the code to
> improve clarity and permit better code generation.

This looks nice; thanks! I'll put this through build testing and get it
applied shortly...

> While the improvement is small, I think the improvement to clarity and
> code generation is a win regardless.

Agreed. I also want to manually inspect the resulting memory just to
make sure things didn't accidentally regress. There's also an LKDTM test
for basic functionality.

-Kees

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

On Mon, Apr 25, 2022 at 03:54:00PM -0700, Kees Cook wrote:
> On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > This series reworks the stackleak code. The first patch fixes some
> > latent issues on arm64, and the subsequent patches improve the code to
> > improve clarity and permit better code generation.
> 
> This looks nice; thanks! I'll put this through build testing and get it
> applied shortly...

Thanks!

Patch 1 is liable to conflict with come other stacktrace bits that may go in
for v5.19, so it'd be good if either that could be queued as a fix for
v5.1-rc4, or we'll have to figure out how to deal with conflicts later.

> > While the improvement is small, I think the improvement to clarity and
> > code generation is a win regardless.
> 
> Agreed. I also want to manually inspect the resulting memory just to
> make sure things didn't accidentally regress. There's also an LKDTM test
> for basic functionality.

I assume that's the STACKLEAK_ERASING test?

I gave that a spin, but on arm64 that test is flaky even on baseline v5.18-rc1.
On x86_64 it seems consistent after 100s of runs. I'll go dig into that now. 

FWIW, I'm testing with defconfig + STACKLEAK + STACKLEAK_RUNTIME_DISABLE +
LKDTM, using GCC 11.1.0 from the kernel.org crosstool pages. When running the
test it passes a few times, then fails dramatically:

| # uname -a
| Linux buildroot 5.18.0-rc1 #1 SMP PREEMPT Tue Apr 26 10:38:12 BST 2022 aarch64 GNU/Linux
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   21.899596] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   21.900102] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   21.900752] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   21.901318] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   22.551022] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   22.551625] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   22.552314] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   22.552915] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   23.137457] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   23.138521] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   23.139173] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   23.139787] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   23.601729] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   23.603159] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   23.603982] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   23.604565] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   24.046171] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   24.046525] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   24.046965] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   24.047562] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   24.481889] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   24.482682] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   24.483361] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   24.483994] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   24.930625] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   24.931168] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   24.931914] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   24.932404] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   25.351606] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   25.352181] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   25.352827] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   25.353496] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   25.762500] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   25.762970] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   25.763396] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   25.763789] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   26.157349] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   26.157880] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   26.158381] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   26.158859] lkdtm: OK: the rest of the thread stack is properly erased
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT
| [   26.527798] lkdtm: Performing direct entry STACKLEAK_ERASING
| [   26.528621] lkdtm: checking unused part of the thread stack (15480 bytes)...
| [   26.529451] lkdtm: the erased part begins after 1440 not poisoned bytes
| [   26.530654] lkdtm: FAIL: bad value number 197 in the erased part: 0xffff8000083d3670
| [   26.531246] lkdtm: FAIL: bad value number 198 in the erased part: 0xaea4d638c4322298
| [   26.531760] lkdtm: FAIL: bad value number 199 in the erased part: 0xffff8000083d3670
| [   26.532219] lkdtm: FAIL: bad value number 200 in the erased part: 0xdead000000000122
| [   26.532640] lkdtm: FAIL: bad value number 201 in the erased part: 0x0
| [   26.532991] lkdtm: FAIL: bad value number 202 in the erased part: 0xdead000000000122
| [   26.533412] lkdtm: FAIL: bad value number 203 in the erased part: 0x101
| [   26.533773] lkdtm: FAIL: bad value number 204 in the erased part: 0xffff2f22033d0000
| [   26.535385] lkdtm: FAIL: bad value number 205 in the erased part: 0xffff8000083d3650
| [   26.536150] lkdtm: FAIL: bad value number 206 in the erased part: 0x2fc3d638c4321e2c
| [   26.536798] lkdtm: FAIL: bad value number 207 in the erased part: 0xffffd638c61c3880
| [   26.537487] lkdtm: FAIL: bad value number 208 in the erased part: 0xffff2f227fbd4878
| [   26.538444] lkdtm: FAIL: bad value number 209 in the erased part: 0xffff8000083d3600
| [   26.539094] lkdtm: FAIL: bad value number 210 in the erased part: 0xfd5d638c4311244
| [   26.539736] lkdtm: FAIL: bad value number 211 in the erased part: 0xffffd638c6139a38
| [   26.540383] lkdtm: FAIL: bad value number 212 in the erased part: 0x0
| [   26.540919] lkdtm: FAIL: bad value number 213 in the erased part: 0x0
| [   26.541458] lkdtm: FAIL: bad value number 214 in the erased part: 0x3eb4d638c43111dc
| [   26.542399] lkdtm: FAIL: bad value number 215 in the erased part: 0xfffffcbc880fa8c0
| [   26.543051] lkdtm: FAIL: bad value number 216 in the erased part: 0xffff2f2203ea3100
| [   26.543698] lkdtm: FAIL: bad value number 217 in the erased part: 0xffff2f2202817500
| [   26.544353] lkdtm: FAIL: bad value number 218 in the erased part: 0xe184d638c447df3c
| [   26.545004] lkdtm: FAIL: bad value number 219 in the erased part: 0xffff8000083d3600
| [   26.545652] lkdtm: FAIL: bad value number 220 in the erased part: 0xffff2f22033d0000
| [   26.546571] lkdtm: FAIL: bad value number 221 in the erased part: 0xffff2f227fbd3b80
| [   26.547110] lkdtm: FAIL: bad value number 222 in the erased part: 0xc5d1d638c42cb164
| [   26.547643] lkdtm: FAIL: bad value number 223 in the erased part: 0xffff8000083d35a0
| [   26.548180] lkdtm: FAIL: bad value number 224 in the erased part: 0x2f94d638c42cb150
| [   26.548716] lkdtm: FAIL: bad value number 225 in the erased part: 0xffff8000083d35a0
| [   26.549263] lkdtm: FAIL: bad value number 226 in the erased part: 0xffff2f22033d0000
| [   26.549798] lkdtm: FAIL: bad value number 227 in the erased part: 0xffffd638c61c3880
| [   26.550684] lkdtm: FAIL: bad value number 228 in the erased part: 0x96ccd638c4477ac8
| [   26.551216] lkdtm: FAIL: bad value number 229 in the erased part: 0xffff8000083d35a0
| [   26.551754] lkdtm: FAIL: bad value number 230 in the erased part: 0x99abd638c4499888
| [   26.552289] lkdtm: FAIL: bad value number 231 in the erased part: 0xffff8000083d3580
| [   26.552821] lkdtm: FAIL: bad value number 232 in the erased part: 0x6ccd638c447e0e0
| [   26.553348] lkdtm: FAIL: bad value number 233 in the erased part: 0xffff8000083d3600
| [   26.554135] lkdtm: FAIL: bad value number 234 in the erased part: 0xffff2f227fbd3b00
| [   26.554674] lkdtm: FAIL: bad value number 235 in the erased part: 0xffff2f220288ba00
| [   26.555210] lkdtm: FAIL: bad value number 236 in the erased part: 0x3da6d638c42c1e34
| [   26.555739] lkdtm: FAIL: bad value number 237 in the erased part: 0xffff8000083d3540
| [   26.556271] lkdtm: FAIL: bad value number 238 in the erased part: 0xc0
| [   26.556723] lkdtm: FAIL: bad value number 239 in the erased part: 0x0
| [   26.557182] lkdtm: FAIL: bad value number 240 in the erased part: 0xffff2f220288ba00
| [   26.557719] lkdtm: FAIL: bad value number 241 in the erased part: 0xffff2f227fbd3b00
| [   26.558478] lkdtm: FAIL: bad value number 242 in the erased part: 0xf882d638c447a134
| [   26.558944] lkdtm: FAIL: bad value number 243 in the erased part: 0xffff8000083d3530
| [   26.559407] lkdtm: FAIL: bad value number 244 in the erased part: 0x14bcd638c4494bf4
| [   26.559862] lkdtm: FAIL: bad value number 245 in the erased part: 0xffff8000083d3510
| [   26.560320] lkdtm: FAIL: bad value number 246 in the erased part: 0x33a7d638c44939c4
| [   26.560774] lkdtm: FAIL: bad value number 247 in the erased part: 0xffff8000083d34e0
| [   26.561227] lkdtm: FAIL: bad value number 248 in the erased part: 0x1
| [   26.561606] lkdtm: FAIL: bad value number 249 in the erased part: 0xffff2f22028701b0
| [   26.562293] lkdtm: FAIL: bad value number 250 in the erased part: 0xfff3d638c448fb6c
| [   26.562753] lkdtm: FAIL: bad value number 251 in the erased part: 0xffff8000083d34c0
| [   26.563206] lkdtm: FAIL: bad value number 252 in the erased part: 0x1
| [   26.563596] lkdtm: FAIL: bad value number 253 in the erased part: 0xffff2f22028701b0
| [   26.564055] lkdtm: FAIL: bad value number 254 in the erased part: 0xc3b1d638c448f978
| [   26.564509] lkdtm: FAIL: bad value number 255 in the erased part: 0xffff8000083d34a0
| [   26.564963] lkdtm: FAIL: bad value number 256 in the erased part: 0x4399d638c42c1cec
| [   26.565420] lkdtm: FAIL: bad value number 257 in the erased part: 0xffff8000083d34b0
| [   26.566045] lkdtm: FAIL: bad value number 258 in the erased part: 0xffff2f227fbd3b80
| [   26.566507] lkdtm: FAIL: bad value number 259 in the erased part: 0xffff2f220288ba80
| [   26.566965] lkdtm: FAIL: bad value number 260 in the erased part: 0xe9e9d638c42cca74
| [   26.567421] lkdtm: FAIL: bad value number 261 in the erased part: 0xffff8000083d34b0
| [   26.567821] lkdtm: FAIL: bad value number 262 in the erased part: 0xffff2f220288ba80
| [   26.568221] lkdtm: FAIL: bad value number 263 in the erased part: 0xffff2f227fbd3b80
| [   26.568620] lkdtm: FAIL: bad value number 264 in the erased part: 0xf697d638c42cb164
| [   26.569015] lkdtm: FAIL: bad value number 265 in the erased part: 0xffff8000083d3450
| [   26.569410] lkdtm: FAIL: bad value number 266 in the erased part: 0x47e5d638c42cb150
| [   26.569947] lkdtm: FAIL: bad value number 267 in the erased part: 0xffff8000083d3450
| [   26.570391] lkdtm: FAIL: bad value number 268 in the erased part: 0xf0b1d638c447ad28
| [   26.570788] lkdtm: FAIL: bad value number 269 in the erased part: 0xffff8000083d3430
| [   26.571189] lkdtm: FAIL: the thread stack is NOT properly erased!

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

On Tue, Apr 26, 2022 at 11:10:52AM +0100, Mark Rutland wrote:
> On Mon, Apr 25, 2022 at 03:54:00PM -0700, Kees Cook wrote:
> > On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > > This series reworks the stackleak code. The first patch fixes some
> > > latent issues on arm64, and the subsequent patches improve the code to
> > > improve clarity and permit better code generation.
> > 
> > This looks nice; thanks! I'll put this through build testing and get it
> > applied shortly...
> 
> Thanks!
> 
> Patch 1 is liable to conflict with come other stacktrace bits that may go in
> for v5.19, so it'd be good if either that could be queued as a fix for
> v5.1-rc4, or we'll have to figure out how to deal with conflicts later.
> 
> > > While the improvement is small, I think the improvement to clarity and
> > > code generation is a win regardless.
> > 
> > Agreed. I also want to manually inspect the resulting memory just to
> > make sure things didn't accidentally regress. There's also an LKDTM test
> > for basic functionality.
> 
> I assume that's the STACKLEAK_ERASING test?
> 
> I gave that a spin, but on arm64 that test is flaky even on baseline v5.18-rc1.
> On x86_64 it seems consistent after 100s of runs. I'll go dig into that now. 

I hacked in some debug, and it looks like the sp used in the test is far above
the current lowest_sp. The test is slightly wrong since it grabs the address of
a local variable rather than using current_stack_pointer, but the offset I see
is much larger:

# echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT 
[   27.665221] lkdtm: Performing direct entry STACKLEAK_ERASING
[   27.665986] lkdtm: FAIL: lowest_stack 0xffff8000083a39e0 is lower than test sp 0xffff8000083a3c80
[   27.667530] lkdtm: FAIL: the thread stack is NOT properly erased!

That's off by 0x2a0 (AKA 672) bytes, and it seems to be consistent from run to
run.

I note that an interrupt occuring could cause similar (since on arm64 those are
taken/triaged on the task stack before moving to the irq stack, and the irq
regs alone will take 300+ bytes), but that doesn't seem to be the problem here
given this is consistent, and it appears some prior function consumed a lot of
stack.

I *think* the same irq problem would apply to x86, but maybe that initial
triage happens on a trampoline stack.

I'll dig a bit more into the arm64 side...

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

On Tue, Apr 26, 2022 at 11:37:47AM +0100, Mark Rutland wrote:
> On Tue, Apr 26, 2022 at 11:10:52AM +0100, Mark Rutland wrote:
> > On Mon, Apr 25, 2022 at 03:54:00PM -0700, Kees Cook wrote:
> > > On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > > > This series reworks the stackleak code. The first patch fixes some
> > > > latent issues on arm64, and the subsequent patches improve the code to
> > > > improve clarity and permit better code generation.
> > > 
> > > This looks nice; thanks! I'll put this through build testing and get it
> > > applied shortly...
> > 
> > Thanks!
> > 
> > Patch 1 is liable to conflict with come other stacktrace bits that may go in
> > for v5.19, so it'd be good if either that could be queued as a fix for
> > v5.1-rc4, or we'll have to figure out how to deal with conflicts later.
> > 
> > > > While the improvement is small, I think the improvement to clarity and
> > > > code generation is a win regardless.
> > > 
> > > Agreed. I also want to manually inspect the resulting memory just to
> > > make sure things didn't accidentally regress. There's also an LKDTM test
> > > for basic functionality.
> > 
> > I assume that's the STACKLEAK_ERASING test?
> > 
> > I gave that a spin, but on arm64 that test is flaky even on baseline v5.18-rc1.
> > On x86_64 it seems consistent after 100s of runs. I'll go dig into that now. 
> 
> I hacked in some debug, and it looks like the sp used in the test is far above
> the current lowest_sp. The test is slightly wrong since it grabs the address of
> a local variable rather than using current_stack_pointer, but the offset I see
> is much larger:
> 
> # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT 
> [   27.665221] lkdtm: Performing direct entry STACKLEAK_ERASING
> [   27.665986] lkdtm: FAIL: lowest_stack 0xffff8000083a39e0 is lower than test sp 0xffff8000083a3c80
> [   27.667530] lkdtm: FAIL: the thread stack is NOT properly erased!
> 
> That's off by 0x2a0 (AKA 672) bytes, and it seems to be consistent from run to
> run.
> 
> I note that an interrupt occuring could cause similar (since on arm64 those are
> taken/triaged on the task stack before moving to the irq stack, and the irq
> regs alone will take 300+ bytes), but that doesn't seem to be the problem here
> given this is consistent, and it appears some prior function consumed a lot of
> stack.
> 
> I *think* the same irq problem would apply to x86, but maybe that initial
> triage happens on a trampoline stack.
> 
> I'll dig a bit more into the arm64 side...

That offset above seems to be due to the earlier logic in direct_entry(), which
I guess is running out-of-line. With that hacked to:

----------------
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index e2228b6fc09bb..53f3027e8202d 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -378,8 +378,9 @@ static ssize_t direct_entry(struct file *f, const char __user *user_buf,
                size_t count, loff_t *off)
 {
        const struct crashtype *crashtype;
-       char *buf;
+       char *buf = "STACKLEAK_ERASING";
 
+#if 0
        if (count >= PAGE_SIZE)
                return -EINVAL;
        if (count < 1)
@@ -395,13 +396,17 @@ static ssize_t direct_entry(struct file *f, const char __user *user_buf,
        /* NULL-terminate and remove enter */
        buf[count] = '\0';
        strim(buf);
+#endif
 
        crashtype = find_crashtype(buf);
+
+#if 0
        free_page((unsigned long) buf);
        if (!crashtype)
                return -EINVAL;
+#endif
 
-       pr_info("Performing direct entry %s\n", crashtype->name);
+       // pr_info("Performing direct entry %s\n", crashtype->name);
        lkdtm_do_action(crashtype);
        *off += count;
 
----------------

... the SP check doesn't fail, but I still see intermittent bad value failures.
Those might be due to interrupt frames.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Alexander Popov]

On 26.04.2022 01:54, Kees Cook wrote:
> On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
>> This series reworks the stackleak code. The first patch fixes some
>> latent issues on arm64, and the subsequent patches improve the code to
>> improve clarity and permit better code generation.
> 
> This looks nice; thanks! I'll put this through build testing and get it
> applied shortly...
> 
>> While the improvement is small, I think the improvement to clarity and
>> code generation is a win regardless.
> 
> Agreed. I also want to manually inspect the resulting memory just to
> make sure things didn't accidentally regress. There's also an LKDTM test
> for basic functionality.

Hi Mark and Kees!

Glad to see this patch series.

I've looked at it briefly. Mark, I see your questions in the patches that I can 
answer.

Please give me some time, I'm going to work on your patch series next week. I'll 
return with review and testing.

Thanks!
Alexander

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

On Tue, Apr 26, 2022 at 06:51:04PM +0300, Alexander Popov wrote:
> On 26.04.2022 01:54, Kees Cook wrote:
> > On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > > This series reworks the stackleak code. The first patch fixes some
> > > latent issues on arm64, and the subsequent patches improve the code to
> > > improve clarity and permit better code generation.
> > 
> > This looks nice; thanks! I'll put this through build testing and get it
> > applied shortly...
> > 
> > > While the improvement is small, I think the improvement to clarity and
> > > code generation is a win regardless.
> > 
> > Agreed. I also want to manually inspect the resulting memory just to
> > make sure things didn't accidentally regress. There's also an LKDTM test
> > for basic functionality.
> 
> Hi Mark and Kees!
> 
> Glad to see this patch series.
> 
> I've looked at it briefly. Mark, I see your questions in the patches that I
> can answer.
> 
> Please give me some time, I'm going to work on your patch series next week.
> I'll return with review and testing.

Sure thing, thanks!

FWIW, I spotted a couple of issues in my patches today while testing,
and if you're happy I can post a v2 later this week with those fixed, so
you don't need to waste time with those.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Mark Rutland]

On Mon, Apr 25, 2022 at 03:54:00PM -0700, Kees Cook wrote:
> On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > This series reworks the stackleak code. The first patch fixes some
> > latent issues on arm64, and the subsequent patches improve the code to
> > improve clarity and permit better code generation.
> 
> This looks nice; thanks! I'll put this through build testing and get it
> applied shortly...

FWIW, looking at testing I've spotted a fencepost error, so I'll spin a
v2 with that fixed (and with updates to the LKDTM test).

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Kees Cook]

On Tue, Apr 26, 2022 at 05:01:27PM +0100, Mark Rutland wrote:
> On Mon, Apr 25, 2022 at 03:54:00PM -0700, Kees Cook wrote:
> > On Mon, Apr 25, 2022 at 12:55:55PM +0100, Mark Rutland wrote:
> > > This series reworks the stackleak code. The first patch fixes some
> > > latent issues on arm64, and the subsequent patches improve the code to
> > > improve clarity and permit better code generation.
> > 
> > This looks nice; thanks! I'll put this through build testing and get it
> > applied shortly...
> 
> FWIW, looking at testing I've spotted a fencepost error, so I'll spin a
> v2 with that fixed (and with updates to the LKDTM test).

Ah! Oops, ok, I'll unpush the series, I missed this. :)

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] stackleak: fixes and rework

[Kees Cook]

On Mon, 25 Apr 2022 12:55:55 +0100, Mark Rutland wrote:
> This series reworks the stackleak code. The first patch fixes some
> latent issues on arm64, and the subsequent patches improve the code to
> improve clarity and permit better code generation.
> 
> I started working on this as a tangent from rework to arm64's stacktrace
> code. Looking at users of the `on_*_stack()` helpers I noticed that the
> assembly generated for stackleak was particularly awful as it performed
> a lot of redundant work and also called instrumentable code, which isn't
> sound.
> 
> [...]

Applied to for-next/hardening, thanks!

[1/8] arm64: stackleak: fix current_top_of_stack()
      https://git.kernel.org/kees/c/b9f8167d08e9
[2/8] stackleak: move skip_erasing() check earlier
      https://git.kernel.org/kees/c/b7d6315d1d7c
[3/8] stackleak: rework stack low bound handling
      https://git.kernel.org/kees/c/1f4f72d1d99e
[4/8] stackleak: clarify variable names
      https://git.kernel.org/kees/c/52a2aa794e0a
[5/8] stackleak: rework stack high bound handling
      https://git.kernel.org/kees/c/83301ac044c9
[6/8] stackleak: remove redundant check
      https://git.kernel.org/kees/c/0cd7ee6880c7
[7/8] stackleak: add on/off stack variants
      https://git.kernel.org/kees/c/9bb0b174fd2b
[8/8] arm64: entry: use stackleak_erase_on_task_stack()
      https://git.kernel.org/kees/c/6a5927e73497

-- 
Kees Cook


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel