| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a250f29d-969a-b704-6dd6-c6cc7b84f526@redhat.com>
Date: Thu, 2 Apr 2020 16:31:05 +0100
From: Julien Thierry <jthierry@...hat.com>
To: Alexandre Chartre <alexandre.chartre@...cle.com>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, jpoimboe@...hat.com,
peterz@...radead.org, tglx@...utronix.de
Subject: Re: [PATCH 4/7] objtool: Add support for return trampoline call
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@...cle.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
Powered by blists - more mailing lists