linux-kernel - Re: [v8,02/12] objtool: Allow alternatives to be ignored

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: <20180122192729.GA28236@roeck-us.net>
Date:   Mon, 22 Jan 2018 11:27:29 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     "Woodhouse, David" <dwmw@...zon.co.uk>,
        Andi Kleen <ak@...ux.intel.com>, Paul Turner <pjt@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...ux-foundation.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>, tglx@...utronix.de,
        Kees Cook <keescook@...gle.com>,
        Rik van Riel <riel@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...capital.net>,
        Jiri Kosina <jikos@...nel.org>, gnomes@...rguk.ukuu.org.uk,
        x86@...nel.org, thomas.lendacky@....com
Subject: Re: [v8,02/12] objtool: Allow alternatives to be ignored

On Thu, Jan 18, 2018 at 01:33:15PM -0600, Josh Poimboeuf wrote:
> On Thu, Jan 18, 2018 at 11:09:31AM -0800, Guenter Roeck wrote:
> > Hi folks,
> > 
> > On Thu, Jan 11, 2018 at 09:46:24PM +0000, Woodhouse, David wrote:
> > > Getting objtool to understand retpolines is going to be a bit of a
> > > challenge.  For now, take advantage of the fact that retpolines are
> > > patched in with alternatives.  Just read the original (sane)
> > > non-alternative instruction, and ignore the patched-in retpoline.
> > > 
> > > This allows objtool to understand the control flow *around* the
> > > retpoline, even if it can't yet follow what's inside.  This means the
> > > ORC unwinder will fail to unwind from inside a retpoline, but will work
> > > fine otherwise.
> > > 
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> > > Signed-off-by: David Woodhouse <dwmw@...zon.co.uk>
> > > ---
> > >  tools/objtool/check.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++-----
> > >  tools/objtool/check.h |  2 +-
> > >  2 files changed, 57 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > index de053fb..f40d46e 100644
> > > --- a/tools/objtool/check.c
> > > +++ b/tools/objtool/check.c
> > > @@ -428,6 +428,40 @@ static void add_ignores(struct objtool_file *file)
> > >  }
> > >  
> > >  /*
> > > + * FIXME: For now, just ignore any alternatives which add retpolines.  This is
> > > + * a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
> > > + * But it at least allows objtool to understand the control flow *around* the
> > > + * retpoline.
> > > + */
> > > +static int add_nospec_ignores(struct objtool_file *file)
> > > +{
> > > +	struct section *sec;
> > > +	struct rela *rela;
> > > +	struct instruction *insn;
> > > +
> > > +	sec = find_section_by_name(file->elf, ".rela.discard.nospec");
> > > +	if (!sec)
> > > +		return 0;
> > > +
> > > +	list_for_each_entry(rela, &sec->rela_list, list) {
> > > +		if (rela->sym->type != STT_SECTION) {
> > > +			WARN("unexpected relocation symbol type in %s", sec->name);
> > > +			return -1;
> > > +		}
> > > +
> > > +		insn = find_insn(file, rela->sym->sec, rela->addend);
> > > +		if (!insn) {
> > > +			WARN("bad .discard.nospec entry");
> > > +			return -1;
> > > +		}
> > > +
> > > +		insn->ignore_alts = true;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > >   * Find the destination instructions for all jumps.
> > >   */
> > >  static int add_jump_destinations(struct objtool_file *file)
> > > @@ -509,11 +543,18 @@ static int add_call_destinations(struct objtool_file *file)
> > >  			dest_off = insn->offset + insn->len + insn->immediate;
> > >  			insn->call_dest = find_symbol_by_offset(insn->sec,
> > >  								dest_off);
> > > +			/*
> > > +			 * FIXME: Thanks to retpolines, it's now considered
> > > +			 * normal for a function to call within itself.  So
> > > +			 * disable this warning for now.
> > > +			 */
> > > +#if 0
> > >  			if (!insn->call_dest) {
> > >  				WARN_FUNC("can't find call dest symbol at offset 0x%lx",
> > >  					  insn->sec, insn->offset, dest_off);
> > >  				return -1;
> > >  			}
> > > +#endif
> > 
> > This crashes for me in is_fentry_call().
> > 
> > Program received signal SIGSEGV, Segmentation fault.
> > is_fentry_call (insn=<optimized out>, insn=<optimized out>) at check.c:1113
> > 1113		if (insn->type == INSN_CALL &&
> > (gdb) info stack
> > #0  is_fentry_call (insn=<optimized out>, insn=<optimized out>) at check.c:1113
> > #1  validate_branch (file=0x7ffffff7e440, first=0x7ffffff7e128, state=...) at check.c:1747
> > #2  0x0000000000404bd3 in validate_branch (file=0x7ffffff7e440, first=0x7ffffff7e128, state=...) at check.c:1770
> > #3  0x0000000000406783 in validate_functions (file=<optimized out>) at check.c:1933
> > #4  check (_objname=0x6bb9d0 "", _no_fp=40, no_unreachable=4, orc=false) at check.c:2006
> > #5  0x00000000004021c1 in handle_internal_command (argv=0x7fffffffe5c0, argc=4) at objtool.c:108
> > #6  main (argc=4, argv=0x7fffffffe5c0) at objtool.c:131
> > 
> > This is not entirely surprising, since insn->call_dest is NULL and
> > is_fentry_call() doesn't expect that.
> > 
> > How is this supposed to work ? What am I missing ?
> 
> Not sure, does your gcc have retpolines?  Give me your .o file and I can
> diagnose it.
> 
> I intended to have an error msg instead of a seg fault for this
> situation, just haven't had a chance to improve that yet in the midst of
> all the hoopla.
> 
Any updates ? I reverted thee offending patch from our latest stable tree
merge, but that doesn't really solve the issue.

Thanks,
Guenter