Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 4/7] objtool: Add support for return trampoline call
To:     Alexandre Chartre <alexandre.chartre@oracle.com>, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org, jpoimboe@redhat.com,
        peterz@infradead.org, tglx@linutronix.de
References: <20200402082220.808-1-alexandre.chartre@oracle.com>
 <20200402082220.808-5-alexandre.chartre@oracle.com>
 <c0f265ed-c86b-d3f1-3894-941c25e42d0e@redhat.com>
 <fc224792-bd1c-08ff-072f-e584740521b4@oracle.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <a250f29d-969a-b704-6dd6-c6cc7b84f526@redhat.com>
Date:   Thu, 2 Apr 2020 16:31:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <fc224792-bd1c-08ff-072f-e584740521b4@oracle.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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