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: <c90f402e-6494-73bc-1df8-516c3113019a@arm.com>
Date:   Thu, 25 Apr 2019 08:12:24 +0000
From:   Raphael Gault <Raphael.Gault@....com>
To:     Josh Poimboeuf <jpoimboe@...hat.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

Hi Josh,

On 4/24/19 5:56 PM, Josh Poimboeuf wrote:
> 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.
>

The motivation behind this is that the `br <Xn>` instruction is a
dynamic jump (jump to the address contained in the provided register).
This instruction is used for sibling calls but can also be used for
switch table. I use this to differentiate these two cases from one another:

Generally the `adr/adrp` instruction is used prior to `br` in order to
load the address into the register. What I do here is go back throught
the instructions and try to identify if the address loaded.

I also thought of implementing some sort of tracking in validate branch
because it could be useful for identifying the switch tables as well.
But it seemed to me like a major change in the sementic of this tool:
indeed, from my perspective I would have to track the state of the
registers and I don't know if we want to do that.

>>>> -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.
>

Those are the load/store of pairs of registers, mainly stp/ldp. Those
are often use in the function prologues/epilogues to save/restore the
stack pointers and frame pointers however it can be used with any
register pair.

The idea to add a new instruction could work but I would need to extend
the `struct op_src` as well I think.

Thanks,

--
Raphael Gault
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.