Re: [PATCH 4/7] objtool: Add support for return trampoline call
From: Julien Thierry
Date: Thu Apr 02 2020 - 11:31:14 EST
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@xxxxxxxxxx>
---
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