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

[Kevin Brodsky]

On 03/12/2024 16:37, Dave Martin wrote:
> 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.)

I did consider this, in fact this is what I did initially. However I
then realised that gcs_signal_entry() itself modifies thread state that
affects control flow. In other words, if it succeeds, then we are
already in a situation where failure should be avoided, so pulling it
out of setup_return() wouldn't necessarily be clearer. There may be more
operations that can fail like gcs_signal_entry() in the future, meaning
we'd have to "revert" such operations if any fails. I think it makes
sense for this to be handled in setup_return(). At least we don't need
to complicate things for now.

- Kevin