[v2,00/13] stackleak: fixes and rework

Message

Mark Rutland April 27, 2022, 5:31 p.m. UTC

Hi Alexander, Kees,

This is the vs I promised. Since Alexander wanted to look at this in
more detail (and since this is subtle and needs review), I'm assuming
that Kees will pick this up some time next week after that's happened,
if all goes well. :)

This series reworks the stackleak code and the associated LKDTM test.
The first patch fixes some latent issues on arm64, and the subsequent
patches improve the code to improve clarity and permit better code
generation. Patches 8-10 address some latent issues in the LKDTM test
and add more diagnostic output.

Since v1 [1]:
* Fix and rework the LKDTM test
* Rework the poison scan

[1] https://lore.kernel.org/lkml/[e-mail]/

Benchmarking arm64 with a QEMU HVF VM on an M1 Macbook Pro shows a
reasonable improvement when stackleak is enabled. I've included figures
for when stackleak is dynamically disabled with v2:

* Calling getpid 1^22 times in a loop (avg 50 runs)
  
  5.18-rc1/on: 0.652099387s ( +-  0.13% )
  v1/on:       0.641005661s ( +-  0.13% ) ; 1.7% improvement
  v2/on:       0.611699824s ( +-  0.08% ) ; 6.2% improvement
  v2/off:      0.507505632s ( +-  0.15% )

* perf bench sched pipe (single run)

  5.18-rc1/on: 2.138s total
  v1/on:       2.118s total ; 0.93% improvement
  v2/on:       2.116s total ; 1.03% improvement
  v2/off:      1.708s total

The large jump between v1 and v2 is largely due to the changes to poison
scanning avoiding redundantly overwriting poison. For the getpid syscall
the poison which gets overwritten is a substantial fraction of the stack
usage, and for more involved syscalls this may be a trivial fraction, so
the average benefit is fairly small.

With the whole series applied, the LKDTM test reliably passes on both
arm64 and x86_64, e.g.

| # uname -a
| Linux buildroot 5.18.0-rc1-00013-g26f638ab0d7c #3 SMP PREEMPT Wed Apr 27 16:21:37 BST 2022 aarch64 GNU/Linux
| # echo STACKLEAK_ERASING > /sys/kernel/debug/provoke-crash/DIRECT 
| lkdtm: Performing direct entry STACKLEAK_ERASING
| lkdtm: stackleak stack usage:
|   high offset: 336 bytes
|   current:     688 bytes
|   lowest:      1232 bytes
|   tracked:     1232 bytes
|   untracked:   672 bytes
|   poisoned:    14136 bytes
|   low offset:  8 bytes
| lkdtm: OK: the rest of the thread stack is properly erased

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 partially 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 6 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 changes the way we scan for poison. This fixes what I believe
are fencepost errors, and also avoids the need to redundantly overwrite
poison.

Patches 8-11 rework the LKDTM test, fixing cases where the test could
spuriously fail and improving the diagnostic output.

Patches 12 and 13 add 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. The former is
used on arm64 where we always perform the erase on the task stack, and
I've included the latter for completeness (as e.g. it could be used on
x86) given it is trivial.

Thanks,
Mark.

Mark Rutland (13):
  arm64: stackleak: fix current_top_of_stack()
  stackleak: move skip_erasing() check earlier
  stackleak: remove redundant check
  stackleak: rework stack low bound handling
  stackleak: clarify variable names
  stackleak: rework stack high bound handling
  stackleak: rework poison scanning
  lkdtm/stackleak: avoid spurious failure
  lkdtm/stackleak: rework boundary management
  lkdtm/stackleak: prevent unexpected stack usage
  lkdtm/stackleak: check stack boundaries
  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 +-
 drivers/misc/lkdtm/stackleak.c     | 133 +++++++++++++++++++----------
 include/linux/stackleak.h          |  55 +++++++++++-
 kernel/stackleak.c                 | 105 ++++++++++++++---------
 5 files changed, 210 insertions(+), 95 deletions(-)

Comments

Kees Cook May 4, 2022, 7:16 p.m. UTC | #1

On Wed, 27 Apr 2022 18:31:15 +0100, Mark Rutland wrote:
> This is the vs I promised. Since Alexander wanted to look at this in
> more detail (and since this is subtle and needs review), I'm assuming
> that Kees will pick this up some time next week after that's happened,
> if all goes well. :)
> 
> This series reworks the stackleak code and the associated LKDTM test.
> The first patch fixes some latent issues on arm64, and the subsequent
> patches improve the code to improve clarity and permit better code
> generation. Patches 8-10 address some latent issues in the LKDTM test
> and add more diagnostic output.
> 
> [...]

I fixed some small commit log typos, but otherwise this looks great. If
anything new comes up we can adjust it.

Applied to for-next/hardening, thanks!

[01/13] arm64: stackleak: fix current_top_of_stack()
        https://git.kernel.org/kees/c/4c849d27b729
[02/13] stackleak: move skip_erasing() check earlier
        https://git.kernel.org/kees/c/e98a7c56d73c
[03/13] stackleak: remove redundant check
        https://git.kernel.org/kees/c/e45d9f71deea
[04/13] stackleak: rework stack low bound handling
        https://git.kernel.org/kees/c/cbe7edb47d3c
[05/13] stackleak: clarify variable names
        https://git.kernel.org/kees/c/e9da2241ed85
[06/13] stackleak: rework stack high bound handling
        https://git.kernel.org/kees/c/cfef4372a4b7
[07/13] stackleak: rework poison scanning
        https://git.kernel.org/kees/c/ff5f6d37e5bc
[08/13] lkdtm/stackleak: avoid spurious failure
        https://git.kernel.org/kees/c/23fd893fa0d7
[09/13] lkdtm/stackleak: rework boundary management
        https://git.kernel.org/kees/c/f4cfacd92972
[10/13] lkdtm/stackleak: prevent unexpected stack usage
        https://git.kernel.org/kees/c/c393c0b98d75
[11/13] lkdtm/stackleak: check stack boundaries
        https://git.kernel.org/kees/c/b6bf5a354eca
[12/13] stackleak: add on/off stack variants
        https://git.kernel.org/kees/c/96c59349a56c
[13/13] arm64: entry: use stackleak_erase_on_task_stack()
        https://git.kernel.org/kees/c/d46ac904fd35