linux-kernel - Re: [RFC][PATCH 15/16] objtool: Implement noinstr validation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200316132419.GF12521@hirez.programming.kicks-ass.net>
Date:   Mon, 16 Mar 2020 14:24:19 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     tglx@...utronix.de, linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [RFC][PATCH 15/16] objtool: Implement noinstr validation

On Sun, Mar 15, 2020 at 01:03:20PM -0500, Josh Poimboeuf wrote:
> On Thu, Mar 12, 2020 at 02:41:22PM +0100, Peter Zijlstra wrote:
> > Validate that any call out of .noinstr.text is in between
> > instr_begin() and instr_end() annotations.
> > 
> > This annotation is useful to ensure correct behaviour wrt tracing
> > sensitive code like entry/exit and idle code. When we run code in a
> > sensitive context we want a guarantee no unknown code is ran.
> > 
> > Since this validation relies on knowing the section of call
> > destination symbols, we must run it on vmlinux.o instead of on
> > individual object files.
> > 
> > Add the -i "noinstr validation only" option because:
> > 
> >  - vmlinux.o isn't 'clean' vs the existing validations
> >  - skipping the other validations (which have already been done
> >    earlier in the build) saves around a second of runtime.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> 
> I find the phrase "noinstr" to be WAY too ambiguous.  To my brain it
> clearly stands for "no instructions" and I have to do a double take
> every time I read it.

Thing is, we use insn for instruction all over both arch/x86/ and
objtool.

My personal naming preference didn't make it due to CoC reasons :-/

> And "read_instr_hints" reads as "read_instruction_hints()".
> 
> Can we come up with a more readable name?  Why not just "notrace"?
> 
> The trace begin/end annotations could be
> 
>   trace_allow_begin()
>   trace_allow_end()

notrace already exists and we didn't want to confuse things further.

> Also -- what happens when a function belongs in both .notrace.text and
> in one of the other special-purpose sections like .sched.text,
> .meminit.text or .entry.text?

Hasn't happened yet, initially we were thinking of using .entry.text for
this as a whole, but decided against that due to how .entry.text is
special for PTI (although exposing most of this code really wouldn't
matter).

The thing with .sched.text is that we really should never call into
scheduling from these contexts anyway. We've not ran into meminit yet.
(all this finicky entry code is ran with IRQs disabled).

The one that could potentially interfere is .cpuidle.text.

> Maybe storing pointers to the functions, like NOKPROBE_SYMBOL does,
> would be better than putting the functions in a separate section.

Thing is, I really _hate_ that annotation style.

> > ---
> >  tools/objtool/builtin-check.c |    4 -
> >  tools/objtool/builtin.h       |    2 
> >  tools/objtool/check.c         |  155 ++++++++++++++++++++++++++++++++++++------
> >  tools/objtool/check.h         |    3 
> >  4 files changed, 140 insertions(+), 24 deletions(-)
> > 
> > --- a/tools/objtool/builtin-check.c
> > +++ b/tools/objtool/builtin-check.c
> > @@ -17,7 +17,7 @@
> >  #include "builtin.h"
> >  #include "check.h"
> >  
> > -bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats;
> > +bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess, stats, noinstr, vmlinux;
> >  
> >  static const char * const check_usage[] = {
> >  	"objtool check [<options>] file.o",
> > @@ -32,6 +32,8 @@ const struct option check_options[] = {
> >  	OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
> >  	OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
> >  	OPT_BOOLEAN('s', "stats", &stats, "print statistics"),
> > +	OPT_BOOLEAN('i', "noinstr", &noinstr, "noinstr validation only"),
> > +	OPT_BOOLEAN('l', "vmlinux", &vmlinux, "vmlinux.o validation"),
> >  	OPT_END(),
> >  };
> 
> It seems like there should be an easy way to auto-detect vmlinux.o,
> without needing a cmdline option.
> 
> For example, if the file name is "vmlinux.o" :-)

Fair enough I suppose...

> Also, maybe we can just hard-code the fact that vmlinux.o is always
> noinstr-only.  Over time we'll probably need to move more per-.o
> functionalities to vmlinux.o and I think we should avoid creating a
> bunch of cmdline options.

but you're ruining things here, see, for a regular x86_64 config, we'd
run this with:

	objtool check -fail vmlinux.o

And I was hoping to get vmlinux.o objtool clean, surprisingly there
really aren't that many complaints. But the -i thing makes it run
significantly faster without duplicating all the bits we've already
checked.

Anyway, let me address and refresh the series while we bicket about
naming later. I'm thikning Thomas would much prefer to get his first
round of patches out first.