linux-kernel - Re: [PATCH v2 02/14] objtool: Fix ORC vs alternatives

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: <alpine.LSU.2.21.2004291840340.3662@pobox.suse.cz>
Date:   Wed, 29 Apr 2020 18:41:36 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Peter Zijlstra <peterz@...radead.org>
cc:     jpoimboe@...hat.com, alexandre.chartre@...cle.com,
        linux-kernel@...r.kernel.org, jthierry@...hat.com,
        tglx@...utronix.de, x86@...nel.org, Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH v2 02/14] objtool: Fix ORC vs alternatives

On Wed, 29 Apr 2020, Peter Zijlstra wrote:

> On Wed, Apr 29, 2020 at 04:33:31PM +0200, Miroslav Benes wrote:
> > On Tue, 28 Apr 2020, Peter Zijlstra wrote:
> > >  /*
> > > + * Alternatives should not contain any ORC entries, this in turn means they
> > > + * should not contain any CFI ops, which implies all instructions should have
> > > + * the same same CFI state.
> > > + *
> > > + * It is possible to constuct alternatives that have unreachable holes that go
> > > + * unreported (because they're NOPs), such holes would result in CFI_UNDEFINED
> > > + * states which then results in ORC entries, which we just said we didn't want.
> > > + *
> > > + * Avoid them by copying the CFI entry of the first instruction into the whole
> > > + * alternative.
> > > + */
> > > +static void fill_alternative_cfi(struct objtool_file *file, struct instruction *insn)
> > > +{
> > > +	struct instruction *first_insn = insn;
> > > +	int alt_group = insn->alt_group;
> > > +
> > > +	sec_for_each_insn_continue(file, insn) {
> > > +		if (insn->alt_group != alt_group)
> > > +			break;
> > > +		insn->cfi = first_insn->cfi;
> > > +	}
> > > +}
> > 
> > If I am reading this and previous patch correctly...
> > 
> > The function would copy cfi only to "orig" alternative (its insn->alts is 
> > non-empty, orig_insn->alt_group differs from new_insn->alt_group), right? 
> 
> Yes.
> 
> > Would it make sense to do the same for "new" alternative, because of the 
> > invariant? It seems to me it is not processed anywhere that way.
> 
> No.
> 
> > Am I missing something? Whenever I try to read all this alternatives 
> > handling in objtool, I get lost pretty soon.
> 
> We only care about the ORC covering the original range, because that is
> the range we execute code from. The memory where we store the
> alternative instructions (.altinstruction section) is never executed,
> that is, RIP should never point there, so we don't need ORC data covering
> it.

Aha, that's what I didn't realize (again). Note to myself (for the 
hundredth time): alternatives are not branches.

Thanks
Miroslav