Re: [arm64, debug] PTRACE_SINGLESTEP does not single-step a valid instruction

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Feb 20, 2020 at 01:29:42PM +0000, Will Deacon wrote:
> Hi Mark,
> 
> Thanks for having a look.
> 
> On Thu, Feb 20, 2020 at 01:02:22PM +0000, Mark Rutland wrote:
> > On Thu, Feb 13, 2020 at 12:01:16PM +0000, Will Deacon wrote:
> > > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > > index cd6e5fa48b9c..d479fbcbd0d2 100644
> > > --- a/arch/arm64/kernel/ptrace.c
> > > +++ b/arch/arm64/kernel/ptrace.c
> > > @@ -1934,8 +1934,8 @@ static int valid_native_regs(struct user_pt_regs *regs)
> > >   */
> > >  int valid_user_regs(struct user_pt_regs *regs, struct task_struct *task)
> > >  {
> > > -	if (!test_tsk_thread_flag(task, TIF_SINGLESTEP))
> > > -		regs->pstate &= ~DBG_SPSR_SS;
> > > +	/* https://lore.kernel.org/lkml/20191118131525.GA4180@willie-the-truck */
> > > +	user_regs_reset_single_step(regs, task);
> > 
> > I think this change means we do the right thing for signal entry/return
> > and ptrace messing with the regs. Instruction emulation seems to do the
> > right thing via skip_faulting_instruction().
> > 
> > I think there are a few more single-step edge cases lying around (e.g.
> > uprobes, rseq), but it looks like those have to be fixed separately. I
> > fear fixing uprobes might require a largler structural change to single
> > step, but ignoring uprobes the changes above seem to be sound.
> 
> Rseq should just abort when delivering the step signal and I'm not sure I
> see the issue with uprobes. Can you elaborate on your concerns a bit,
> please?

For rseq I wasn't sure what state PSTATE.SS should be when we head to
the abort handler -- I think the sensible thing would be that it
immediately triggers a single-step exception, but I don't see where we'd
clear PSTATE.SS to ensure that.

For uprobes I fear that the uprobes xol single-stepping might end up
conflicting with the regular ptrace single-stepping, and that the
uprobes emulation might not always advance the state machine correctly.

> > If userspace doesn't consume the SS value today, I wonder if we should
> > hide it when dumping the SPSR to userspace, so that userspace has a
> > consistent view regardless of whether it's being stepped.
> 
> You can't really hide it though, because '0' has a meaning so I don't think
> it gains us a lot other than increasing the scope of the change.

I think that it reduces the likelihood that single-stepping a program
changes its behaviour unexpectedly. This patch makes the kernel
disregard the PSTATE.SS value provided by userspace, so what is gained
by exposing PSTATE.SS to userspace at all?

I do agree that there are potentially subtle landmines here; I just
can't see a legitimate reason for userspace to need the value.

> > I'll try to dig into the uprobes stuff this afternoon, just in case
> > that
> > needs us to do something substantially different.
> 
> Thanks.

I didn't get the chance to do this yesterday, but I did think of another
potential problem.

I *think* that when attempting to single-step a syscall, if prior to
return from the syscall the tracer messed with the tracee's regs (e.g.
to mess with arguments or the retun value) then valid_user_regs() will
set the SS bit, and upon return from the syscall the next instruction
would be executed rather than first raising a single-step exception.

This patch relies on valid_user_regs() being a signal that PSTATE.SS is
stale, but that's not always the case. To handle that generally I
suspect we need two bits of state rather than just TIF_SINGLESTEP.

> > The existing logic in valid_user_regs() doesn't make sense to me, given
> > SPSR_EL1.SS is immaterial unless MSCDR_EL1.SS == 1. I'm not sure if that
> > was overzealous or I've forgotten an edge case that we cared about in
> > the past.
> 
> I think it was just part of sanitising the registers to a consistent value,
> but I agree that it wouldn't have a functional impact.

Thanks for confirming my understanding. I guess this may have minimized
the cases where userspace saw PSTATE.SS set.

> > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> > > index 339882db5a91..bc54bdbfd760 100644
> > > --- a/arch/arm64/kernel/signal.c
> > > +++ b/arch/arm64/kernel/signal.c
> > > @@ -505,8 +505,12 @@ static int restore_sigframe(struct pt_regs *regs,
> > >  	forget_syscall(regs);
> > >  
> > >  	err |= !valid_user_regs(&regs->user_regs, current);
> > > -	if (err == 0)
> > > +
> > > +	if (err == 0) {
> > > +		/* Make it look like we stepped the sigreturn system call */
> > > +		user_fastforward_single_step(current);
> > >  		err = parse_user_sigframe(&user, sf);
> > > +	}
> > 
> > I don't understand this. AFAICT  we don't likewise for other SVCs, so
> > either I'm missing that, or there's something else I'm missing.
> > 
> > Why do we need to step sigreturn but not SVC generally?
> 
> Because we restore the SPSR from the sigframe during sigreturn, so we will
> end up with PSTATE.SS set when it should be cleared.

Ah, I see. As above, I think we can hit a similar case when
single-stepping an SVC for a regular syscall.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel