linux-kernel - Re: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200331204047.GF2452@worktop.programming.kicks-ass.net>
Date:   Tue, 31 Mar 2020 22:40:47 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     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: [RFC][PATCH] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET

On Tue, Mar 31, 2020 at 03:23:15PM -0500, Josh Poimboeuf wrote:
> On Tue, Mar 31, 2020 at 01:16:52PM +0200, Peter Zijlstra wrote:
> > Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
> > 
> > This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that applies
> > to the following instructions:
> > 
> >  - 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.
> > 
> >  - EXCEPTION_RETURN (a new INSN_type that splits IRET out of
> >    CONTEXT_SWITCH) and here it denotes a @sp_offset sized POP and makes
> >    the instruction continue.
> 
> Looking closer, I see how my UNWIND_HINT_ADJUST idea doesn't work for
> the ftrace_regs_caller() case.  The ORC data is actually correct there.
> So basically we need a way to tell objtool to be quiet.

Right.

> I now understand what you're trying to do with the RET_TAIL thing, and I
> guess it's ok for the ftrace case.  But I'd rather an UNWIND_HINT_IGNORE
> before the tail cail, which would tell objtool to just silence the tail
> call warning.  It's simpler for the user to understand, it's simpler
> logic in objtool, and I think an "ignore warnings for the next insn"
> hint would be more generally applicable anyway.

I like how this is specific on how far the stack can be off, as opposed
so say 'ignore any warning on this instruction'.

Because by saying this RET should be +8, we'll still get a warning when
this is not the case (and in fact I should strengthen the patch to
implement that).

Also, you don't want to suppress any other valid warning at that
instruction.

Furthermore, I really don't think we ought to worry about ease-of-use
here, there's really not that many people writing x86 assembly.

> But also... the RET_OFFSET usage for sync_core() *really* bugs me.

Fair enough.

> I know you said it's like an indirect tail call with a bigger frame, but
> that's kind of stretching it because the function frame is still there.
> 
> And objtool doesn't treat it like a tail call at all.  In fact, it
> handles it *completely* differently from the normal ret-tail-call case.
> Instead of silencing a tail call warning, it adjusts the stack offset
> and continues the code path.
> 
> This basically adds *two* new hint types, while trying to call them the
> same thing.  There's no overlapping functionality between them in
> objtool, other than the use of the same insn->ret_offset variable.  But
> it's two distinct functionalities, depending on the context (return/tail
> vs IRETQ).

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 ?