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

Message-ID: <20230216160619.rleqhnrufcilizy2@treble>
Date:   Thu, 16 Feb 2023 08:06:19 -0800
From:   Josh Poimboeuf <jpoimboe@...nel.org>
To:     Peter Zijlstra <peterz@...radead.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 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?

-- 
Josh