| 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: <d2cad75e-1708-f0bf-7f88-194bcb29e61d@redhat.com>
Date: Wed, 1 Apr 2020 16:43:35 +0100
From: Julien Thierry <jthierry@...hat.com>
To: Peter Zijlstra <peterz@...radead.org>,
Josh Poimboeuf <jpoimboe@...hat.com>
Cc: tglx@...utronix.de, linux-kernel@...r.kernel.org, x86@...nel.org,
mhiramat@...nel.org, mbenes@...e.cz,
Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
Hi Peter,
On 3/31/20 11:27 PM, Peter Zijlstra wrote:
> On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote:
>> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote:
>>>> I'm not against adding a second/separate hint for this. In fact, I
>>>> almost considered teaching objtool how to interpret the whole IRET frame
>>>> so that we can do it without hints. It's just that that's too much code
>>>> for this one case.
>>>>
>>>> HINT_IRET_SELF ?
>>>
>>> Despite my earlier complaint about stack size knowledge, we could just
>>> forget the hint and make "iretq in C code" equivalent to "reduce stack
>>> size by arch_exception_stack_size()" and keep going. There's
>>> file->c_file which tells you it's a C file.
>>
>> Or maybe "iretq in an STT_FUNC" is better since this pattern could
>> presumably happen in a callable asm function.
>
> Like so then?
>
> ---
> Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> From: Peter Zijlstra <peterz@...radead.org>
> Date: Tue, 31 Mar 2020 13:16:52 +0200
>
> This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that
> applies to any instruction that terminates a function, like: RETURN
> and sibling calls. It allows the stack-frame to be off by @sp_offset,
> ie. it allows stuffing the return stack.
>
> For ftrace_64.S we split the return path and make sure the
> ftrace_epilogue call is seen as a sibling/tail-call turning it into it's
> own function.
>
> By splitting the return path every instruction has a unique stack setup
> and ORC can generate correct unwinds. Then employ the RET_OFFSET hint to
> the tail-call exit that has the direct-call (orig_eax) stuffed on the
> return stack.
>
> For sync_core() we teach objtool that an IRET inside an STT_FUNC
> simply consumes the exception stack and continues.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
> arch/x86/include/asm/orc_types.h | 9 ++-
> arch/x86/include/asm/processor.h | 2
> arch/x86/include/asm/unwind_hints.h | 12 +---
> arch/x86/kernel/ftrace.c | 12 ++++
> arch/x86/kernel/ftrace_64.S | 27 ++++-------
> tools/arch/x86/include/asm/orc_types.h | 9 ++-
> tools/objtool/Makefile | 2
> tools/objtool/arch.h | 3 +
> tools/objtool/arch/x86/decode.c | 5 +-
> tools/objtool/check.c | 80 ++++++++++-----------------------
> tools/objtool/check.h | 4 +
> 11 files changed, 74 insertions(+), 91 deletions(-)
>
[snip]
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt
>
> cfa = &insn->state.cfa;
>
> - if (hint->type == UNWIND_HINT_TYPE_SAVE) {
> - insn->save = true;
> - continue;
> -
> - } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) {
> - insn->restore = true;
> - insn->hint = true;
> + if (hint->type == UNWIND_HINT_TYPE_RET_OFFSET) {
> + insn->ret_offset = hint->sp_offset;
> continue;
> }
>
> @@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru
> return false;
> }
>
> -static bool has_modified_stack_frame(struct insn_state *state)
> +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state)
> {
> + u8 ret_offset = insn->ret_offset;
> int i;
>
> - if (state->cfa.base != initial_func_cfi.cfa.base ||
> - state->cfa.offset != initial_func_cfi.cfa.offset ||
> - state->stack_size != initial_func_cfi.cfa.offset ||
> - state->drap)
> + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap)
> + return true;
> +
> + if (state->cfa.offset != initial_func_cfi.cfa.offset &&
> + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
Isn't that the same thing as "state->cfa.offset !=
initial_func_cfi.cfa.offset + ret_offset" ?
> + return true;
> +
> + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset)
> return true;
>
> - for (i = 0; i < CFI_NUM_REGS; i++)
> + 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)
> return true;
> + }
>
> return false;
> }
> @@ -1971,7 +1972,7 @@ static int validate_call(struct instruct
>
> static int validate_sibling_call(struct instruction *insn, struct insn_state *state)
> {
> - if (has_modified_stack_frame(state)) {
> + if (has_modified_stack_frame(insn, state)) {
> WARN_FUNC("sibling call from callable instruction with modified stack frame",
> insn->sec, insn->offset);
> return 1;
> @@ -2000,7 +2001,7 @@ static int validate_return(struct symbol
> return 1;
> }
>
> - if (func && has_modified_stack_frame(state)) {
> + if (func && has_modified_stack_frame(insn, state)) {
> WARN_FUNC("return with modified stack frame",
> insn->sec, insn->offset);
> return 1;
> @@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo
> return 0;
> }
>
> - if (insn->hint) {
> - if (insn->restore) {
> - struct instruction *save_insn, *i;
> -
> - i = insn;
> - save_insn = NULL;
> - sym_for_each_insn_continue_reverse(file, func, i) {
> - if (i->save) {
> - save_insn = i;
> - break;
> - }
> - }
> -
> - if (!save_insn) {
> - WARN_FUNC("no corresponding CFI save for CFI restore",
> - sec, insn->offset);
> - return 1;
> - }
> -
> - if (!save_insn->visited) {
> - /*
> - * Oops, no state to copy yet.
> - * Hopefully we can reach this
> - * instruction from another branch
> - * after the save insn has been
> - * visited.
> - */
> - if (insn == first)
> - return 0;
> -
> - WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo",
> - sec, insn->offset);
> - return 1;
> - }
> -
> - insn->state = save_insn->state;
> - }
> -
> + if (insn->hint)
> state = insn->state;
> -
> - } else
> + else
> insn->state = state;
>
> insn->visited |= visited;
> @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo
>
> break;
>
> + case INSN_EXCEPTION_RETURN:
> + if (func) {
> + state.stack_size -= arch_exception_frame_size;
> + break;
Why break instead of returning? Shouldn't an exception return mark the
end of a branch (whether inside or outside a function) ?
Here it seems it will continue to the next instruction which might have
been unreachable.
> + }
> +
> + /* fallthrough */
What is the purpose of the fallthrough here? If the exception return was
in a function, it carried on to the next instruction, so it won't use
the WARN_FUNC(). So, if I'm looking at the right version of the code
only the "return 0;" will be used. And, unless my previous comment is
wrong, I'd argue that we should return both for func and !func.
> case INSN_CONTEXT_SWITCH:
> if (func && (!next_insn || !next_insn->hint)) {
> WARN_FUNC("unsupported instruction in callable function",
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -33,9 +33,11 @@ struct instruction {
> unsigned int len;
> enum insn_type type;
> unsigned long immediate;
> - bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts;
> + bool alt_group, dead_end, ignore, ignore_alts;
> + bool hint;
> bool retpoline_safe;
> u8 visited;
> + u8 ret_offset;
> struct symbol *call_dest;
> struct instruction *jump_dest;
> struct instruction *first_jump_src;
>
>
Cheers,
--
Julien Thierry
Powered by blists - more mailing lists