Compile-time stack validation

This is v11 of the compile-time stack validation patch set, along with proposed fixes for many of the warnings it found. It's based on the tip/master branch.

The only real change since v10 is some improvements in patch 3 to the documentation and changelog which attempt to better describe why stack validation is needed.

v10 can be found here:

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

For more information about the motivation behind this patch set, and more details about what it does, please see the changelog in patch 3. Patch 3 also has Documentation/stack-validation.txt which has further details.

Patches 1-5 are the stackvalidate tool and some related macros.

Patches 6-20 are some proposed fixes for several of the warnings reported by stackvalidate. They've been compile-tested and boot tested in a VM, but I haven't attempted any meaningful testing for many of them.

v11:
- attempt to answer the "why" question better in the documentation and commit message
- s/FP_SAVE/FRAME_BEGIN/ in documentation

v10:
- add scripts/mod to directory ignores
- remove circular dependencies for ignored objects which are built before stackvalidate
- fix CONFIG_MODVERSIONS incompatibility

v9:
- rename FRAME/ENDFRAME -> FRAME_BEGIN/FRAME_END
- fix jump table issue for when the original instruction is a jump
- drop paravirt thunk alignment patch
- add maintainers to CC for proposed warning fixes

v8:
- add proposed fixes for warnings
- fix all memory leaks
- process ignores earlier and add more ignore checks
- always assume POPCNT alternative is enabled
- drop hweight inline asm fix
- drop __schedule() ignore patch
- change .Ltemp_\@ to .Lstackvalidate_ignore_\@ in asm macro
- fix CONFIG_* checks in asm macros
- add C versions of ignore macros and frame macros
- change ";" to "\n" in C macros
- add ifdef CONFIG_STACK_VALIDATION checks in C ignore macros
- use numbered label in C ignore macro
- add missing break in switch case statement in arch-x86.c

v7:
- sibling call support
- document proposed solution for inline asm() frame pointer issues
- say "kernel entry/exit" instead of "context switch"
- clarify the checking of switch statement jump tables
- discard __stackvalidate_ignore_* sections in linker script
- use .Ltemp_\@ to get a unique label instead of static 3-digit number
- change STACKVALIDATE_IGNORE_FUNC variable to a static
- move STACKVALIDATE_IGNORE_INSN to arch-specific .h file

v6:
- rename asmvalidate -> stackvalidate (again)
- gcc-generated object file support
- recursive branch state analysis
- external jump support
- fixup/exception table support
- jump label support
- switch statement jump table support
- added documentation
- detection of "noreturn" dead end functions
- added a Kbuild mechanism for skipping files and dirs
- moved frame pointer macros to arch/x86/include/asm/frame.h
- moved ignore macros to include/linux/stackvalidate.h

v5:
- stackvalidate -> asmvalidate
- frame pointers only required for non-leaf functions
- check for the use of the FP_SAVE/RESTORE macros instead of manually analyzing code to detect frame pointer usage
- additional checks to ensure each function doesn't leave its boundaries
- make the macros simpler and more flexible
- support for analyzing ALTERNATIVE macros
- simplified the arch interfaces in scripts/asmvalidate/arch.h
- fixed some asmvalidate warnings
- rebased onto latest tip asm cleanups
- many more small changes

v4:
- Changed the default to CONFIG_STACK_VALIDATION=n, until all the asm code can get cleaned up.
- Fixed a stackvalidate error path exit code issue found by Michal Marek.

v3:
- Added a patch to make the push/pop CFI macros arch-independent, as suggested by H. Peter Anvin

v2:
- Fixed memory leaks reported by Petr Mladek

Josh Poimboeuf (20):
x86/asm: Frame pointer macro cleanup
x86/asm: Add C versions of frame pointer macros x86/stackvalidate: Compile-time stack validation x86/stackvalidate: Add file and directory ignores x86/stackvalidate: Add ignore macros
x86/xen: Add stack frame dependency to hypercall inline asm calls x86/paravirt: Add stack frame dependency to PVOP inline asm calls x86/paravirt: Create a stack frame in PV_CALLEE_SAVE_REGS_THUNK x86/amd: Set ELF function type for vide()
x86/reboot: Add ljmp instructions to stackvalidate whitelist x86/xen: Add xen_cpuid() and xen_setup_gdt() to stackvalidate
whitelists
x86/asm/crypto: Create stack frames in aesni-intel_asm.S x86/asm/crypto: Move .Lbswap_mask data to .rodata section x86/asm/crypto: Move jump_table to .rodata section x86/asm/crypto: Create stack frames in clmul_ghash_mul/update() x86/asm/entry: Create stack frames in thunk functions x86/asm/acpi: Create a stack frame in do_suspend_lowlevel() x86/asm: Create stack frames in rwsem functions x86/asm/efi: Create a stack frame in efi_call() x86/asm/power: Create stack frames in hibernate_asm_64.S

 Documentation/stack-validation.txt        | 338 +++++++++++
 MAINTAINERS                               |   8 +
 arch/Kconfig                              |   6 +
 arch/x86/Kconfig                          |   1 +
 arch/x86/Makefile                         |   6 +-
 arch/x86/boot/Makefile                    |   3 +-
 arch/x86/boot/compressed/Makefile         |   3 +-
 arch/x86/crypto/aesni-intel_asm.S         |  21 +
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S |   8 +-
 arch/x86/crypto/ghash-clmulni-intel_asm.S |   5 +
 arch/x86/entry/thunk_64.S                 |   4 +
 arch/x86/entry/vdso/Makefile              |   5 +-
 arch/x86/include/asm/frame.h              |  51 +-
 arch/x86/include/asm/paravirt.h           |   9 +-
 arch/x86/include/asm/paravirt_types.h     |  18 +-
 arch/x86/include/asm/stackvalidate.h      |  45 ++
 arch/x86/include/asm/xen/hypercall.h      |   5 +-
 arch/x86/kernel/acpi/wakeup_64.S          |   3 +
 arch/x86/kernel/cpu/amd.c                 |   5 +-
 arch/x86/kernel/reboot.c                  |   7 +-
 arch/x86/kernel/vmlinux.lds.S             |   5 +-
 arch/x86/lib/rwsem.S                      |  11 +-
 arch/x86/platform/efi/efi_stub_64.S       |   3 +
 arch/x86/power/hibernate_asm_64.S         |   7 +
 arch/x86/purgatory/Makefile               |   2 +
 arch/x86/realmode/Makefile                |   4 +-
 arch/x86/realmode/rm/Makefile             |   3 +-
 arch/x86/xen/enlighten.c                  |   4 +-
 drivers/firmware/efi/libstub/Makefile     |   1 +
 include/linux/stackvalidate.h             |  28 +
 lib/Kconfig.debug                         |  11 +
 scripts/Makefile                          |   1 +
 scripts/Makefile.build                    |  37 +-
 scripts/mod/Makefile                      |   2 +
 scripts/stackvalidate/Makefile            |  24 +
 scripts/stackvalidate/arch-x86.c          | 160 +++++
 scripts/stackvalidate/arch.h              |  44 ++
 scripts/stackvalidate/elf.c               | 427 +++++++++++++
 scripts/stackvalidate/elf.h               |  92 +++
 scripts/stackvalidate/list.h              | 217 +++++++
 scripts/stackvalidate/special.c           | 199 ++++++
 scripts/stackvalidate/special.h           |  42 ++
 scripts/stackvalidate/stackvalidate.c     | 976 ++++++++++++++++++++++++++++++
 43 files changed, 2803 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/stack-validation.txt
 create mode 100644 arch/x86/include/asm/stackvalidate.h
 create mode 100644 include/linux/stackvalidate.h
 create mode 100644 scripts/stackvalidate/Makefile
 create mode 100644 scripts/stackvalidate/arch-x86.c
 create mode 100644 scripts/stackvalidate/arch.h
 create mode 100644 scripts/stackvalidate/elf.c
 create mode 100644 scripts/stackvalidate/elf.h
 create mode 100644 scripts/stackvalidate/list.h
 create mode 100644 scripts/stackvalidate/special.c
 create mode 100644 scripts/stackvalidate/special.h
 create mode 100644 scripts/stackvalidate/stackvalidate.c

2.4.3

The asm macros for setting up and restoring the frame pointer aren't currently being used. However, they will be needed soon to help asm functions to comply with stackvalidate.

Rename FRAME/ENDFRAME to FRAME_BEGIN/FRAME_END for more symmetry. Also make the code more readable and improve the comments.

---
 arch/x86/include/asm/frame.h | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

index 793179c..8a6cd26 100644
@@ -1,23 +1,26 @@
+#ifndef _ASM_X86_FRAME_H
+#define _ASM_X86_FRAME_H
+
 #ifdef __ASSEMBLY__
 
 #include <asm/asm.h>
 
-/* The annotation hides the frame from the unwinder and makes it look
-   like a ordinary ebp save/restore. This avoids some special cases for
-   frame pointer later */
+/*
+ * These are stack frame creation macros.  They should be used by every
+ * callable non-leaf asm function to make kernel stack traces more reliable.
+ */
+.macro FRAME_BEGIN
+#ifdef CONFIG_FRAME_POINTER
+	push %_ASM_BP
+	_ASM_MOV %_ASM_SP, %_ASM_BP
+#endif
+.endm
+
+.macro FRAME_END
 #ifdef CONFIG_FRAME_POINTER
-	.macro FRAME
-	__ASM_SIZE(push,)	%__ASM_REG(bp)
-	__ASM_SIZE(mov)		%__ASM_REG(sp), %__ASM_REG(bp)
-	.endm
-	.macro ENDFRAME
-	__ASM_SIZE(pop,)	%__ASM_REG(bp)
-	.endm
-#else
-	.macro FRAME
-	.endm
-	.macro ENDFRAME
-	.endm
+	pop %_ASM_BP
 #endif
+.endm
 
 #endif  /*  __ASSEMBLY__  */
+#endif /* _ASM_X86_FRAME_H */

2.4.3

Add C versions of the frame pointer macros which can be used to create a stack frame in inline assembly.

---
 arch/x86/include/asm/frame.h | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

index 8a6cd26..9a30ec7 100644
@@ -1,10 +1,10 @@
 #ifndef _ASM_X86_FRAME_H
 #define _ASM_X86_FRAME_H
 
-#ifdef __ASSEMBLY__
-
 #include <asm/asm.h>
 
+#ifdef __ASSEMBLY__
+
 /*
  * These are stack frame creation macros.  They should be used by every
  * callable non-leaf asm function to make kernel stack traces more reliable.

@@ -22,5 +22,21 @@
 #endif
 .endm
 
+#else /* !__ASSEMBLY__ */
+
+#ifdef CONFIG_FRAME_POINTER
+
+#define FRAME_BEGIN				\
+	"push %" _ASM_BP "\n"			\
+	_ASM_MOV "%" _ASM_SP ", %" _ASM_BP "\n"
+
+#define FRAME_END "pop %" _ASM_BP "\n"
+
+#else /* !CONFIG_FRAME_POINTER */
+
+#define FRAME_BEGIN ""
+#define FRAME_END ""
+
+#endif /* CONFIG_FRAME_POINTER */
 #endif  /*  __ASSEMBLY__  */
 #endif /* _ASM_X86_FRAME_H */

2.4.3

This adds a CONFIG_STACK_VALIDATION option which enables a host tool named stackvalidate which runs at compile time. It analyzes every .o file and ensures the validity of its stack metadata. It enforces a set of rules on asm code and C inline assembly code so that stack traces can be reliable.

For each function, it recursively follows all possible code paths and validates the correct frame pointer state at each instruction.

It also follows code paths involving special sections, like .altinstructions, __jump_table, and __ex_table, which can add alternative execution paths to a given instruction (or set of instructions). Similarly, it knows how to follow switch statements, for which gcc sometimes uses jump tables.

Here are some of the benefits of validating stack metadata:

a) More reliable stack traces for frame pointer enabled kernels

Frame pointers are used for debugging purposes. They allow runtime code and debug tools to be able to walk the stack to determine the chain of function call sites that led to the currently executing code.

For some architectures, frame pointers are enabled by CONFIG_FRAME_POINTER. For some other architectures they may be required by the ABI (sometimes referred to as "backchain pointers").

For C code, gcc automatically generates instructions for setting up frame pointers when the -fno-omit-frame-pointer option is used.

But for asm code, the frame setup instructions have to be written by hand, which most people don't do. So the end result is that CONFIG_FRAME_POINTER is honored for C code but not for most asm code.

For stack traces based on frame pointers to be reliable, all functions which call other functions must first create a stack frame and update the frame pointer. If a first function doesn't properly create a stack frame before calling a second function, the *caller* of the first function will be skipped on the stack trace.

For example, consider the following example backtrace with frame pointers enabled:

     [<ffffffff81812584>] dump_stack+0x4b/0x63
     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
     [<ffffffff8127f568>] seq_read+0x108/0x3e0
     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
     [<ffffffff81256197>] __vfs_read+0x37/0x100
     [<ffffffff81256b16>] vfs_read+0x86/0x130
     [<ffffffff81257898>] SyS_read+0x58/0xd0
     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76

It correctly shows that the caller of cmdline_proc_show() is seq_read().

If we remove the frame pointer logic from cmdline_proc_show() by replacing the frame pointer related instructions with nops, here's what it looks like instead:

     [<ffffffff81812584>] dump_stack+0x4b/0x63
     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
     [<ffffffff81256197>] __vfs_read+0x37/0x100
     [<ffffffff81256b16>] vfs_read+0x86/0x130
     [<ffffffff81257898>] SyS_read+0x58/0xd0
     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76

Notice that cmdline_proc_show()'s caller, seq_read(), has been skipped. Instead the stack trace seems to show that cmdline_proc_show() was called by proc_reg_read().

The benefit of stackvalidate here is that because it ensures that *all* functions honor CONFIG_FRAME_POINTER, no functions will ever[*] be skipped on a stack trace.

[*] unless an interrupt or exception has occurred at the very
beginning of a function before the stack frame has been created, or at the very end of the function after the stack frame has been destroyed. This is an inherent limitation of frame pointers.

b) 100% reliable stack traces for DWARF enabled kernels

This is not yet implemented. See Documentation/stack-validation.txt for more details about what is planned.

c) Higher live patching compatibility rate

This is not yet implemented. See Documentation/stack-validation.txt for more details about what is planned.

To achieve the validation, stackvalidate enforces the following rules:

1. Each callable function must be annotated as such with the ELF function type. In asm code, this is typically done using the ENTRY/ENDPROC macros. If stackvalidate finds a return instruction outside of a function, it flags an error since that usually indicates callable code which should be annotated accordingly.

This rule is needed so that stackvalidate can properly identify each callable function in order to analyze its stack metadata.

2. Conversely, each section of code which is *not* callable should *not* be annotated as an ELF function. The ENDPROC macro shouldn't be used in this case.

This rule is needed so that stackvalidate can ignore non-callable code. Such code doesn't have to follow any of the other rules.

3. Each callable function which calls another function must have the correct frame pointer logic, if required by CONFIG_FRAME_POINTER or the architecture's back chain rules. This can by done in asm code with the FRAME_BEGIN/FRAME_END macros.

This rule ensures that frame pointer based stack traces will work as designed. If function A doesn't create a stack frame before calling function B, the _caller_ of function A will be skipped on the stack trace.

4. Dynamic jumps and jumps to undefined symbols are only allowed if:

a) the jump is part of a switch statement; or

b) the jump matches sibling call semantics and the frame pointer has the same value it had on function entry.

This rule is needed so that stackvalidate can reliably analyze all of a function's code paths. If a function jumps to code in another file, and it's not a sibling call, stackvalidate has no way to follow the jump because it only analyzes a single file at a time.

5. A callable function may not execute kernel entry/exit instructions. The only code which needs such instructions is kernel entry code, which shouldn't be be in callable functions anyway.

This rule is just a sanity check to ensure that callable functions return normally.

It currently only supports x86_64. I tried to make the code generic so that support for other architectures can hopefully be plugged in relatively easily.

As a first step, CONFIG_STACK_VALIDATION is disabled by default, and all reported non-compliances result in warnings. Once we get them all cleaned up, we can change the default to CONFIG_STACK_VALIDATION=y and change the warnings to errors to keep the stack metadata clean.

On my Lenovo laptop with a i7-4810MQ 4-core/8-thread CPU, building the kernel with it enabled adds less than 3 seconds of total build time. It hasn't been optimized for performance yet, so there are probably some opportunities for better build performance.

---
 Documentation/stack-validation.txt    | 338 ++++++++++++
 MAINTAINERS                           |   8 +
 arch/Kconfig                          |   6 +
 arch/x86/Kconfig                      |   1 +
 arch/x86/Makefile                     |   6 +-
 lib/Kconfig.debug                     |  11 +
 scripts/Makefile                      |   1 +
 scripts/Makefile.build                |  37 +-
 scripts/stackvalidate/Makefile        |  24 +
 scripts/stackvalidate/arch-x86.c      | 160 ++++++
 scripts/stackvalidate/arch.h          |  44 ++
 scripts/stackvalidate/elf.c           | 427 +++++++++++++++
 scripts/stackvalidate/elf.h           |  92 ++++
 scripts/stackvalidate/list.h          | 217 ++++++++
 scripts/stackvalidate/special.c       | 199 +++++++
 scripts/stackvalidate/special.h       |  42 ++
 scripts/stackvalidate/stackvalidate.c | 976 ++++++++++++++++++++++++++++++++++
 17 files changed, 2584 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/stack-validation.txt
 create mode 100644 scripts/stackvalidate/Makefile
 create mode 100644 scripts/stackvalidate/arch-x86.c
 create mode 100644 scripts/stackvalidate/arch.h
 create mode 100644 scripts/stackvalidate/elf.c
 create mode 100644 scripts/stackvalidate/elf.h
 create mode 100644 scripts/stackvalidate/list.h
 create mode 100644 scripts/stackvalidate/special.c
 create mode 100644 scripts/stackvalidate/special.h
 create mode 100644 scripts/stackvalidate/stackvalidate.c

new file mode 100644
index 0000000..87a5ab8
@@ -0,0 +1,338 @@
+Compile-time stack validation
+=============================
+
+
+Overview
+--------
+
+The CONFIG_STACK_VALIDATION option enables a host tool named
+stackvalidate which runs at compile time.  It analyzes every .o file and
+ensures the validity of its stack metadata.  It enforces a set of rules
+on asm code and C inline assembly code so that stack traces can be
+reliable.
+
+Currently it only checks frame pointer usage, but there are plans to add
+CFI validation for C files and CFI generation for asm files.
+
+For each function, it recursively follows all possible code paths and
+validates the correct frame pointer state at each instruction.
+
+It also follows code paths involving special sections, like
+.altinstructions, __jump_table, and __ex_table, which can add
+alternative execution paths to a given instruction (or set of
+instructions).  Similarly, it knows how to follow switch statements, for
+which gcc sometimes uses jump tables.
+
+
+Why do we need stack validation?
+--------------------------------
+
+Here are some of the benefits of validating stack metadata:
+
+a) More reliable stack traces for frame pointer enabled kernels
+
+   Frame pointers are used for debugging purposes.  They allow runtime
+   code and debug tools to be able to walk the stack to determine the
+   chain of function call sites that led to the currently executing
+   code.
+
+   For some architectures, frame pointers are enabled by
+   CONFIG_FRAME_POINTER.  For some other architectures they may be
+   required by the ABI (sometimes referred to as "backchain pointers").
+
+   For C code, gcc automatically generates instructions for setting up
+   frame pointers when the -fno-omit-frame-pointer option is used.
+
+   But for asm code, the frame setup instructions have to be written by
+   hand, which most people don't do.  So the end result is that
+   CONFIG_FRAME_POINTER is honored for C code but not for most asm code.
+
+   For stack traces based on frame pointers to be reliable, all
+   functions which call other functions must first create a stack frame
+   and update the frame pointer.  If a first function doesn't properly
+   create a stack frame before calling a second function, the *caller*
+   of the first function will be skipped on the stack trace.
+
+   For example, consider the following example backtrace with frame
+   pointers enabled:
+
+     [<ffffffff81812584>] dump_stack+0x4b/0x63
+     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
+     [<ffffffff8127f568>] seq_read+0x108/0x3e0
+     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
+     [<ffffffff81256197>] __vfs_read+0x37/0x100
+     [<ffffffff81256b16>] vfs_read+0x86/0x130
+     [<ffffffff81257898>] SyS_read+0x58/0xd0
+     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76
+
+   It correctly shows that the caller of cmdline_proc_show() is
+   seq_read().
+
+   If we remove the frame pointer logic from cmdline_proc_show() by
+   replacing the frame pointer related instructions with nops, here's
+   what it looks like instead:
+
+     [<ffffffff81812584>] dump_stack+0x4b/0x63
+     [<ffffffff812d6dc2>] cmdline_proc_show+0x12/0x30
+     [<ffffffff812cce62>] proc_reg_read+0x42/0x70
+     [<ffffffff81256197>] __vfs_read+0x37/0x100
+     [<ffffffff81256b16>] vfs_read+0x86/0x130
+     [<ffffffff81257898>] SyS_read+0x58/0xd0
+     [<ffffffff8181c1f2>] entry_SYSCALL_64_fastpath+0x12/0x76
+
+   Notice that cmdline_proc_show()'s caller, seq_read(), has been
+   skipped.  Instead the stack trace seems to show that
+   cmdline_proc_show() was called by proc_reg_read().
+
+   The benefit of stackvalidate here is that because it ensures that
+   *all* functions honor CONFIG_FRAME_POINTER, no functions will ever[*]
+   be skipped on a stack trace.
+
+   [*] unless an interrupt or exception has occurred at the very
+       beginning of a function before the stack frame has been created,
+       or at the very end of the function after the stack frame has been
+       destroyed.  This is an inherent limitation of frame pointers.
+
+b) 100% reliable stack traces for DWARF enabled kernels
+
+   (NOTE: This is not yet implemented)
+
+   As an alternative to frame pointers, DWARF Call Frame Information
+   (CFI) metadata can be used to walk the stack.  Unlike frame pointers,
+   CFI metadata is out of band.  So it doesn't affect runtime
+   performance and it can be reliable even when interrupts or exceptions
+   are involved.
+
+   For C code, gcc automatically generates DWARF CFI metadata.  But for
+   asm code, generating CFI is a tedious manual approach which requires
+   manually placed .cfi assembler macros to be scattered throughout the
+   code.  It's clumsy and very easy to get wrong, and it makes the real
+   code harder to read.
+
+   Stackvalidate will improve this situation in several ways.  For code
+   which already has CFI annotations, it will validate them.  For code
+   which doesn't have CFI annotations, it will generate them.  So an
+   architecture can opt to strip out all the manual .cfi annotations
+   from their asm code and have stackvalidate generate them instead.
+
+   We might also add a runtime stack validation debug option where we
+   periodically walk the stack from schedule() and/or an NMI to ensure
+   that the stack metadata is sane and that we reach the bottom of the
+   stack.
+
+   So the benefit of stackvalidate here will be that external tooling
+   should always show perfect stack traces.  And the same will be true
+   for kernel warning/oops traces if the architecture has a runtime
+   DWARF unwinder.
+
+c) Higher live patching compatibility rate
+
+   (NOTE: This is not yet implemented)
+
+   Currently with CONFIG_LIVEPATCH there's a basic live patching
+   framework which is safe for roughly 85-90% of "security" fixes.  But
+   patches can't have complex features like function dependency or
+   prototype changes, or data structure changes.
+
+   There's a strong need to support patches which have the more complex
+   features so that the patch compatibility rate for security fixes can
+   eventually approach something resembling 100%.  To achieve that, a
+   "consistency model" is needed, which allows tasks to be safely
+   transitioned from an unpatched state to a patched state.
+
+   One of the key requirements of the currently proposed livepatch
+   consistency model [*] is that it needs to walk the stack of each
+   sleeping task to determine if it can be transitioned to the patched
+   state.  If stackvalidate can ensure that stack traces are reliable,
+   this consistency model can be used and the live patching
+   compatibility rate can be improved significantly.
+
+   [*] https://lkml.kernel.org/r/[email protected]
+
+
+Rules
+-----
+
+To achieve the validation, stackvalidate enforces the following rules:
+
+1. Each callable function must be annotated as such with the ELF
+   function type.  In asm code, this is typically done using the
+   ENTRY/ENDPROC macros.  If stackvalidate finds a return instruction
+   outside of a function, it flags an error since that usually indicates
+   callable code which should be annotated accordingly.
+
+   This rule is needed so that stackvalidate can properly identify each
+   callable function in order to analyze its stack metadata.
+
+2. Conversely, each section of code which is *not* callable should *not*
+   be annotated as an ELF function.  The ENDPROC macro shouldn't be used
+   in this case.
+
+   This rule is needed so that stackvalidate can ignore non-callable
+   code.  Such code doesn't have to follow any of the other rules.
+
+3. Each callable function which calls another function must have the
+   correct frame pointer logic, if required by CONFIG_FRAME_POINTER or
+   the architecture's back chain rules.  This can by done in asm code
+   with the FRAME_BEGIN/FRAME_END macros.
+
+   This rule ensures that frame pointer based stack traces will work as
+   designed.  If function A doesn't create a stack frame before calling
+   function B, the _caller_ of function A will be skipped on the stack
+   trace.
+
+4. Dynamic jumps and jumps to undefined symbols are only allowed if:
+
+   a) the jump is part of a switch statement; or
+
+   b) the jump matches sibling call semantics and the frame pointer has
+      the same value it had on function entry.
+
+   This rule is needed so that stackvalidate can reliably analyze all of
+   a function's code paths.  If a function jumps to code in another
+   file, and it's not a sibling call, stackvalidate has no way to follow
+   the jump because it only analyzes a single file at a time.
+
+5. A callable function may not execute kernel entry/exit instructions.
+   The only code which needs such instructions is kernel entry code,
+   which shouldn't be be in callable functions anyway.
+
+   This rule is just a sanity check to ensure that callable functions
+   return normally.
+
+
+Errors in .S files
+------------------
+
+If you're getting an error in a compiled .S file which you don't
+understand, first make sure that the affected code follows the above
+rules.
+
+Here are some examples of common warnings reported by stackvalidate,
+what they mean, and suggestions for how to fix them.
+
+
+1. stackvalidate: asm_file.o: func()+0x128: call without frame pointer save/setup
+
+   The func() function made a function call without first saving and/or
+   updating the frame pointer.
+
+   If func() is indeed a callable function, add proper frame pointer
+   logic using the FRAME_BEGIN and FRAME_END macros.  Otherwise, remove
+   its ELF function annotation by changing ENDPROC to END.
+
+   If you're getting this error in a .c file, see the "Errors in .c
+   files" section.
+
+
+2. stackvalidate: asm_file.o: .text+0x53: return instruction outside of a callable function
+
+   A return instruction was detected, but stackvalidate couldn't find a
+   way for a callable function to reach the instruction.
+
+   If the return instruction is inside (or reachable from) a callable
+   function, the function needs to be annotated with the ENTRY/ENDPROC
+   macros.
+
+   If you _really_ need a return instruction outside of a function, and
+   are 100% sure that it won't affect stack traces, you can tell
+   stackvalidate to ignore it.  See the "Adding exceptions" section
+   below.
+
+
+3. stackvalidate: asm_file.o: func()+0x9: function has unreachable instruction
+
+   The instruction lives inside of a callable function, but there's no
+   possible control flow path from the beginning of the function to the
+   instruction.
+
+   If the instruction is actually needed, and it's actually in a
+   callable function, ensure that its function is properly annotated
+   with ENTRY/ENDPROC.
+
+   If it's not actually in a callable function (e.g. kernel entry code),
+   change ENDPROC to END.
+
+
+4. stackvalidate: asm_file.o: func(): can't find starting instruction
+   or
+   stackvalidate: asm_file.o: func()+0x11dd: can't decode instruction
+
+   Did you put data in a text section?  If so, that can confuse
+   stackvalidate's instruction decoder.  Move the data to a more
+   appropriate section like .data or .rodata.
+
+
+5. stackvalidate: asm_file.o: func()+0x6: kernel entry/exit from callable instruction
+
+   This is a kernel entry/exit instruction like sysenter or sysret.
+   Such instructions aren't allowed in a callable function, and are most
+   likely part of the kernel entry code.
+
+   If the instruction isn't actually in a callable function, change
+   ENDPROC to END.
+
+
+6. stackvalidate: asm_file.o: func()+0x26: sibling call from callable instruction with changed frame pointer
+
+   This is a dynamic jump or a jump to an undefined symbol.
+   Stackvalidate assumed it's a sibling call and detected that the frame
+   pointer wasn't first restored to its original state.
+
+   If it's not really a sibling call, you may need to move the
+   destination code to the local file.
+
+   If the instruction is not actually in a callable function (e.g.
+   kernel entry code), change ENDPROC to END.
+
+
+7. stackvalidate: asm_file: func()+0x5c: frame pointer state mismatch
+
+   The instruction's frame pointer state is inconsistent, depending on
+   which execution path was taken to reach the instruction.
+
+   Make sure the function pushes and sets up the frame pointer (for
+   x86_64, this means rbp) at the beginning of the function and pops it
+   at the end of the function.  Also make sure that no other code in the
+   function touches the frame pointer.
+
+
+Errors in .c files
+------------------
+
+If you're getting a stackvalidate error in a compiled .c file, chances
+are the file uses an asm() statement which has a "call" instruction.  An
+asm() statement with a call instruction must declare the use of the
+stack pointer in its output operand.  For example, on x86_64:
+
+   register void *__sp asm("rsp");
+   asm volatile("call func" : "+r" (__sp));
+
+Otherwise the stack frame may not get created before the call.
+
+Another possible cause for errors in C code is if the Makefile removes
+-fno-omit-frame-pointer or adds -fomit-frame-pointer to the gcc options.
+
+Also see the above section for .S file errors for more information what
+the individual error messages mean.
+
+
+
+Adding exceptions
+-----------------
+
+If you _really_ need stackvalidate to ignore something, and are 100%
+sure that it won't affect kernel stack traces, you can tell
+stackvalidate to ignore it:
+
+- To skip validation of an instruction, use the
+  STACKVALIDATE_IGNORE_INSN macro immediately before the instruction.
+
+- To skip validation of a function, use the STACKVALIDATE_IGNORE_FUNC
+  macro.
+
+- To skip validation of a file, add "STACKVALIDATE_filename.o := n" to
+  the Makefile.
+
+- To skip validation of a directory, add "STACKVALIDATE := n" to the
+  Makefile.

index ff85821..bf4f210 100644
@@ -9688,6 +9688,14 @@ L:	[email protected]
 S:	Supported
 F:	Documentation/stable_kernel_rules.txt
 
+STACK VALIDATION
+M:	Josh Poimboeuf <[email protected]>
+S:	Supported
+F:	scripts/stackvalidate/
+F:	include/linux/stackvalidate.h
+F:	arch/x86/include/asm/stackvalidate.h
+F:	Documentation/stack-validation.txt
+
 STAGING SUBSYSTEM
 M:	Greg Kroah-Hartman <[email protected]>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

index 021f8f9..ceb4eda 100644
@@ -525,6 +525,12 @@ config HAVE_COPY_THREAD_TLS
 	  normal C parameter passing, rather than extracting the syscall
 	  argument from pt_regs.
 
+config HAVE_STACK_VALIDATION
+	bool
+	help
+	  Architecture supports the stackvalidate host tool, which adds
+	  compile-time stack metadata validation.
+
 #
 # ABI hall of shame
 #

index f37010f..50877dc 100644
@@ -151,6 +151,7 @@ config X86
 	select VIRT_TO_BUS
 	select X86_DEV_DMA_OPS			if X86_64
 	select X86_FEATURE_NAMES		if PROC_FS
+	select HAVE_STACK_VALIDATION		if X86_64
 
 config INSTRUCTION_DECODER
 	def_bool y

index 747860c..fb7135b 100644
@@ -181,9 +181,13 @@ KBUILD_CFLAGS += -fno-asynchronous-unwind-tables
 KBUILD_CFLAGS += $(mflags-y)
 KBUILD_AFLAGS += $(mflags-y)
 
-archscripts: scripts_basic
+archscripts: scripts_basic $(objtree)/arch/x86/lib/inat-tables.c
 	$(Q)$(MAKE) $(build)=arch/x86/tools relocs
 
+# this file is needed early by scripts/stackvalidate
+$(objtree)/arch/x86/lib/inat-tables.c:
+	$(Q)$(MAKE) $(build)=arch/x86/lib $@
+
 ###
 # Syscall table generation
 

index ab76b99..1c235c6 100644
@@ -332,6 +332,17 @@ config FRAME_POINTER
 	  larger and slower, but it gives very useful debugging information
 	  in case of kernel bugs. (precise oopses/stacktraces/warnings)
 
+config STACK_VALIDATION
+	bool "Enable compile-time stack metadata validation"
+	depends on HAVE_STACK_VALIDATION
+	default n
+	help
+	  Add compile-time checks to validate stack metadata, including frame
+	  pointers and back chain pointers.  This helps ensure that runtime
+	  stack traces are more reliable.
+
+	  For more information, see Documentation/stack-validation.txt.
+
 config DEBUG_FORCE_WEAK_PER_CPU
 	bool "Force weak per-cpu definitions"
 	depends on DEBUG_KERNEL

index 2016a64..c882a91 100644
@@ -37,6 +37,7 @@ subdir-y                     += mod
 subdir-$(CONFIG_SECURITY_SELINUX) += selinux
 subdir-$(CONFIG_DTC)         += dtc
 subdir-$(CONFIG_GDB_SCRIPTS) += gdb
+subdir-$(CONFIG_STACK_VALIDATION) += stackvalidate
 
 # Let clean descend into subdirs
 subdir-	+= basic kconfig package

index 01df30a..a1c5376 100644
@@ -241,10 +241,30 @@ cmd_record_mcount =						\
 	fi;
 endif
 
+ifdef CONFIG_STACK_VALIDATION
+
+__stackvalidate_obj = $(objtree)/scripts/stackvalidate/stackvalidate
+
+ifndef CONFIG_FRAME_POINTER
+nofp = --no-frame-pointer
+endif
+
+# Set STACKVALIDATE_foo.o=n to skip stack validation for a file.
+# Set STACKVALIDATE=n to skip stack validation for a directory.
+stackvalidate_obj = $(if $(patsubst n%,, \
+	$(STACKVALIDATE_$(basetarget).o)$(STACKVALIDATE)y), \
+	$(__stackvalidate_obj))
+cmd_stackvalidate = $(if $(patsubst n%,, \
+	$(STACKVALIDATE_$(basetarget).o)$(STACKVALIDATE)y), \
+	$(__stackvalidate_obj) $(nofp) "$(@)";)
+
+endif # CONFIG_STACK_VALIDATION
+
 define rule_cc_o_c
 	$(call echo-cmd,checksrc) $(cmd_checksrc)			  \
 	$(call echo-cmd,cc_o_c) $(cmd_cc_o_c);				  \
 	$(cmd_modversions)						  \
+	$(cmd_stackvalidate)						  \
 	$(call echo-cmd,record_mcount)					  \
 	$(cmd_record_mcount)						  \
 	scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,cc_o_c)' >    \

@@ -253,14 +273,23 @@ define rule_cc_o_c
 	mv -f $(dot-target).tmp $(dot-target).cmd
 endef
 
+define rule_as_o_S
+	$(call echo-cmd,as_o_S) $(cmd_as_o_S);				  \
+	$(cmd_stackvalidate)						  \
+	scripts/basic/fixdep $(depfile) $@ '$(call make-cmd,as_o_S)' >    \
+	                                              $(dot-target).tmp;  \
+	rm -f $(depfile);						  \
+	mv -f $(dot-target).tmp $(dot-target).cmd
+endef
+
 # Built-in and composite module parts
-$(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
+$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(stackvalidate_obj) FORCE
 	$(call cmd,force_checksrc)
 	$(call if_changed_rule,cc_o_c)
 
 # Single-part modules are special since we need to mark them in $(MODVERDIR)
 
-$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
+$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(stackvalidate_obj) FORCE
 	$(call cmd,force_checksrc)
 	$(call if_changed_rule,cc_o_c)
 	@{ echo $(@:.o=.ko); echo $@; } > $(MODVERDIR)/$(@F:.o=.mod)

@@ -290,8 +319,8 @@ $(obj)/%.s: $(src)/%.S FORCE
 quiet_cmd_as_o_S = AS $(quiet_modtag)  $@
 cmd_as_o_S       = $(CC) $(a_flags) -c -o $@ $<
 
-$(obj)/%.o: $(src)/%.S FORCE
-	$(call if_changed_dep,as_o_S)
+$(obj)/%.o: $(src)/%.S $(stackvalidate_obj) FORCE
+	$(call if_changed_rule,as_o_S)
 
 targets += $(real-objs-y) $(real-objs-m) $(lib-y)
 targets += $(extra-y) $(MAKECMDGOALS) $(always)

new file mode 100644
index 0000000..468c075
@@ -0,0 +1,24 @@
+hostprogs-y := stackvalidate
+always := $(hostprogs-y)
+
+stackvalidate-objs := stackvalidate.o elf.o special.o
+
+HOSTCFLAGS += -Werror
+HOSTLOADLIBES_stackvalidate := -lelf
+
+ifdef CONFIG_X86
+
+stackvalidate-objs += arch-x86.o
+
+HOSTCFLAGS_arch-x86.o := -I$(objtree)/arch/x86/lib/ \
+			 -I$(srctree)/arch/x86/include/ \
+			 -I$(srctree)/arch/x86/lib/
+
+$(obj)/arch-x86.o: $(srctree)/arch/x86/lib/insn.c \
+		   $(srctree)/arch/x86/lib/inat.c \
+		   $(srctree)/arch/x86/include/asm/inat_types.h \
+		   $(srctree)/arch/x86/include/asm/inat.h \
+		   $(srctree)/arch/x86/include/asm/insn.h \
+		   $(objtree)/arch/x86/lib/inat-tables.c
+
+endif

2.4.3

The automatic CFI generation tool seems like a bad idea to me. There's not that much assembler code in Linux, and often when new assembler code is added it is something tricky. In this case you may end up spending more time fixing the tool than just fixing the assembler.

It would be also quite bad to require people who want to add some new assembler code to learn how to fix your tool to make their assembler work.

It also wouldn't surprise me if there are some possible assembler tricks that are very hard/impossible to handle for a tool. For example how do you have alternative() style patching? (that's a generic problem with your approach BTW)

Doing some kind of CFI verifier would seem more feasible, but it would need a black/white list to override it to handle the above cases.

BTW how do handle the increasing number of JITs in the kernel?

-Andi

Really I don't see that being much of a problem. The enforced rules were probably too stringent in earlier versions of the patch set. But I relaxed the rules quite a bit and now they allow things like sibling calls, jumps to outside the function, alternatives, etc.

Generally it's easy and straightforward to follow the rules and make the tool happy. If you're trying to do something too weird, then yes the tool will complain, but IMO that's a good thing as it really makes you consider whether the weirdness (and associated complexity) is justified.

I tried to document everything an asm coder would need to know. Also I have an invested interest in keeping the tool working and useful, and I'm listed in the MAINTAINERS file. So any frustrated people will know who to yell at.

Yeah, there are definitely a lot of these tricks, but the tool can already handle them today: alternatives, jump labels, exception tables, gcc switch jump tables. It's usually a matter of parsing a special section and then treating these alternative code paths as conditional branches which need to be recursively followed and analyzed.

When such conditional branches converge, it ensures that they have the same debug state at the convergence point.

CFI generation isn't any harder than validation. Either way the tool needs to do the code analysis which involves recursively following all branches and associating a debug state with each instruction. At the end it either compares those debug states with existing CFI, or writes them as new CFI.

Also there are already some whitelist mechanisms for ignoring files, functions, or instructions.

Yeah, compile-time CFI wouldn't be applicable for code which is generated at runtime. Maybe we will need a mechanism to allow eBPF to quickly create minimal CFI-like metadata corresponding to the JIT code it generates, which can be used by stack dumping code to identify the JIT code and find the previous stack pointer on the stack.

We can also add a debug NMI handler which validates stacks periodically to ensure that stacks are always sane.

Josh

I find it somewhat ironic that you're proposing to simplifying write assembler code by making people hack some tool instead.

Perhaps I'm missing something, but for the hot patching you need some solution for this, as you rely on 100% accuracy. Right?

I guess for now it could be some kind of big reader/writer lock for JIT code and reject hot patching if something is active there.

-Andi

We'll probably need something like that eventually, once we have an in-kernel CFI unwinder. Until then, for live patching we would just need to make sure the JIT generated code honors CONFIG_FRAME_POINTER.

Yeah, maybe. Another easy way to handle it would be to bail if we can't find the CFI for a given address on a task's stack.

Josh

Tell stackvalidate to skip validation of the following code:

- boot image
- vdso image
- kexec purgatory
- realmode
- efi libstub
- scripts/mod

They all run outside the kernel's normal mode of operation and they don't affect runtime kernel stack traces, so they're free to skirt the stackvalidate rules.

---
 arch/x86/boot/Makefile                | 3 ++-
 arch/x86/boot/compressed/Makefile     | 3 ++-
 arch/x86/entry/vdso/Makefile          | 5 ++++-
 arch/x86/purgatory/Makefile           | 2 ++
 arch/x86/realmode/Makefile            | 4 +++-
 arch/x86/realmode/rm/Makefile         | 3 ++-
 drivers/firmware/efi/libstub/Makefile | 1 +
 scripts/mod/Makefile                  | 2 ++
 8 files changed, 18 insertions(+), 5 deletions(-)

index 0d553e5..d31808c 100644
@@ -14,7 +14,8 @@
 # Set it to -DSVGA_MODE=NORMAL_VGA if you just want the EGA/VGA mode.
 # The number is the same as you would ordinarily press at bootup.
 
-KASAN_SANITIZE := n
+KASAN_SANITIZE	:= n
+STACKVALIDATE	:= n
 
 SVGA_MODE	:= -DSVGA_MODE=NORMAL_VGA
 

index 0a291cd..530a46f 100644
@@ -16,7 +16,8 @@
 #	(see scripts/Makefile.lib size_append)
 #	compressed vmlinux.bin.all + u32 size of vmlinux.bin.all
 
-KASAN_SANITIZE := n
+KASAN_SANITIZE	:= n
+STACKVALIDATE	:= n
 
 targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
 	vmlinux.bin.xz vmlinux.bin.lzo vmlinux.bin.lz4

index a3d0767..b7cbf61 100644
@@ -3,7 +3,9 @@
 #
 
 KBUILD_CFLAGS += $(DISABLE_LTO)
-KASAN_SANITIZE := n
+
+KASAN_SANITIZE	:= n
+STACKVALIDATE	:= n
 
 VDSO64-$(CONFIG_X86_64)		:= y
 VDSOX32-$(CONFIG_X86_X32_ABI)	:= y

@@ -15,6 +17,7 @@ vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
 
 # files to link into kernel
 obj-y				+= vma.o
+STACKVALIDATE_vma.o		:= y
 
 # vDSO images to build
 vdso_img-$(VDSO64-y)		+= 64

index 2c835e3..a736c19 100644
@@ -1,3 +1,5 @@
+STACKVALIDATE := n
+
 purgatory-y := purgatory.o stack.o setup-x86_$(BITS).o sha256.o entry64.o string.o
 
 targets += $(purgatory-y)

index e02c2c6..7a2d4df 100644
@@ -6,7 +6,9 @@
 # for more details.
 #
 #
-KASAN_SANITIZE := n
+KASAN_SANITIZE	:= n
+STACKVALIDATE	:= n
+
 subdir- := rm
 
 obj-y += init.o

index 2730d77..d462a57 100644
@@ -6,7 +6,8 @@
 # for more details.
 #
 #
-KASAN_SANITIZE := n
+KASAN_SANITIZE	:= n
+STACKVALIDATE	:= n
 
 always := realmode.bin realmode.relocs
 

index 816dbe9..b392f3f 100644
@@ -20,6 +20,7 @@ KBUILD_CFLAGS			:= $(cflags-y) \
 
 GCOV_PROFILE			:= n
 KASAN_SANITIZE			:= n
+STACKVALIDATE			:= n
 
 lib-y				:= efi-stub-helper.o
 lib-$(CONFIG_EFI_ARMSTUB)	+= arm-stub.o fdt.o

index c11212f..374c413 100644
@@ -1,3 +1,5 @@
+STACKVALIDATE	:= n
+
 hostprogs-y	:= modpost mk_elfconfig
 always		:= $(hostprogs-y) empty.o
 

2.4.3

Add new stackvalidate ignore macros: STACKVALIDATE_IGNORE_INSN and STACKVALIDATE_IGNORE_FUNC. These can be used to tell stackvalidate to skip validation of an instruction or a function, respectively.

---
 arch/x86/include/asm/stackvalidate.h | 45 ++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/vmlinux.lds.S        |  5 +++-
 include/linux/stackvalidate.h        | 28 ++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/stackvalidate.h
 create mode 100644 include/linux/stackvalidate.h

new file mode 100644
index 0000000..95db052
@@ -0,0 +1,45 @@
+#ifndef _ASM_X86_STACKVALIDATE_H
+#define _ASM_X86_STACKVALIDATE_H
+
+#include <asm/asm.h>
+
+#ifdef __ASSEMBLY__
+
+/*
+ * This asm macro tells the stack validation script to ignore the instruction
+ * immediately after the macro.  It should only be used in special cases where
+ * you're 100% sure it won't affect the reliability of frame pointers and
+ * kernel stack traces.
+ *
+ * For more information, see Documentation/stack-validation.txt.
+ */
+.macro STACKVALIDATE_IGNORE_INSN
+#ifdef CONFIG_STACK_VALIDATION
+	.Lstackvalidate_ignore_\@:
+	.pushsection __stackvalidate_ignore_insn, "a"
+	_ASM_ALIGN
+	.long .Lstackvalidate_ignore_\@ - .
+	.popsection
+#endif
+.endm
+
+#else /* !__ASSEMBLY__ */
+
+#ifdef CONFIG_STACK_VALIDATION
+
+#define STACKVALIDATE_IGNORE_INSN				\
+	"1:\n"							\
+	".pushsection __stackvalidate_ignore_insn, \"a\"\n"	\
+	_ASM_ALIGN "\n"						\
+	".long 1b - .\n"					\
+	".popsection\n"
+
+#else /* !CONFIG_STACK_VALIDATION */
+
+#define STACKVALIDATE_IGNORE_INSN ""
+
+#endif /* CONFIG_STACK_VALIDATION */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_X86_STACKVALIDATE_H */

index 00bf300..f2f8d7a 100644
@@ -332,7 +332,10 @@ SECTIONS
 
 	/* Sections to be discarded */
 	DISCARDS
-	/DISCARD/ : { *(.eh_frame) }
+	/DISCARD/ : {
+		*(.eh_frame)
+		*(__stackvalidate_ignore_*)
+	}
 }
 
 

new file mode 100644
index 0000000..4ae242c
@@ -0,0 +1,28 @@
+#ifndef _LINUX_STACKVALIDATE_H
+#define _LINUX_STACKVALIDATE_H
+
+#include <asm/stackvalidate.h>
+
+#ifndef __ASSEMBLY__
+
+#ifdef CONFIG_STACK_VALIDATION
+/*
+ * This C macro tells the stack validation script to ignore the function.  It
+ * should only be used in special cases where you're 100% sure it won't affect
+ * the reliability of frame pointers and kernel stack traces.
+ *
+ * For more information, see Documentation/stack-validation.txt.
+ */
+#define STACKVALIDATE_IGNORE_FUNC(_func) \
+	static void __used __section(__stackvalidate_ignore_func) \
+		*__stackvalidate_ignore_func_##_func = _func
+
+#else /* !CONFIG_STACK_VALIDATION */
+
+#define STACKVALIDATE_IGNORE_FUNC(_func)
+
+#endif /* CONFIG_STACK_VALIDATION */
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* _LINUX_STACKVALIDATE_H */

2.4.3

If a hypercall is inlined at the beginning of a function, gcc can insert the call instruction before setting up a stack frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace.

Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by listing the stack pointer as an output operand for the hypercall inline asm statements.

---
 arch/x86/include/asm/xen/hypercall.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index ca08a27..4488fbc 100644
@@ -110,9 +110,10 @@ extern struct { char _entry[32]; } hypercall_page[];
 	register unsigned long __arg2 asm(__HYPERCALL_ARG2REG) = __arg2; \
 	register unsigned long __arg3 asm(__HYPERCALL_ARG3REG) = __arg3; \
 	register unsigned long __arg4 asm(__HYPERCALL_ARG4REG) = __arg4; \
-	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5;
+	register unsigned long __arg5 asm(__HYPERCALL_ARG5REG) = __arg5; \
+	register void *__sp asm(_ASM_SP);
 
-#define __HYPERCALL_0PARAM	"=r" (__res)
+#define __HYPERCALL_0PARAM	"=r" (__res), "+r" (__sp)
 #define __HYPERCALL_1PARAM	__HYPERCALL_0PARAM, "+r" (__arg1)
 #define __HYPERCALL_2PARAM	__HYPERCALL_1PARAM, "+r" (__arg2)
 #define __HYPERCALL_3PARAM	__HYPERCALL_2PARAM, "+r" (__arg3)

2.4.3

If a PVOP call macro is inlined at the beginning of a function, gcc can insert the call instruction before setting up a stack frame, which breaks frame pointer convention if CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace.

Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by listing the stack pointer as an output operand for the PVOP inline asm statements.

---
 arch/x86/include/asm/paravirt_types.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

index ce029e4..1812294 100644
@@ -497,8 +497,9 @@ int paravirt_disable_iospace(void);
  * makes sure the incoming and outgoing types are always correct.
  */
 #ifdef CONFIG_X86_32
-#define PVOP_VCALL_ARGS				\
-	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx
+#define PVOP_VCALL_ARGS							\
+	unsigned long __eax = __eax, __edx = __edx, __ecx = __ecx;	\
+	register void *__sp asm("esp")
 #define PVOP_CALL_ARGS			PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"a" ((unsigned long)(x))

@@ -516,9 +517,10 @@ int paravirt_disable_iospace(void);
 #define VEXTRA_CLOBBERS
 #else  /* CONFIG_X86_64 */
 /* [re]ax isn't an arg, but the return val */
-#define PVOP_VCALL_ARGS					\
-	unsigned long __edi = __edi, __esi = __esi,	\
-		__edx = __edx, __ecx = __ecx, __eax = __eax
+#define PVOP_VCALL_ARGS						\
+	unsigned long __edi = __edi, __esi = __esi,		\
+		__edx = __edx, __ecx = __ecx, __eax = __eax;	\
+	register void *__sp asm("rsp")
 #define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))

@@ -557,7 +559,7 @@ int paravirt_disable_iospace(void);
 			asm volatile(pre				\
 				     paravirt_alt(PARAVIRT_CALL)	\
 				     post				\
-				     : call_clbr			\
+				     : "+r" (__sp), call_clbr		\
 				     : paravirt_type(op),		\
 				       paravirt_clobber(clbr),		\
 				       ##__VA_ARGS__			\

@@ -567,7 +569,7 @@ int paravirt_disable_iospace(void);
 			asm volatile(pre				\
 				     paravirt_alt(PARAVIRT_CALL)	\
 				     post				\
-				     : call_clbr			\
+				     : "+r" (__sp), call_clbr		\
 				     : paravirt_type(op),		\
 				       paravirt_clobber(clbr),		\
 				       ##__VA_ARGS__			\

@@ -594,7 +596,7 @@ int paravirt_disable_iospace(void);
 		asm volatile(pre					\
 			     paravirt_alt(PARAVIRT_CALL)		\
 			     post					\
-			     : call_clbr				\
+			     : "+r" (__sp), call_clbr			\
 			     : paravirt_type(op),			\
 			       paravirt_clobber(clbr),			\
 			       ##__VA_ARGS__				\

2.4.3

A function created with the PV_CALLEE_SAVE_REGS_THUNK macro doesn't set up a new stack frame before the call instruction, which breaks frame pointer convention if CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace. Also, the thunk functions aren't annotated as ELF callable functions.

Create a stack frame when CONFIG_FRAME_POINTER is enabled and add the ELF function type.

---
 arch/x86/include/asm/paravirt.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index 10d0596..23a382b 100644
@@ -13,6 +13,7 @@
 #include <linux/bug.h>
 #include <linux/types.h>
 #include <linux/cpumask.h>
+#include <asm/frame.h>
 
 static inline int paravirt_enabled(void)
 {

@@ -776,15 +777,19 @@ static __always_inline void __ticket_unlock_kick(struct arch_spinlock *lock,
  * call. The return value in rax/eax will not be saved, even for void
  * functions.
  */
+#define PV_THUNK_NAME(func) "__raw_callee_save_" #func
 #define PV_CALLEE_SAVE_REGS_THUNK(func)					\
 	extern typeof(func) __raw_callee_save_##func;			\
 									\
 	asm(".pushsection .text;"					\
-	    ".globl __raw_callee_save_" #func " ; "			\
-	    "__raw_callee_save_" #func ": "				\
+	    ".globl " PV_THUNK_NAME(func) ";"				\
+	    ".type " PV_THUNK_NAME(func) ", @function;"			\
+	    PV_THUNK_NAME(func) ":"					\
+	    FRAME_BEGIN							\
 	    PV_SAVE_ALL_CALLER_REGS					\
 	    "call " #func ";"						\
 	    PV_RESTORE_ALL_CALLER_REGS					\
+	    FRAME_END							\
 	    "ret;"							\
 	    ".popsection")
 

2.4.3

vide() is a callable function, but is missing the ELF function type, which confuses tools like stackvalidate.

Properly annotate it to be a callable function. The generated code is unchanged.

---
 arch/x86/kernel/cpu/amd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 4a70fc6..c010de5 100644
@@ -75,7 +75,10 @@ static inline int wrmsrl_amd_safe(unsigned msr, unsigned long long val)
  */
 
 extern __visible void vide(void);
-__asm__(".globl vide\n\t.align 4\nvide: ret");
+__asm__(".globl vide\n"
+	".type vide, @function\n"
+	".align 4\n"
+	"vide: ret\n");
 
 static void init_amd_k5(struct cpuinfo_x86 *c)
 {

2.4.3

stackvalidate reports a false positive warning for the ljmp instruction in machine_real_restart(). Normally, ljmp isn't allowed in a function, but this is a special case where it's jumping into real mode.

Add the jumps to a whitelist which tells stackvalidate to ignore them.

---
 arch/x86/kernel/reboot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 86db4bc..0931d87 100644
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/tboot.h>
 #include <linux/delay.h>
+#include <linux/stackvalidate.h>
 #include <acpi/reboot.h>
 #include <asm/io.h>
 #include <asm/apic.h>

@@ -97,11 +98,13 @@ void __noreturn machine_real_restart(unsigned int type)
 
 	/* Jump to the identity-mapped low memory code */
 #ifdef CONFIG_X86_32
-	asm volatile("jmpl *%0" : :
+	asm volatile(STACKVALIDATE_IGNORE_INSN
+		     "jmpl *%0;" : :
 		     "rm" (real_mode_header->machine_real_restart_asm),
 		     "a" (type));
 #else
-	asm volatile("ljmpl *%0" : :
+	asm volatile(STACKVALIDATE_IGNORE_INSN
+		     "ljmpl *%0" : :
 		     "m" (real_mode_header->machine_real_restart_asm),
 		     "D" (type));
 #endif

2.4.3

stackvalidate reports the following false positive warnings:

stackvalidate: arch/x86/xen/enlighten.o: xen_cpuid()+0x41: can't find jump dest instruction at .text+0x108
stackvalidate: arch/x86/xen/enlighten.o: xen_setup_gdt.constprop.23()+0x2e: kernel entry/exit from callable instruction

The first warning is due to xen_cpuid()'s use of XEN_EMULATE_PREFIX to insert some fake instructions which stackvalidate doesn't know how to decode.

The second warning is due to xen_setup_gdt()'s use of an lret instruction, which stackvalidate normally doesn't allow in callable functions. But this seems to be a valid use of the instruction.

Add both functions to the stackvalidate whitelist.

---
 arch/x86/xen/enlighten.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index d9cfa45..4ca89ac 100644
@@ -32,6 +32,7 @@
 #include <linux/gfp.h>
 #include <linux/memblock.h>
 #include <linux/edd.h>
+#include <linux/stackvalidate.h>
 
 #include <xen/xen.h>
 #include <xen/events.h>

@@ -345,8 +346,8 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx,
 	*cx &= maskecx;
 	*cx |= setecx;
 	*dx &= maskedx;
-
 }
+STACKVALIDATE_IGNORE_FUNC(xen_cpuid);
 
 static bool __init xen_check_mwait(void)
 {

@@ -1443,6 +1444,7 @@ static void __ref xen_setup_gdt(int cpu)
 	pv_cpu_ops.write_gdt_entry = xen_write_gdt_entry;
 	pv_cpu_ops.load_gdt = xen_load_gdt;
 }
+STACKVALIDATE_IGNORE_FUNC(xen_setup_gdt);
 
 #ifdef CONFIG_XEN_PVH
 /*

2.4.3

aesni-intel_asm.S has several callable non-leaf functions which don't honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create stack frames for them when CONFIG_FRAME_POINTER is enabled.

---
 arch/x86/crypto/aesni-intel_asm.S | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

index 6bd2c6c..289ef12 100644
@@ -31,6 +31,7 @@
 
 #include <linux/linkage.h>
 #include <asm/inst.h>
+#include <asm/frame.h>
 
 /*
  * The following macros are used to move an (un)aligned 16 byte value to/from

@@ -1800,6 +1801,7 @@ ENDPROC(_key_expansion_256b)
  *                   unsigned int key_len)
  */
 ENTRY(aesni_set_key)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl KEYP
 	movl 8(%esp), KEYP		# ctx

@@ -1905,6 +1907,7 @@ ENTRY(aesni_set_key)
 #ifndef __x86_64__
 	popl KEYP
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_set_key)
 

@@ -1912,6 +1915,7 @@ ENDPROC(aesni_set_key)
  * void aesni_enc(struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
  */
 ENTRY(aesni_enc)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl KEYP
 	pushl KLEN

@@ -1927,6 +1931,7 @@ ENTRY(aesni_enc)
 	popl KLEN
 	popl KEYP
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_enc)
 

@@ -2101,6 +2106,7 @@ ENDPROC(_aesni_enc4)
  * void aesni_dec (struct crypto_aes_ctx *ctx, u8 *dst, const u8 *src)
  */
 ENTRY(aesni_dec)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl KEYP
 	pushl KLEN

@@ -2117,6 +2123,7 @@ ENTRY(aesni_dec)
 	popl KLEN
 	popl KEYP
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_dec)
 

@@ -2292,6 +2299,7 @@ ENDPROC(_aesni_dec4)
  *		      size_t len)
  */
 ENTRY(aesni_ecb_enc)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl LEN
 	pushl KEYP

@@ -2342,6 +2350,7 @@ ENTRY(aesni_ecb_enc)
 	popl KEYP
 	popl LEN
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_ecb_enc)
 

@@ -2350,6 +2359,7 @@ ENDPROC(aesni_ecb_enc)
  *		      size_t len);
  */
 ENTRY(aesni_ecb_dec)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl LEN
 	pushl KEYP

@@ -2401,6 +2411,7 @@ ENTRY(aesni_ecb_dec)
 	popl KEYP
 	popl LEN
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_ecb_dec)
 

@@ -2409,6 +2420,7 @@ ENDPROC(aesni_ecb_dec)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_cbc_enc)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl IVP
 	pushl LEN

@@ -2443,6 +2455,7 @@ ENTRY(aesni_cbc_enc)
 	popl LEN
 	popl IVP
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_cbc_enc)
 

@@ -2451,6 +2464,7 @@ ENDPROC(aesni_cbc_enc)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_cbc_dec)
+	FRAME_BEGIN
 #ifndef __x86_64__
 	pushl IVP
 	pushl LEN

@@ -2534,6 +2548,7 @@ ENTRY(aesni_cbc_dec)
 	popl LEN
 	popl IVP
 #endif
+	FRAME_END
 	ret
 ENDPROC(aesni_cbc_dec)
 

@@ -2598,6 +2613,7 @@ ENDPROC(_aesni_inc)
  *		      size_t len, u8 *iv)
  */
 ENTRY(aesni_ctr_enc)
+	FRAME_BEGIN
 	cmp $16, LEN
 	jb .Lctr_enc_just_ret
 	mov 480(KEYP), KLEN

@@ -2651,6 +2667,7 @@ ENTRY(aesni_ctr_enc)
 .Lctr_enc_ret:
 	movups IV, (IVP)
 .Lctr_enc_just_ret:
+	FRAME_END
 	ret
 ENDPROC(aesni_ctr_enc)
 

@@ -2677,6 +2694,7 @@ ENDPROC(aesni_ctr_enc)
  *			 bool enc, u8 *iv)
  */
 ENTRY(aesni_xts_crypt8)
+	FRAME_BEGIN
 	cmpb $0, %cl
 	movl $0, %ecx
 	movl $240, %r10d

@@ -2777,6 +2795,7 @@ ENTRY(aesni_xts_crypt8)
 	pxor INC, STATE4
 	movdqu STATE4, 0x70(OUTP)
 
+	FRAME_END
 	ret
 ENDPROC(aesni_xts_crypt8)
 

2.4.3

stackvalidate reports the following warning:

stackvalidate: arch/x86/crypto/aesni-intel_asm.o: _aesni_inc_init(): can't find starting instruction

stackvalidate gets confused when it tries to disassemble the following data in the .text section:

.Lbswap_mask:

          .byte 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0

Move it to .rodata which is a more appropriate section for read-only data.

---
 arch/x86/crypto/aesni-intel_asm.S | 2 ++
 1 file changed, 2 insertions(+)

index 289ef12..18f0ced 100644
@@ -2553,9 +2553,11 @@ ENTRY(aesni_cbc_dec)
 ENDPROC(aesni_cbc_dec)
 
 #ifdef __x86_64__
+.pushsection .rodata
 .align 16
 .Lbswap_mask:
 	.byte 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0
+.popsection
 
 /*
  * _aesni_inc_init:	internal ABI

2.4.3

stackvalidate reports the following warning:

stackvalidate: arch/x86/crypto/crc32c-pcl-intel-asm_64.o: crc_pcl()+0x11dd: can't decode instruction

It gets confused when trying to decode jump_table data. Move jump_table to the .rodata section which is a more appropriate home for read-only data.

---
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 225be06..dc05f01 100644
@@ -170,8 +170,8 @@ continue_block:
 	## branch into array
 	lea	jump_table(%rip), bufp
 	movzxw  (bufp, %rax, 2), len
-	offset=crc_array-jump_table
-	lea     offset(bufp, len, 1), bufp
+	lea	crc_array(%rip), bufp
+	lea     (bufp, len, 1), bufp
 	jmp     *bufp
 
 	################################################################

@@ -310,7 +310,9 @@ do_return:
 	popq    %rdi
 	popq    %rbx
         ret
+ENDPROC(crc_pcl)
 
+.section	.rodata, "a", %progbits
         ################################################################
         ## jump table        Table is 129 entries x 2 bytes each
         ################################################################

@@ -324,13 +326,11 @@ JMPTBL_ENTRY %i
 	i=i+1
 .endr
 
-ENDPROC(crc_pcl)
 
 	################################################################
 	## PCLMULQDQ tables
 	## Table is 128 entries x 2 words (8 bytes) each
 	################################################################
-.section	.rotata, "a", %progbits
 .align 8
 K_table:
 	.long 0x493c7d27, 0x00000001

2.4.3

clmul_ghash_mul() and clmul_ghash_update() are callable non-leaf functions which don't honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create stack frames for them when CONFIG_FRAME_POINTER is enabled.

---
 arch/x86/crypto/ghash-clmulni-intel_asm.S | 5 +++++
 1 file changed, 5 insertions(+)

index 5d1e007..eed55c8 100644
@@ -18,6 +18,7 @@
 
 #include <linux/linkage.h>
 #include <asm/inst.h>
+#include <asm/frame.h>
 
 .data
 

@@ -94,6 +95,7 @@ ENDPROC(__clmul_gf128mul_ble)
 
 /* void clmul_ghash_mul(char *dst, const u128 *shash) */
 ENTRY(clmul_ghash_mul)
+	FRAME_BEGIN
 	movups (%rdi), DATA
 	movups (%rsi), SHASH
 	movaps .Lbswap_mask, BSWAP

@@ -101,6 +103,7 @@ ENTRY(clmul_ghash_mul)
 	call __clmul_gf128mul_ble
 	PSHUFB_XMM BSWAP DATA
 	movups DATA, (%rdi)
+	FRAME_END
 	ret
 ENDPROC(clmul_ghash_mul)
 

@@ -109,6 +112,7 @@ ENDPROC(clmul_ghash_mul)
  *			   const u128 *shash);
  */
 ENTRY(clmul_ghash_update)
+	FRAME_BEGIN
 	cmp $16, %rdx
 	jb .Lupdate_just_ret	# check length
 	movaps .Lbswap_mask, BSWAP

@@ -128,5 +132,6 @@ ENTRY(clmul_ghash_update)
 	PSHUFB_XMM BSWAP DATA
 	movups DATA, (%rdi)
 .Lupdate_just_ret:
+	FRAME_END
 	ret
 ENDPROC(clmul_ghash_update)

2.4.3

Thunk functions are callable non-leaf functions that don't honor CONFIG_FRAME_POINTER, which can result in bad stack traces. Also they aren't annotated as ELF callable functions which can confuse tooling.

Create stack frames for them when CONFIG_FRAME_POINTER is enabled and add the ELF function type.

---
 arch/x86/entry/thunk_64.S | 4 ++++
 1 file changed, 4 insertions(+)

index efb2b93..98df1fa 100644
@@ -8,11 +8,14 @@
 #include <linux/linkage.h>
 #include "calling.h"
 #include <asm/asm.h>
+#include <asm/frame.h>
 
 	/* rdi:	arg1 ... normal C conventions. rax is saved/restored. */
 	.macro THUNK name, func, put_ret_addr_in_rdi=0
 	.globl \name
+	.type \name, @function
 \name:
+	FRAME_BEGIN
 
 	/* this one pushes 9 elems, the next one would be %rIP */
 	pushq %rdi

@@ -62,6 +65,7 @@ restore:
 	popq %rdx
 	popq %rsi
 	popq %rdi
+	FRAME_END
 	ret
 	_ASM_NOKPROBE(restore)
 #endif

2.4.3

do_suspend_lowlevel() is a callable non-leaf function which doesn't honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create a stack frame for it when CONFIG_FRAME_POINTER is enabled.

---
 arch/x86/kernel/acpi/wakeup_64.S | 3 +++
 1 file changed, 3 insertions(+)

index 8c35df4..169963f 100644
@@ -5,6 +5,7 @@
 #include <asm/page_types.h>
 #include <asm/msr.h>
 #include <asm/asm-offsets.h>
+#include <asm/frame.h>
 
 # Copyright 2003 Pavel Machek <[email protected]>, distribute under GPLv2
 

@@ -39,6 +40,7 @@ bogus_64_magic:
 	jmp	bogus_64_magic
 
 ENTRY(do_suspend_lowlevel)
+	FRAME_BEGIN
 	subq	$8, %rsp
 	xorl	%eax, %eax
 	call	save_processor_state

@@ -109,6 +111,7 @@ ENTRY(do_suspend_lowlevel)
 
 	xorl	%eax, %eax
 	addq	$8, %rsp
+	FRAME_END
 	jmp	restore_processor_state
 ENDPROC(do_suspend_lowlevel)
 

2.4.3

rwsem.S has several callable non-leaf functions which don't honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create stack frames for them when CONFIG_FRAME_POINTER is enabled.

---
 arch/x86/lib/rwsem.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

index 40027db..be110ef 100644
@@ -15,6 +15,7 @@
 
 #include <linux/linkage.h>
 #include <asm/alternative-asm.h>
+#include <asm/frame.h>
 
 #define __ASM_HALF_REG(reg)	__ASM_SEL(reg, e##reg)
 #define __ASM_HALF_SIZE(inst)	__ASM_SEL(inst##w, inst##l)

@@ -84,24 +85,29 @@
 
 /* Fix up special calling conventions */
 ENTRY(call_rwsem_down_read_failed)
+	FRAME_BEGIN
 	save_common_regs
 	__ASM_SIZE(push,) %__ASM_REG(dx)
 	movq %rax,%rdi
 	call rwsem_down_read_failed
 	__ASM_SIZE(pop,) %__ASM_REG(dx)
 	restore_common_regs
+	FRAME_END
 	ret
 ENDPROC(call_rwsem_down_read_failed)
 
 ENTRY(call_rwsem_down_write_failed)
+	FRAME_BEGIN
 	save_common_regs
 	movq %rax,%rdi
 	call rwsem_down_write_failed
 	restore_common_regs
+	FRAME_END
 	ret
 ENDPROC(call_rwsem_down_write_failed)
 
 ENTRY(call_rwsem_wake)
+	FRAME_BEGIN
 	/* do nothing if still outstanding active readers */
 	__ASM_HALF_SIZE(dec) %__ASM_HALF_REG(dx)
 	jnz 1f

@@ -109,15 +115,18 @@ ENTRY(call_rwsem_wake)
 	movq %rax,%rdi
 	call rwsem_wake
 	restore_common_regs
-1:	ret
+1:	FRAME_END
+	ret
 ENDPROC(call_rwsem_wake)
 
 ENTRY(call_rwsem_downgrade_wake)
+	FRAME_BEGIN
 	save_common_regs
 	__ASM_SIZE(push,) %__ASM_REG(dx)
 	movq %rax,%rdi
 	call rwsem_downgrade_wake
 	__ASM_SIZE(pop,) %__ASM_REG(dx)
 	restore_common_regs
+	FRAME_END
 	ret
 ENDPROC(call_rwsem_downgrade_wake)

2.4.3

efi_call() is a callable non-leaf function which doesn't honor CONFIG_FRAME_POINTER, which can result in bad stack traces.

Create a stack frame for it when CONFIG_FRAME_POINTER is enabled.

---
 arch/x86/platform/efi/efi_stub_64.S | 3 +++
 1 file changed, 3 insertions(+)

index 86d0f9e..0df2dcc 100644
@@ -11,6 +11,7 @@
 #include <asm/msr.h>
 #include <asm/processor-flags.h>
 #include <asm/page_types.h>
+#include <asm/frame.h>
 
 #define SAVE_XMM			\
 	mov %rsp, %rax;			\

@@ -74,6 +75,7 @@
 	.endm
 
 ENTRY(efi_call)
+	FRAME_BEGIN
 	SAVE_XMM
 	mov (%rsp), %rax
 	mov 8(%rax), %rax

@@ -88,6 +90,7 @@ ENTRY(efi_call)
 	RESTORE_PGT
 	addq $48, %rsp
 	RESTORE_XMM
+	FRAME_END
 	ret
 ENDPROC(efi_call)
 

2.4.3

swsusp_arch_suspend() and restore_registers() are callable non-leaf functions which don't honor CONFIG_FRAME_POINTER, which can result in bad stack traces. Also they aren't annotated as ELF callable functions which can confuse tooling.

Create a stack frame for them when CONFIG_FRAME_POINTER is enabled and give them proper ELF function annotations.

---
 arch/x86/power/hibernate_asm_64.S | 7 +++++++
 1 file changed, 7 insertions(+)

index e2386cb..4400a43 100644
@@ -21,8 +21,10 @@
 #include <asm/page_types.h>
 #include <asm/asm-offsets.h>
 #include <asm/processor-flags.h>
+#include <asm/frame.h>
 
 ENTRY(swsusp_arch_suspend)
+	FRAME_BEGIN
 	movq	$saved_context, %rax
 	movq	%rsp, pt_regs_sp(%rax)
 	movq	%rbp, pt_regs_bp(%rax)

@@ -50,7 +52,9 @@ ENTRY(swsusp_arch_suspend)
 	movq	%rax, restore_cr3(%rip)
 
 	call swsusp_save
+	FRAME_END
 	ret
+ENDPROC(swsusp_arch_suspend)
 
 ENTRY(restore_image)
 	/* switch to temporary page tables */

@@ -107,6 +111,7 @@ ENTRY(core_restore_code)
 	 */
 
 ENTRY(restore_registers)
+	FRAME_BEGIN
 	/* go back to the original page tables */
 	movq    %rbx, %cr3
 

@@ -147,4 +152,6 @@ ENTRY(restore_registers)
 	/* tell the hibernation core that we've just restored the memory */
 	movq	%rax, in_suspend(%rip)
 
+	FRAME_END
 	ret
+ENDPROC(restore_registers)

2.4.3

Btw., would you be against putting scripts/stackvalidate/ into tools/stackvalidate/ ?

That way it would be more generic, and could be used in a standalone fashion as well I suspect. It would not change any of your current usage of it.

(The other Kbuild glue changes still need to be in scripts/.)

Thanks,

Ingo

I like that idea. It could indeed be useful for any other projects which have asm code. In fact the musl project has been trying to solve similar problems with their CFI generation awk script.

It does have some kernel-isms related to the reading of special sections (.altinstructions, __jump_table, etc). But that should be harmless for non-kernel code and could easily be factored out with a compile-time or cmdline flag if needed.

BTW, since I'm planning on adding a CFI generation option to this tool, I wonder if "stackvalidate" is a misnomer. I can't think of anything better. Any naming ideas are welcome :-)

Josh

I'd suggest 'debuginfo' and a Git-alike set of subcommands:

debuginfo help
debuginfo check
debuginfo check framepointers
debuginfo check cfi

Such a command scheme keeps open various future commands, such as:

debuginfo show
debuginfo size
debuginfo compress
debuginfo strip

etc.

I also checked that there does not appear to be such a Linux tool with such a name at the moment.

Thanks,

Ingo

IMO every name which is not googlable (like this one, because it already has other meaning) is bad.

I always hated how 'crash' utlity is called, exactly because of this.

Jiri Kosina
SUSE Labs

My feeling is that the subcommand model wouldn't fit this tool very well. Its core functionality is to analyze code paths -- which it does in a single pass, regardless of whether it's checking frame pointers, checking CFI, generating CFI, or some combination. Splitting it up into subcommands would mean having to repeat the same code analysis pass multiple times unnecessarily.

Also, since it's really focused on stack-related code path analysis, I wouldn't expect it to be branching out into other debuginfo-related tasks.

As far as naming goes, "debuginfo" usually refers to DWARF metadata. But this tool isn't DWARF-specific, so that would be confusing IMO. I also agree with Jiri that re-using a generic name which already has another meaning would add to the confusion.

The goal of this tool is to fix stack traces, so how about "stackfix"?

usage: stackfix [options] <file.o>
-f, --frame-pointers Validate frame pointer usage
-c, --cfi Validate CFI if it exists
-g, --cfi-gen Generate CFI if it doesn't already exist

The options could all be combinable.

Josh

Huh?

The subcommand approach is a user UI that does not limit the tool in any way: you are free to provide subcommands that combine more atomic functionality - similarly to how Git provides a 'git pull' subcommand that is a combination of 'fetch' and 'merge' steps.

In this case it would be a simple:

debuginfo check all

to check everything. You can also make the selection of debuginfo components to check a regular option, not a subcommand.

The important step is to not limit the tool to 'checking' only, if there's reasonable other tasks it could perform:

- For example in the future we might want to sanity check the dwarf debuginfo whether it's all well suited for kernel probing.

- And if the tool is doing such a good job analyzing stacks, why not extend it trivially to print max stack backtrace estimations?

etc. By limiting the name at inception unreasonably you make all these things less obvious to add.

The tool should obviously not be named 'kitchensink' just for future extensability, but at least don't limit it to stacks and checking only ...

Sigh, please don't limit the tool's purpose with its name ...

and choose a better name! :-) The tool does not 'fix' anything, and the normal case would be for it to produce no warnings. I.e. it's not a fixing, it's a checking tool.

So something like 'stackcheck' or 'stackinfo' - but I really think 'debuginfo' is better, because for more complex kernel image operations like live patching the kernel has to double check the untrusted debuginfo that compilers barf out ...

Frame pointers are simply a legacy debuginfo variant, one that the kernel happens to use as its primary debuginfo. I don't think it's confusing at all, especially if any dwarf logic is added ...

Thanks,

Ingo

Sure, but it doesn't scale if *all* the subcommands are combinable. For n subcommands which can be combined, you'd need (2^n - 1) total subcommands to cover all possible combinations. In that case, subcommands would be much more unwieldy than just having n flags that can be easily combined.

This is an important point because I think any hypothetical future options would be likely to be combinable if they take advantage of the tool's main functionality, which is walking all the code paths. If they don't take advantage of that, they should probably be in a separate tool anyway.

The reason I proposed a name change is that it will soon do *more* than just checking. It will also do CFI generation by modifying the object file.

What subcommand would you suggest for the following?

- do frame pointer validation; and

- if CFI exists, do CFI validation, else do CFI generation.

Yes, and this is planned soon.

Sure, we could do that.

But note these examples are still related to stacks, so having "stack" in the name of the tool wouldn't be limiting (for these examples at least).

I proposed the "fix" in "stackfix" because it will do more than just
checking: it will also be able to modify the object file (as I describe above). And "stack" because thus far the proposed scope of the tool is strictly related to stacks.

I think "debuginfo" is limiting in its own way. The core functionality of the tool is to analyze all possible code paths, which isn't directly related to debuginfo. We might want to do other kinds of code path analysis which are unrelated to debuginfo.

For example, the tool could have a replacement for "make checkstack", which generates a list of functions which are stack hogs. That has nothing to do with debuginfo.

(And note this is a further example of why subcommands are not a good fit. We would want to be able to combine this option with the others without needing an exponential growth in the number of subcommands.)

Josh

I think there's some misunderstanding here. Initially you only need a single subcommand, 'check'. With that if the main subcommand for checking is:

debuginfo check

then you can still add options after the 'check' subcommand if you think it's more intuitive - or list them as sub-subcommands - which is generally more intuitive to humans:

debuginfo check cfi fp

or:

debuginfo check all

See below for more complex examples:

It's a simple option string namespace - look at how tools/perf/ is using it, it's very flexible.

The main functionality here is to fix up the CFI info, so I'd name it:

debuginfo fix cfi

where the 'fix' subcommand would use functionality from the 'check' subcommand to see whether there's CFI info present (and if yes, sanity check it and warn if it's not good).

perf does this all the time: for example 'perf top', 'perf report' and 'perf annotate' deeply share functionality. Since under the hood it's all one single binary, it's all very easy and intuitive to do.

Absolutely, I'd name it 'debuginfo' at minimum to not unnecessarily limit things at the inception of the tool with 'stackfix'.

So if you can think of an even more generic name than 'debuginfo', that would be even better - what I objected to was the limiting 'stackfix' name.

For example 'binary' might work well too, here's a few mockup subcommands:

   binary check fp                   # checks framepointers in a binary
   binary check all                  # checks everything it can in a binary
   binary generate cfi               # generates CFI info
   binary ls                         # prints section sizes
   binary compress                   # strip out NOPs and other padding from a binary if possible

(But 'fix' instead of 'generate' would work as well.)

Note how intuitive the wording it, it's almost a free flowing English sentence.

That's actually a powerful example of how subcommands would work naturally:

binary check cfi fp stacksize

see how it's checking various aspects of an executable?

Note that per Git and perf option parsing only the first word after 'debuginfo' is a subcommand. The 'cfi fp stacksize' options will be interpreted within the 'check' subcommand.

and note how writing:

binary check help

or:

binary check -h

will give the user context sensitive help. It won't print any help text about any 'fix' functionality, generating CFI information for example.

I still don't see where we'd (ever!) get such exponential growth of subcommands. If you do it right and structure it into an intuitive interface, it won't happen.

Thanks,

Ingo

I still don't see how that would work. Here's how we would achieve that example with my latest proposal:

  stacktool --check-frame-pointer --check-cfi --gen-cfi

Notice how clear it is *exactly* what the tool is doing.

On the other hand, "debuginfo fix cfi" hides two of the three tasks.

It's also confusing and inconsistent: use the "check" subcommand for checking, but use the "fix" subcommand for both checking *and* generation? That's far from obvious for the user.

Further, with my proposal all three options can be added and removed in various combinations. So you can do things like:

stacktool --gen-cfi
stacktool --check-frame-pointer --gen-cfi
stacktool --check-cfi --gen-cfi

How would you do those with subcommands?

Actually I would use the same argument *against* debuginfo. We've already identified a realistic potential usage that's unrelated to debug
info: stack size checking. "debuginfo" is too limiting for that case.

OTOH, I don't think we've yet conceived of any non-stack-related uses of the tool (other than some suggestions which are outside the scope of its core functionality of analyzing all code paths). So I really think something with "stack" in the name would be appropriate.

That's way too generic IMO. A tool named "binary" could do practically anything. The name doesn't give the user any idea about what it actually does.

Also the proposed subcommands "ls" and "compress" have nothing to do with recursive code path analysis and really belong in separate tools.

I really don't see any benefit to making a big monolithic tool which does all things related to ELF/DWARF binary analysis. There are already a lot of good tools out there which do a lot of those things.

I don't see any good reason to grow this tool beyond its core functionality of recursive code analysis. At some point we have to draw a reasonable line in the sand about what it will eventually do. Otherwise we should just call it "tool" ;-)

Josh