linux-kernel - Re: [PATCH 2/2] x86/entry: Fix unwinding from kprobe on PUSH/POP instruction

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] [day] [month] [year] [list]

Date:   Fri, 17 Feb 2023 13:44:20 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Josh Poimboeuf <jpoimboe@...nel.org>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        Chen Zhongjin <chenzhongjin@...wei.com>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH 2/2] x86/entry: Fix unwinding from kprobe on PUSH/POP
 instruction

On Thu, Feb 16, 2023 at 08:06:19AM -0800, Josh Poimboeuf wrote:
> On Thu, Feb 16, 2023 at 12:58:58PM +0100, Peter Zijlstra wrote:
> > On Fri, Feb 10, 2023 at 02:42:02PM -0800, Josh Poimboeuf wrote:
> > 
> > > The problem is that #BP saves the pointer to the instruction immediately
> > > *after* the INT3, rather than to the INT3 itself.  The instruction
> > > replaced by the INT3 hasn't actually run, but ORC assumes otherwise and
> > > expects the wrong stack layout.
> > > 
> > > Fix it by annotating the #BP exception as a non-signal stack frame,
> > > which tells the ORC unwinder to decrement the instruction pointer before
> > > looking up the corresponding ORC entry.
> > > 
> > > Reported-by: Chen Zhongjin <chenzhongjin@...wei.com>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> > > ---
> > >  arch/x86/entry/entry_64.S | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> > > index 15739a2c0983..8d21881adf86 100644
> > > --- a/arch/x86/entry/entry_64.S
> > > +++ b/arch/x86/entry/entry_64.S
> > > @@ -385,7 +385,14 @@ SYM_CODE_END(xen_error_entry)
> > >   */
> > >  .macro idtentry vector asmsym cfunc has_error_code:req
> > >  SYM_CODE_START(\asmsym)
> > > -	UNWIND_HINT_IRET_REGS offset=\has_error_code*8
> > > +
> > > +	.if \vector == X86_TRAP_BP
> > > +		/* #BP advances %rip to the next instruction */
> > > +		UNWIND_HINT_IRET_REGS offset=\has_error_code*8 signal=0
> > 
> > So the fact that INT3 is trap like is not the problem, the problem is
> > that we use INT3 to overwrite stack modifying instruction and we should
> > not assume those instructions have completed when in the #BP handler.
> > 
> > Now, the reason all this actually works is because INT3 itself does not
> > modify the stack so rewinding on non-overwrite INT3 instructions is
> > invariant wrt stack state.
> 
> Right, that's what my patch description attempting to say.
> 
> That's also why I was asking about INT1, which is a trap.  Do we care
> about INT1?

We do not care about INT1, #DB is an IST and an allround pain in the
backside, INT3 is where it's at.