[PATCH] arm64: ptrace: Fix PTRACE_SINGLESTEP into signal handler

[PATCH] arm64: ptrace: Fix PTRACE_SINGLESTEP into signal handler

[Keno Fischer]

Executing PTRACE_SINGLESTEP at a signal stop is special. It
is supposed to step merely the signal setup work that the
kernel does, but not any instructions in user space. Since
not all architectures have the ability to generate a
single-step exception directly upon return from user-space,
there is a generic pseudo-single-step-stop that may be used
for this purpose (tracehook_signal_handler). Now, arm64 does
have the ability to generate single-step exceptions directly
upon return to userspace and was using this capability (rather
than the generic pseudo-trap) to obtain a similar effect. However,
there is actually a subtle difference that becomes noticeable
when the signal handler in question attempts to block SIGTRAP
(either because it is set in sa_mask, or because it is a handler
for SIGTRAP itself and SA_NODEFER is not set). In such a
situation, a real single step exception will cause the SIGTRAP
signal to be forcibly unblocked and the signal disposition
to be reset to SIG_DFL. The generic pseudo-single-step does
not suffer from this problem, because the SIGTRAP it delivers
is not real. The arm64 behavior is problematic, because a forced
reset of the signal disposition can be quite disruptive to the
userspace program. This patch brings the arm64 behavior in line
with the other major architectures by using the generic
pseudo-single-step-stop, avoiding the problematic interaction
with SIGTRAP masks.

Fixes: 2c020ed8 ("arm64: Signal handling support")
Signed-off-by: Keno Fischer <keno@juliacomputing.com>
---
 arch/arm64/kernel/signal.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 339882db5a91..cf237ee9443b 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -808,14 +808,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	 */
 	ret |= !valid_user_regs(&regs->user_regs, current);
 
-	/*
-	 * Fast forward the stepping logic so we step into the signal
-	 * handler.
-	 */
-	if (!ret)
-		user_fastforward_single_step(tsk);
-
-	signal_setup_done(ret, ksig, 0);
+	signal_setup_done(ret, ksig, test_thread_flag(TIF_SINGLESTEP));
 }
 
 /*
-- 
2.25.1

Re: [PATCH] arm64: ptrace: Fix PTRACE_SINGLESTEP into signal handler

[Will Deacon]

On Sun, May 24, 2020 at 12:38:27AM -0400, Keno Fischer wrote:
> Executing PTRACE_SINGLESTEP at a signal stop is special. It
> is supposed to step merely the signal setup work that the
> kernel does, but not any instructions in user space. Since
> not all architectures have the ability to generate a
> single-step exception directly upon return from user-space,
> there is a generic pseudo-single-step-stop that may be used
> for this purpose (tracehook_signal_handler). Now, arm64 does
> have the ability to generate single-step exceptions directly
> upon return to userspace and was using this capability (rather
> than the generic pseudo-trap) to obtain a similar effect. However,
> there is actually a subtle difference that becomes noticeable
> when the signal handler in question attempts to block SIGTRAP
> (either because it is set in sa_mask, or because it is a handler
> for SIGTRAP itself and SA_NODEFER is not set). In such a
> situation, a real single step exception will cause the SIGTRAP
> signal to be forcibly unblocked and the signal disposition
> to be reset to SIG_DFL. The generic pseudo-single-step does
> not suffer from this problem, because the SIGTRAP it delivers
> is not real. The arm64 behavior is problematic, because a forced
> reset of the signal disposition can be quite disruptive to the
> userspace program. This patch brings the arm64 behavior in line
> with the other major architectures by using the generic
> pseudo-single-step-stop, avoiding the problematic interaction
> with SIGTRAP masks.
> 
> Fixes: 2c020ed8 ("arm64: Signal handling support")

nit: please use a 12-character ID here.

> Signed-off-by: Keno Fischer <keno@juliacomputing.com>
> ---
>  arch/arm64/kernel/signal.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 339882db5a91..cf237ee9443b 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -808,14 +808,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  	 */
>  	ret |= !valid_user_regs(&regs->user_regs, current);
>  
> -	/*
> -	 * Fast forward the stepping logic so we step into the signal
> -	 * handler.
> -	 */
> -	if (!ret)
> -		user_fastforward_single_step(tsk);
> -
> -	signal_setup_done(ret, ksig, 0);
> +	signal_setup_done(ret, ksig, test_thread_flag(TIF_SINGLESTEP));

another nit: tsk is now unused, so this generates a compiler warning:


arch/arm64/kernel/signal.c:787:22: warning: unused variable 'tsk' [-Wunused-variable]
        struct task_struct *tsk = current;
                            ^
1 warning generated.


Also, the si_code used by signal_setup_done seems to be SIGTRAP, whereas
we usually set TRAP_TRACE. What's the correct behaviour here? Looks like x86
uses TRAP_BRKPT... :/

Will