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

[Luis Machado <luis.machado@linaro.org>]

Hi,

On 2/21/20 8:16 AM, Mark Rutland wrote:
> 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.
> 

Did we have any further developments on this front? Has a patch made its 
way upstream for review?

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