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

[Dave Martin]

Hi,

On Tue, Dec 03, 2024 at 04:06:24PM +0100, Kevin Brodsky wrote:
> On 03/12/2024 12:56, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 10:03:22AM +0000, Kevin Brodsky wrote:
> >> [...]
> >> @@ -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?
> 
> That's a fair point, I could add a short comment after the call to
> setup_return() to clarify this.

That would make sense.

(Or refactor so that the point of no return is at a function boundary;
but since this is a Fixes: patch, it's probably best to keep things
simple.)
> 
> >>  
> >>  	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).
> 
> setup_return() is a bit of a misnomer, unless you interpret "return" as
> "return to userspace". Aside from setting LR, setup_return() isn't about
> how the handler returns but simply how it is invoked. For instance the
> line just above sets X0 to the signal number, the first argument of the
> handler. Setting X1/X2 here seems pretty natural (they're also arguments
> for the handler).
> 
> - Kevin

I guess.  I remember the function names here already being a bit
counterintuitive, so this patch is not making things much more
confusing than they already were.

Cheers
---Dave
>