Message-ID: <070a10c3-2d87-7ac8-96ee-95ae71c29df7@arm.com>
Date:   Wed, 1 May 2019 15:09:21 +0000
From:   Raphael Gault <Raphael.Gault@....com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
CC:     Julien Thierry <Julien.Thierry@....com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        Catalin Marinas <Catalin.Marinas@....com>,
        Will Deacon <Will.Deacon@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC 3/6] objtool: arm64: Adapt the stack frame checks and the
 section analysis for the arm architecture

Hi Josh,

On 4/30/19 1:20 PM, Raphael Gault wrote:
> 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.

I wanted to get your thoughts on the solution I found without waiting
for v2. If it's too much trouble reviewing it now I'll wait for the v2.

I added a field to the struct stack_op in order to have access to an
extra register. This way I can provide the extra register to the instruction
in order to use it later.

diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index 52599ebd89fb..ae9ae25b3bdc 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -51,6 +51,7 @@ struct op_dest {
  int offset;
  };

+
  enum op_src_type {
  OP_SRC_REG,
  OP_SRC_REG_INDIRECT,
@@ -66,9 +67,16 @@ struct op_src {
  int offset;
  };

+struct op_extra {
+unsigned char used;
+unsigned char reg;
+int offset;
+};
+
  struct stack_op {
  struct op_dest dest;
  struct op_src src;
+struct op_extra extra;
  };

  struct instruction;
diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c
index 17b5d59f16ad..86ad37c8397c 100644
--- a/tools/objtool/arch/arm64/decode.c
+++ b/tools/objtool/arch/arm64/decode.c
@@ -1743,23 +1673,26 @@ int arm_decode_ld_st_regs_pair_off(u32 instr, unsigned char *type,
  op->src.type = OP_SRC_REG_INDIRECT;
  op->src.reg = CFI_SP;
  op->src.offset = 0;
-state.curr_offset = 8;
  op->dest.type = OP_DEST_REG;
-op->dest.reg = state.regs[0];
+op->dest.reg = rt;
  op->dest.offset = 0;
+op->extra.used = 1;
+op->extra.reg = rt2;
+op->extra.offset = 8;
  break;
  default:
  op->dest.type = OP_DEST_REG_INDIRECT;
  op->dest.reg = CFI_SP;
  op->dest.offset = 8;
-state.curr_offset = 0;
  op->src.type = OP_SRC_REG;
-op->src.reg = state.regs[1];
+op->src.reg = rt2;
  op->src.offset = 0;
+op->extra.used = 1;
+op->extra.reg = rt;
+op->extra.offset = 0;
  /* store */
  }
-state.op = *op;
-return INSN_COMPOSED;
+return 0;
  }

  int arm_decode_ld_st_regs_pair_post(u32 instr, unsigned char *type,
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5ddb25414de5..df1fb6ce1e8f 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1580,6 +1569,18 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
      initial_func_cfi.cfa.base == CFI_CFA)
  cfa->base = CFI_CFA;

+if (op->extra.used) {
+if (regs[op->extra.reg].offset == -state->stack_size)
+restore_reg(state, op->extra.reg);
+state->stack_size -= 8;
+if (cfa->base == CFI_SP)
+cfa->offset -= 8;
+if (cfa->base == CFI_SP &&
+    cfa->offset == 0 &&
+    initial_func_cfi.cfa.base == CFI_CFA)
+cfa->base = CFI_CFA;
+}
+
  break;

  case OP_SRC_REG_INDIRECT:
@@ -1598,12 +1599,22 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
  /* drap: mov disp(%rbp), %reg */
  restore_reg(state, op->dest.reg);

+if (op->extra.used &&
+    op->src.reg == CFI_BP &&
+    op->extra.offset == regs[op->extra.reg].offset)
+restore_reg(state, op->extra.reg);
+
  } else if (op->src.reg == cfa->base &&
      op->src.offset == regs[op->dest.reg].offset + cfa->offset) {

  /* mov disp(%rbp), %reg */
  /* mov disp(%rsp), %reg */
  restore_reg(state, op->dest.reg);
+
+if (op->extra.used &&
+    op->src.reg == cfa->base &&
+    op->extra.offset == regs[op->extra.reg].offset + cfa->offset)
+restore_reg(state, op->extra.reg);
  }

  break;
@@ -1653,6 +1664,21 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
  save_reg(state, op->src.reg, CFI_CFA, -state->stack_size);
  }

+if (op->extra.used) {
+state->stack_size += 8;
+if (cfa->base == CFI_CFA)
+cfa->base = CFI_SP;
+if (cfa->base == CFI_SP)
+cfa->offset += 8;
+if (!state->drap ||
+    (!(op->extra.reg == cfa->base &&
+       op->extra.reg == state->drap_reg) &&
+     !(op->extra.reg == CFI_BP &&
+       cfa->base == state->drap_reg) &&
+     regs[op->extra.reg].base == CFI_UNDEFINED))
+save_reg(state, op->extra.reg, CFI_CFA,
+ -state->stack_size);
+}
  /* detect when asm code uses rbp as a scratch register */
  if (!no_fp && insn->func && op->src.reg == CFI_BP &&
      cfa->base != CFI_BP)
@@ -1671,11 +1697,19 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
  /* save drap offset so we know when to restore it */
  state->drap_offset = op->dest.offset;
  }
+if (op->extra.used && op->extra.reg == cfa->base &&
+    op->extra.reg == state->drap_reg) {
+cfa->base = CFI_BP_INDIRECT;
+cfa->offset = op->extra.offset;
+}

  else if (regs[op->src.reg].base == CFI_UNDEFINED) {

  /* drap: mov reg, disp(%rbp) */
  save_reg(state, op->src.reg, CFI_BP, op->dest.offset);
+if (op->extra.used)
+save_reg(state, op->extra.reg, CFI_BP,
+ op->extra.offset);
  }

  } else if (op->dest.reg == cfa->base) {
@@ -1684,8 +1718,12 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
  /* mov reg, disp(%rsp) */
  save_reg(state, op->src.reg, CFI_CFA,
   op->dest.offset - state->cfa.offset);
+if (op->extra.used)
+save_reg(state, op->extra.reg, CFI_CFA,
+ op->extra.offset - state->cfa.offset);
  }

+
  break;

  case OP_DEST_LEAVE:

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