linux-kernel - Re: [RFC 3/6] objtool: arm64: Adapt the stack frame checks and the section analysis for the arm architecture

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: <20190424165640.5yeg2yicl7ej7g3i@treble>
Date:   Wed, 24 Apr 2019 11:56:40 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Raphael Gault <Raphael.Gault@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        Catalin Marinas <Catalin.Marinas@....com>,
        Will Deacon <Will.Deacon@....com>,
        Julien Thierry <Julien.Thierry@....com>
Subject: Re: [RFC 3/6] objtool: arm64: Adapt the stack frame checks and the
 section analysis for the arm architecture

On Wed, Apr 24, 2019 at 04:32:44PM +0000, Raphael Gault wrote:
> >> diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c
> >> index 0feb3ae3af5d..8b293eae2b38 100644
> >> --- a/tools/objtool/arch/arm64/decode.c
> >> +++ b/tools/objtool/arch/arm64/decode.c
> >> @@ -105,6 +105,33 @@ unsigned long arch_compute_rela_sym_offset(int addend)
> >>   return addend;
> >>   }
> >>
> >> +/*
> >> + * In order to know if we are in presence of a sibling
> >> + * call and not in presence of a switch table we look
> >> + * back at the previous instructions and see if we are
> >> + * jumping inside the same function that we are already
> >> + * in.
> >> + */
> >> +bool arch_is_insn_sibling_call(struct instruction *insn)
> >> +{
> >> +struct instruction *prev;
> >> +struct list_head *l;
> >> +struct symbol *sym;
> >> +list_for_each_prev(l, &insn->list) {
> >> +prev = (void *)l;
> >> +if (!prev->func
> >> +|| prev->func->pfunc != insn->func->pfunc)
> >> +return false;
> >> +if (prev->stack_op.src.reg != ADR_SOURCE)
> >> +continue;
> >> +sym = find_symbol_containing(insn->sec, insn->immediate);
> >> +if (!sym || sym->type != STT_FUNC
> >> +|| sym->pfunc != insn->func->pfunc)
> >> +return true;
> >> +break;
> >> +}
> >> +return true;
> >> +}
> >
> > I get the feeling there might be a better way to do this, but I can't
> > figure out what this function is actually doing.  It looks like it
> > searches backwards in the function for an instruction which has
> > stack_op.src.reg != ADR_SOURCE -- what does that mean?  And why doesn't
> > it do anything with the instruction after it finds it?
> >
> 
> I will indeed try to make it better.

I still don't quite get what it's trying to accomplish, but I wonder if
there's some kind of tracking you can add in validate_branch() to keep
track of whatever you're looking for, leading up to the indirect jump.

> >> -hash_add(file->insn_hash, &insn->hash, insn->offset);
> >> +/*
> >> + * For arm64 architecture, we sometime split instructions so that
> >> + * we can track the state evolution (i.e. load/store of pairs of registers).
> >> + * We thus need to take both into account and not erase the previous ones.
> >> + */
> >
> > Ew...  Is this an architectural thing, or just a quirk of the arm64
> > decoder?
> >
> 
> The motivation for this is to simulate the two consecutive operations
> that would be executed on x86 but are done in one on arm64. This is
> strictly a decoder related quirk. I don't know if there is a better way
> to do it without modifying the struct op_src and struct instruction.

Ah.  Which ops are those?  Hopefully we can find a better way to
represent that with a single instruction.  Adding fake instructions is
fragile.

-- 
Josh