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

[Josh Poimboeuf]

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@xxxxxxxxxx>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > ---
> >  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