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

[Dave Martin]

Hi,

On Tue, Dec 10, 2024 at 04:09:40PM +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>
> ---
> v1..v2:
> * Added a short comment in setup_rt_frame() after the call to
>   setup_return() to discourage adding failure points there.
>   [Dave's suggestion]

Assuming that this was the only change, I was OK for you to add:

Reviewed-by: Dave Martin <Dave.Martin 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 | 48 ++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c

[...]

> @@ -1537,14 +1553,16 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,

[...]

>  		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;
> -		}
> -	}
> +
> +	/*
> +	 * We must not fail if setup_return() succeeded - see comment at the
> +	 * beginning of setup_return().
> +	 */
>  
>  	if (err == 0)
>  		set_handler_user_access_state();
> 
> base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4
> -- 
> 2.47.0
> 
> 

Cheers
---Dave