Message-ID: <9a5f48e4-4846-7a47-48c6-52d937bf01e1@arm.com>
Date:   Tue, 30 Apr 2019 12:20:56 +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/25/19 5:25 PM, Josh Poimboeuf wrote:
> On Thu, Apr 25, 2019 at 08:12:24AM +0000, Raphael Gault wrote:
>> 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.
>
> I don't have much time to look at this today (and I'll be out next
> week), but we had a similar problem in x86.  See the comments above
> find_switch_table(), particularly #3.  Does that function not work for
> the arm64 case?
>

Honestly, I don't have a full understanding of how the switch tables are
handled on arm64. All I know is that I've identified a case in which it
doesn't work (and I get an unreachable instruction warning).
When trying to figure out how the switch tables work on arm64 and how
objtool is retrieving them (on x86 at least) I realised that you look
for 2 relocations :
- One from (.rela).text which refers to the .rodata section
- One from (.rela).rodata which refers somewhere else.
On the case I identified the second relocation doesn't exist thus the
function doesn't find the switch table.

Again since I do not have a good understanding about this I am not able
to say if it is a corner case or not.

>>>>>> -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.
>
> Again I don't have much time to look at it, but I do think that changing
> op_src/dest to allow for the stp/ldp instructions would work better than
> inserting a fake instruction to emulate x86.
>
> Or another idea would be to associate multiple stack_ops with a single
> instruction.
>

I haven't looked at it in depth yet but I will try to figure out a good
way to represent those instructions on a more proper manner.

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.

Powered by blists - more mailing lists