Messages in this thread | | | Subject | Re: [PATCH 4/7] objtool: Add support for return trampoline call | From | Julien Thierry <> | Date | Thu, 2 Apr 2020 16:31:05 +0100 |
| |
On 4/2/20 3:46 PM, Alexandre Chartre wrote: > > On 4/2/20 3:26 PM, Julien Thierry wrote: >> 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). > > The problem is that an intra-function call is not necessarily going > to return. With retpoline (or RSB stuffing) intra-function calls are > basically fake calls only present to fill the RSB buffer. Such calls > won't return, the stack pointer is just adjusted to cancel the impact > of these calls on the stack. >
Right, but running into an intra-function call will start a validate branch with a modified stack frame.
So, starting from an intra-function call, if we run into a return, I guess objtool will complain about a return with modified stack frame.
My understanding is that once you find an intra-function call, either you hit a return, ending the branch, so the return should undo the modification the intra-function call did (whether is it a retpoline return or not). Otherwise, the intra-function call branch will need to reach an end in some way (e.g. hitting a CONTEXT_SWITCH instruction, calling a dead_end_function).
Am I missing something?
-- Julien Thierry
| |