lkml.org 
[lkml]   [2020]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4/7] objtool: Add support for return trampoline call
From
Date


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

\
 
 \ /
  Last update: 2020-04-02 17:31    [W:0.143 / U:0.360 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site