All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	live-patching@vger.kernel.org, Michal Marek <mmarek@suse.cz>,
	Peter Zijlstra <peterz@infradead.org>,
	Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andi Kleen <andi@firstfloor.org>, Pedro Alves <palves@redhat.com>,
	Namhyung Kim <namhyung@gmail.com>,
	Bernd Petrovitsch <bernd@petrovitsch.priv.at>,
	Chris J Arges <chris.j.arges@canonical.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jiri Slaby <jslaby@suse.cz>,
	Arnaldo Carvalho de Melo <acme@kernel.org>
Subject: Re: [PATCH v15 06/25] x86/stacktool: Compile-time stack metadata validation
Date: Tue, 19 Jan 2016 13:02:55 +0100	[thread overview]
Message-ID: <20160119120255.GA18533@gmail.com> (raw)
In-Reply-To: <ca128e3391e1f32fe5a9a92590d179d4dbad856c.1450442274.git.jpoimboe@redhat.com>


* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> This adds a CONFIG_STACK_VALIDATION option which enables a host tool
> named stacktool 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 stacktool 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.  For more details about what is planned,
>    see tools/stacktool/Documentation/stack-validation.txt.
> 
> c) Higher live patching compatibility rate
> 
>    This is not yet implemented.  For more details about what is planned,
>    see tools/stacktool/Documentation/stack-validation.txt.
> 
> To achieve the validation, stacktool 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 stacktool 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 stacktool 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 stacktool 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 stacktool 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, stacktool 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 about three seconds of total build time.  It
> hasn't been optimized for performance yet, so there are probably some
> opportunities for better build performance.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  MAINTAINERS                                        |   7 +
>  Makefile                                           |   5 +-
>  arch/Kconfig                                       |   6 +
>  arch/x86/Kconfig                                   |   1 +
>  lib/Kconfig.debug                                  |  12 +
>  scripts/Makefile.build                             |  38 +-
>  tools/Makefile                                     |  11 +-
>  tools/stacktool/.gitignore                         |   2 +
>  tools/stacktool/Build                              |  13 +
>  tools/stacktool/Documentation/stack-validation.txt | 336 +++++++
>  tools/stacktool/Makefile                           |  60 ++
>  tools/stacktool/arch.h                             |  44 +
>  tools/stacktool/arch/x86/Build                     |  12 +
>  tools/stacktool/arch/x86/decode.c                  | 163 ++++
>  .../stacktool/arch/x86/insn/gen-insn-attr-x86.awk  | 387 ++++++++
>  tools/stacktool/arch/x86/insn/inat.c               |  97 ++
>  tools/stacktool/arch/x86/insn/inat.h               | 221 +++++
>  tools/stacktool/arch/x86/insn/inat_types.h         |  29 +
>  tools/stacktool/arch/x86/insn/insn.c               | 594 +++++++++++++
>  tools/stacktool/arch/x86/insn/insn.h               | 201 +++++
>  tools/stacktool/arch/x86/insn/x86-opcode-map.txt   | 984 +++++++++++++++++++++
>  tools/stacktool/builtin-check.c                    | 961 ++++++++++++++++++++
>  tools/stacktool/builtin.h                          |  22 +
>  tools/stacktool/elf.c                              | 403 +++++++++
>  tools/stacktool/elf.h                              |  79 ++
>  tools/stacktool/special.c                          | 199 +++++
>  tools/stacktool/special.h                          |  42 +
>  tools/stacktool/stacktool.c                        | 133 +++
>  tools/stacktool/warn.h                             |  60 ++
>  29 files changed, 5112 insertions(+), 10 deletions(-)

So since this commit is pretty big and complex, could you split up into multiple 
patches? I'd suggest the following series of 3 patches:

 - add the tools/ bitsl - don't change any kernel side code.

 - add the generic parts of the .config driven automatic build-time stack layout 
   checking machinery

 - add x86 Kconfig option and header file that allows the checking to be done on
   x86

I realize that at #2 it's not enabled for any architecture yet - but that's OK.

Thanks,

	Ingo

  parent reply	other threads:[~2016-01-19 12:03 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-18 12:39 [PATCH v15 00/25] " Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 01/25] tools: Fix formatting of the "make -C tools" help message Josh Poimboeuf
2016-01-13  9:40   ` [tip:perf/urgent] " tip-bot for Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 02/25] tools: Make list.h self-sufficient Josh Poimboeuf
2016-01-12 12:35   ` Borislav Petkov
2016-01-12 14:54     ` Arnaldo Carvalho de Melo
2016-01-12 15:59       ` Borislav Petkov
2016-01-12 17:16         ` Arnaldo Carvalho de Melo
2016-01-13  9:40   ` [tip:perf/urgent] " tip-bot for Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 03/25] tools subcmd: Add missing NORETURN define for parse-options.h Josh Poimboeuf
2016-01-13  9:41   ` [tip:perf/urgent] " tip-bot for Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 04/25] x86/asm: Frame pointer macro cleanup Josh Poimboeuf
2016-01-19 13:39   ` [tip:x86/asm] x86/asm: Clean up frame pointer macros tip-bot for Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 05/25] x86/asm: Add C versions of " Josh Poimboeuf
2016-01-19 13:40   ` [tip:x86/asm] " tip-bot for Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 06/25] x86/stacktool: Compile-time stack metadata validation Josh Poimboeuf
2016-01-12 14:48   ` Borislav Petkov
2016-01-12 15:06     ` Josh Poimboeuf
2016-01-12 16:10       ` Borislav Petkov
2016-01-19 12:02   ` Ingo Molnar [this message]
2015-12-18 12:39 ` [PATCH v15 07/25] x86/stacktool: Add file and directory ignores Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 08/25] x86/stacktool: Add ignore macros Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 09/25] x86/xen: Add stack frame dependency to hypercall inline asm calls Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 10/25] x86/paravirt: Add stack frame dependency to PVOP " Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 11/25] x86/paravirt: Create a stack frame in PV_CALLEE_SAVE_REGS_THUNK Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 12/25] x86/amd: Set ELF function type for vide() Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist Josh Poimboeuf
2016-01-12 16:47   ` Borislav Petkov
2016-01-12 17:43     ` Josh Poimboeuf
2016-01-12 17:55       ` Borislav Petkov
2016-01-12 18:56         ` Josh Poimboeuf
2016-01-12 19:37           ` Borislav Petkov
2016-01-13 10:55       ` Ingo Molnar
2016-01-15  6:06         ` Josh Poimboeuf
2016-01-15 10:41           ` Borislav Petkov
2016-01-15 11:00             ` Ingo Molnar
2016-01-15 11:11               ` Borislav Petkov
2016-01-15 11:13                 ` Ingo Molnar
2016-01-20  5:42               ` Josh Poimboeuf
2016-01-20  5:50                 ` H. Peter Anvin
2016-01-20  6:09                   ` Josh Poimboeuf
2016-01-20 10:44                 ` Borislav Petkov
2016-01-15 10:56           ` Ingo Molnar
2015-12-18 12:39 ` [PATCH v15 14/25] x86/xen: Add xen_cpuid() and xen_setup_gdt() to stacktool whitelists Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 15/25] x86/asm/crypto: Create stack frames in aesni-intel_asm.S Josh Poimboeuf
2016-01-12 16:53   ` Borislav Petkov
2016-01-12 16:54     ` Borislav Petkov
2015-12-18 12:39 ` [PATCH v15 16/25] x86/asm/crypto: Move .Lbswap_mask data to .rodata section Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 17/25] x86/asm/crypto: Move jump_table " Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 18/25] x86/asm/crypto: Create stack frames in clmul_ghash_mul/update() Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 19/25] x86/asm/entry: Create stack frames in thunk functions Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 20/25] x86/asm/acpi: Create a stack frame in do_suspend_lowlevel() Josh Poimboeuf
2015-12-20 16:13   ` Rafael J. Wysocki
2015-12-18 12:39 ` [PATCH v15 21/25] x86/asm: Create stack frames in rwsem functions Josh Poimboeuf
2016-01-12 12:41   ` Borislav Petkov
2016-01-12 14:36     ` Josh Poimboeuf
2016-01-12 14:40       ` Borislav Petkov
2015-12-18 12:39 ` [PATCH v15 22/25] x86/asm/efi: Create a stack frame in efi_call() Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 23/25] x86/asm/power: Create stack frames in hibernate_asm_64.S Josh Poimboeuf
2015-12-20 16:14   ` Rafael J. Wysocki
2015-12-18 12:39 ` [PATCH v15 24/25] x86/uaccess: Add stack frame output operand in get_user inline asm Josh Poimboeuf
2015-12-18 12:39 ` [PATCH v15 25/25] x86/stacktool: Ignore head_$(BITS) files Josh Poimboeuf
2016-01-12 14:58 ` [PATCH v15 00/25] Compile-time stack metadata validation Arnaldo Carvalho de Melo
2016-01-12 17:17 ` Borislav Petkov
2016-01-12 17:50   ` Josh Poimboeuf
2016-01-12 18:04     ` Borislav Petkov
2016-01-13 10:18   ` Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160119120255.GA18533@gmail.com \
    --to=mingo@kernel.org \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=andi@firstfloor.org \
    --cc=bernd@petrovitsch.priv.at \
    --cc=bp@alien8.de \
    --cc=chris.j.arges@canonical.com \
    --cc=hpa@zytor.com \
    --cc=jpoimboe@redhat.com \
    --cc=jslaby@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mmarek@suse.cz \
    --cc=namhyung@gmail.com \
    --cc=palves@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v15 06/25] x86/stacktool: Compile-time stack metadata validation' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.