From: Josh Poimboeuf <email@example.com> To: Borislav Petkov <firstname.lastname@example.org> Cc: Thomas Gleixner <email@example.com>, Ingo Molnar <firstname.lastname@example.org>, "H. Peter Anvin" <email@example.com>, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, Michal Marek <email@example.com>, Peter Zijlstra <firstname.lastname@example.org>, Andy Lutomirski <email@example.com>, Linus Torvalds <firstname.lastname@example.org>, Andi Kleen <email@example.com>, Pedro Alves <firstname.lastname@example.org>, Namhyung Kim <email@example.com>, Bernd Petrovitsch <firstname.lastname@example.org>, Chris J Arges <email@example.com>, Andrew Morton <firstname.lastname@example.org>, Jiri Slaby <email@example.com>, Arnaldo Carvalho de Melo <firstname.lastname@example.org> Subject: Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist Date: Tue, 12 Jan 2016 12:56:06 -0600 [thread overview] Message-ID: <20160112185606.GF310@treble.redhat.com> (raw) In-Reply-To: <20160112175540.GI30558@pd.tnic> On Tue, Jan 12, 2016 at 06:55:40PM +0100, Borislav Petkov wrote: > On Tue, Jan 12, 2016 at 11:43:01AM -0600, Josh Poimboeuf wrote: > > Well, these STACKTOOL_IGNORE whitelist markers are only needed in a > > handful of places, and only for code that does very weird things. Yes, > > they're a bit ugly, but IMO they also communicate valuable information: > > "be careful, this code does something very weird." > > Weird for whom? Well, for example, here are the 3 places in the code where I needed to use STACKTOOL_IGNORE: - machine_real_restart() does an "ljmp" (far jump) into real mode and never returns - xen_setup_gdt() does an "lret" (far return) - xen_cpuid() inserts some fake xen-specific (non-x86) asm instructions I'd say those are all weird things that C code should _normally_ not do (especially emitting fake instructions!). But yet they should be allowed for those specific rare cases where we can manually verify that they won't cause problems. > > As for whether to put the whitelist info in the code vs hard-coding it > > in stacktool, I think it's clearer and less "magical" to put them > > directly in the code. > > I don't think so. All that unnecessary clutter just gets in the way > of actually writing code. Sure, those tools are all good and nice but > again, they should *not* have to touch the code. We want people to > concentrate on writing code, not paying attention to gazillion tools > breaking from their changes. Generally I agree (but I don't know what other tools you're talking about which require adding clutter). We don't want the tools to get in the way. I've tried *very* hard to make stacktool as unobtrusive as possible. And I think I've done a good job at it. As I said there's hopefully only a handful of code locations which need the STACKTOOL_IGNORE stuff. > > It's also more resilient to future code changes, e.g. if the offending > > instruction gets moved or if the function gets renamed. > > So make your tool parse vmlinux properly. > > > And it gives you the ability to more granularly whitelist instructions > > rather than entire functions, which could cause other offending stack > > violations in the function to get overlooked. > > No no no, the moment you have to *whitelist* something is already wrong. > Normal kernel code shouldn't have to whitelist anything - the tools > should strive to be smart, instead. The tool *does* strive to be smart. It disassembles every instruction in both C and asm object files, follows every code path, understands the "alternatives" hot-patching stuff, jump tables, exception tables, gcc switch tables, tail function calls, etc, etc, etc. That's why we're now at v15 :-) Taking a hard line of "no whitelists" is just not very realistic when you take into account some of the special cases in the kernel. For example, how can it possibly grok a fake xen instruction without some kind of a whitelist, either hard-coded in the tool or annotated some other way? If you know of some other way to avoid all false positives without having to whitelist them, then I'm all for it. > > Another thing is that stacktool could be a nice general purpose tool for > > finding stack issues in other code bases, and so I think requiring it to > > have hard-coded knowledge about the code base would greatly limit its > > general usefulness. (Though maybe this problem could be remediated with > > a user-provided whitelist file which lists functions to be ignored.) > > I can use the same argument for me: all those other code bases would > need annotating too. Maybe so, although the kernel is more "special" than most code bases and is thus more likely to need annotations. But again, we really try to keep the annotations to a minimum. The tradeoff is worth it IMO. > Again, the onus should always be on the tool to do the right thing. > If it cannot, it should not say anything. Saying nothing at all in order to prevent false positives would also by definition allow some false negatives, which would make stacktool useless for its intended purpose of enabling reliable stack traces. -- Josh
next prev parent reply other threads:[~2016-01-12 18:56 UTC|newest] Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-18 12:39 [PATCH v15 00/25] Compile-time stack metadata validation 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 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 [this message] 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=20160112185606.GF310@treble.redhat.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist' \ /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.