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

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

On 12/12/2024 15:35, Dave Martin wrote:
> 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@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@arm.com>

Apologies, forgot to add it, thanks for the reminder!

- Kevin