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

[Kevin Brodsky <kevin.brodsky@arm.com>]

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.

>>  
>>  	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