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

From: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Luis Machado <luis.machado@linaro.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [arm64, debug] PTRACE_SINGLESTEP does not single-step a valid instruction
Date: Thu, 20 Feb 2020 13:29:42 +0000	[thread overview]
Message-ID: <20200220132941.GB14459@willie-the-truck> (raw)
In-Reply-To: <20200220130222.GA28634@lakrids.cambridge.arm.com>

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?

> 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'll try to dig into the uprobes stuff this afternoon, just in case that
> needs us to do something substantially different.

Thanks.

> 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.

> > 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.

Will

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