| 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
| ||
|
Date: Thu, 2 Apr 2020 14:26:57 +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
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).
This might mean that intra_function_call should be part of the state (as
intra_function_calls pass a modified state to validate_branch() ).
Second is supporting retpoline_ret which is just accepting that the
return address in the stack frame has changed.
> + */
> + if (insn->retpoline_ret) {
> + update_insn_state(insn, &state);
> + check_registers = false;
> + } else {
> + check_registers = true;
> + }
> +
> + if (func && has_modified_stack_frame(&state,
> + check_registers)) {
> WARN_FUNC("return with modified stack frame",
> sec, insn->offset);
> return 1;
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 2bd6d2f46baa..5ecd16ad71a8 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -37,6 +37,7 @@ struct instruction {
> bool dead_end, ignore, hint, save, restore, ignore_alts;
> bool intra_function_call;
> bool retpoline_safe;
> + bool retpoline_ret;
> u8 visited;
> struct symbol *call_dest;
> struct instruction *jump_dest;
>
Cheers,
--
Julien Thierry
Powered by blists - more mailing lists