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:

quoted

On Thu, Feb 13, 2020 at 12:01:16PM +0000, Will Deacon wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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