[PATCH] arm64: signal: Ensure signal delivery failure is recoverable

[Dave Martin]

On Tue, Dec 03, 2024 at 10:03:22AM +0000, Kevin Brodsky wrote:
> Commit eaf62ce1563b ("arm64/signal: Set up and restore the GCS
> context for signal handlers") introduced a potential failure point
> at the end of setup_return(). This is unfortunate as it is too late
> to deliver a SIGSEGV: if that SIGSEGV is handled, the subsequent
> sigreturn will end up returning to the original handler, which is
> not the intention (since we failed to deliver that signal).
> 
> Make sure this does not happen by calling gcs_signal_entry()
> at the very beginning of setup_return(), and add a comment just
> after to discourage error cases being introduced from that point
> onwards.
> 
> While at it, also take care of copy_siginfo_to_user(): since it may
> fail, we shouldn't be calling it after setup_return() either. Call
> it before setup_return() instead, and move the setting of X1/X2
> inside setup_return() where it belongs (after the "point of no
> failure").
> 
> Background: the first part of setup_rt_frame(), including
> setup_sigframe(), has no impact on the execution of the interrupted
> thread. The signal frame is written to the stack, but the stack
> pointer remains unchanged. Failure at this stage can be recovered by
> a SIGSEGV handler, and sigreturn will restore the original context,
> at the point where the original signal occurred. On the other hand,
> once setup_return() has updated registers including SP, the thread's
> control flow has been modified and we must deliver the original
> signal.
> 
> Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")
> Signed-off-by: Kevin Brodsky <kevin.brodsky at arm.com>
> ---
> Cc: broonie at kernel.org
> Cc: catalin.marinas at arm.com
> Cc: Dave.Martin at arm.com
> Cc: will at kernel.org
> ---
>  arch/arm64/kernel/signal.c | 43 +++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 14ac6fdb872b..0adaa165b876 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -1462,10 +1462,33 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
>  			 struct rt_sigframe_user_layout *user, int usig)
>  {
>  	__sigrestore_t sigtramp;
> +	int err;
> +
> +	if (ksig->ka.sa.sa_flags & SA_RESTORER)
> +		sigtramp = ksig->ka.sa.sa_restorer;
> +	else
> +		sigtramp = VDSO_SYMBOL(current->mm->context.vdso, sigtramp);
> +
> +	err = gcs_signal_entry(sigtramp, ksig);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * We must not fail from this point onwards. We are going to update
> +	 * registers, including SP, in order to invoke the signal handler. If
> +	 * we failed and attempted to deliver a nested SIGSEGV to a handler
> +	 * after that point, the subsequent sigreturn would end up restoring
> +	 * the (partial) state for the original signal handler.
> +	 */

The same is true at the call site of this function; could problems
creep back in there?

>  
>  	regs->regs[0] = usig;
> +	if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
> +		regs->regs[1] = (unsigned long)&user->sigframe->info;
> +		regs->regs[2] = (unsigned long)&user->sigframe->uc;

Nit: This looks like it should work, but it's not really anything to do
with setting up how the handler returns (we're in setup_return() here).

Would it be cleaner just to stage the destructive state changes
somewhere and apply them right at the end (as with POR_EL0)?

> +	}
>  	regs->sp = (unsigned long)user->sigframe;
>  	regs->regs[29] = (unsigned long)&user->next_frame->fp;
> +	regs->regs[30] = (unsigned long)sigtramp;
>  	regs->pc = (unsigned long)ksig->ka.sa.sa_handler;
>  
>  	/*
> @@ -1506,14 +1529,7 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
>  		sme_smstop();
>  	}
>  
> -	if (ksig->ka.sa.sa_flags & SA_RESTORER)
> -		sigtramp = ksig->ka.sa.sa_restorer;
> -	else
> -		sigtramp = VDSO_SYMBOL(current->mm->context.vdso, sigtramp);
> -
> -	regs->regs[30] = (unsigned long)sigtramp;
> -
> -	return gcs_signal_entry(sigtramp, ksig);
> +	return 0;
>  }
>  
>  static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
> @@ -1537,14 +1553,11 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>  
>  	err |= __save_altstack(&frame->uc.uc_stack, regs->sp);
>  	err |= setup_sigframe(&user, regs, set, &ua_state);
> -	if (err == 0) {
> +	if (ksig->ka.sa.sa_flags & SA_SIGINFO)
> +		err |= copy_siginfo_to_user(&frame->info, &ksig->info);
> +
> +	if (err == 0)
>  		err = setup_return(regs, ksig, &user, usig);
> -		if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
> -			err |= copy_siginfo_to_user(&frame->info, &ksig->info);
> -			regs->regs[1] = (unsigned long)&frame->info;
> -			regs->regs[2] = (unsigned long)&frame->uc;
> -		}
> -	}
>  
>  	if (err == 0)
>  		set_handler_user_access_state();

[...]

Cheers
---Dave