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: <20200429155109.GN13592@hirez.programming.kicks-ass.net>
Date:   Wed, 29 Apr 2020 17:51:09 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Miroslav Benes <mbenes@...e.cz>
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, 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.