Messages in this thread | | | Subject | Re: [PATCH 4/7] objtool: Add support for return trampoline call | From | Julien Thierry <> | Date | Thu, 2 Apr 2020 14:26:57 +0100 |
| |
Hi Alexandre,
On 4/2/20 9:22 AM, Alexandre Chartre wrote: > With retpoline, the return instruction is used to branch to an address > stored on the stack. So, unlike a regular return instruction, when a > retpoline return instruction is reached the stack has been modified > compared to what we have when the function was entered. > > Provide the mechanism to explicitly call-out such return instruction > so that objtool can correctly handle them. > > Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com> > --- > tools/objtool/check.c | 78 +++++++++++++++++++++++++++++++++++++++++-- > tools/objtool/check.h | 1 + > 2 files changed, 76 insertions(+), 3 deletions(-) > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 0cec91291d46..ed8e3ea1d8da 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -1344,6 +1344,48 @@ static int read_intra_function_call(struct objtool_file *file) > return 0; > } > > +static int read_retpoline_ret(struct objtool_file *file) > +{ > + struct section *sec; > + struct instruction *insn; > + struct rela *rela; > + > + sec = find_section_by_name(file->elf, ".rela.discard.retpoline_ret"); > + if (!sec) > + return 0; > + > + list_for_each_entry(rela, &sec->rela_list, list) { > + if (rela->sym->type != STT_SECTION) { > + WARN("unexpected relocation symbol type in %s", > + sec->name); > + return -1; > + } > + > + insn = find_insn(file, rela->sym->sec, rela->addend); > + if (!insn) { > + WARN("bad .discard.retpoline_ret entry"); > + return -1; > + } > + > + if (insn->type != INSN_RETURN) { > + WARN_FUNC("retpoline_ret not a return", > + insn->sec, insn->offset); > + return -1; > + } > + > + insn->retpoline_ret = true; > + /* > + * For the impact on the stack, make a return trampoline > + * behaves like a pop of the return address. > + */ > + insn->stack_op.src.type = OP_SRC_POP; > + insn->stack_op.dest.type = OP_DEST_REG; > + insn->stack_op.dest.reg = CFI_RA; > + } > + > + return 0; > +} > + > static void mark_rodata(struct objtool_file *file) > { > struct section *sec; > @@ -1403,6 +1445,10 @@ static int decode_sections(struct objtool_file *file) > if (ret) > return ret; > > + ret = read_retpoline_ret(file); > + if (ret) > + return ret; > + > ret = add_call_destinations(file); > if (ret) > return ret; > @@ -1432,7 +1478,8 @@ static bool is_fentry_call(struct instruction *insn) > return false; > } > > -static bool has_modified_stack_frame(struct insn_state *state) > +static bool has_modified_stack_frame(struct insn_state *state, > + bool check_registers) > { > int i; > > @@ -1442,6 +1489,9 @@ static bool has_modified_stack_frame(struct insn_state *state) > state->drap) > return true; > > + if (!check_registers) > + return false; > + > for (i = 0; i < CFI_NUM_REGS; i++) > if (state->regs[i].base != initial_func_cfi.regs[i].base || > state->regs[i].offset != initial_func_cfi.regs[i].offset) > @@ -1987,7 +2037,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state) > > static int validate_sibling_call(struct instruction *insn, struct insn_state *state) > { > - if (has_modified_stack_frame(state)) { > + if (has_modified_stack_frame(state, true)) { > WARN_FUNC("sibling call from callable instruction with modified stack frame", > insn->sec, insn->offset); > return 1; > @@ -2009,6 +2059,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > struct alternative *alt; > struct instruction *insn, *next_insn; > struct section *sec; > + bool check_registers; > u8 visited; > int ret; > > @@ -2130,7 +2181,28 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > return 1; > } > > - if (func && has_modified_stack_frame(&state)) { > + /* > + * With retpoline, the return instruction is used > + * to branch to an address stored on the stack. > + * So when we reach the ret instruction, the stack > + * frame has been modified with the address to > + * branch to and we need update the stack state. > + * > + * The retpoline address to branch to is typically > + * pushed on the stack from a register, but this > + * confuses the logic which checks callee saved > + * registers. So we don't check if registers have > + * been modified if we have a return trampoline.
I think there are two different things to consider here.
First, the update of the stack frame which I believe should be done when returning from intra_function_calls, as it undoes what the call instruction did (push more stuff on the stack in the case of x86).
This might mean that intra_function_call should be part of the state (as intra_function_calls pass a modified state to validate_branch() ).
Second is supporting retpoline_ret which is just accepting that the return address in the stack frame has changed.
> + */ > + if (insn->retpoline_ret) { > + update_insn_state(insn, &state); > + check_registers = false; > + } else { > + check_registers = true; > + } > + > + if (func && has_modified_stack_frame(&state, > + check_registers)) { > WARN_FUNC("return with modified stack frame", > sec, insn->offset); > return 1; > diff --git a/tools/objtool/check.h b/tools/objtool/check.h > index 2bd6d2f46baa..5ecd16ad71a8 100644 > --- a/tools/objtool/check.h > +++ b/tools/objtool/check.h > @@ -37,6 +37,7 @@ struct instruction { > bool dead_end, ignore, hint, save, restore, ignore_alts; > bool intra_function_call; > bool retpoline_safe; > + bool retpoline_ret; > u8 visited; > struct symbol *call_dest; > struct instruction *jump_dest; >
Cheers,
-- Julien Thierry
| |