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

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

[Kevin Brodsky]

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]

Cc: broonie@kernel.org
Cc: catalin.marinas@arm.com
Cc: Dave.Martin@arm.com
Cc: will@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
index 14ac6fdb872b..37e24f1bd227 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -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.
+	 */
 
 	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;
+	}
 	regs->sp = (unsigned long)user->sigframe;
 	regs->regs[29] = (unsigned long)&user->next_frame->fp;
+	regs->regs[30] = (unsigned long)sigtramp;
 	regs->pc = (unsigned long)ksig->ka.sa.sa_handler;
 
 	/*
@@ -1506,14 +1529,7 @@ static int setup_return(struct pt_regs *regs, struct ksignal *ksig,
 		sme_smstop();
 	}
 
-	if (ksig->ka.sa.sa_flags & SA_RESTORER)
-		sigtramp = ksig->ka.sa.sa_restorer;
-	else
-		sigtramp = VDSO_SYMBOL(current->mm->context.vdso, sigtramp);
-
-	regs->regs[30] = (unsigned long)sigtramp;
-
-	return gcs_signal_entry(sigtramp, ksig);
+	return 0;
 }
 
 static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
@@ -1537,14 +1553,16 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
 
 	err |= __save_altstack(&frame->uc.uc_stack, regs->sp);
 	err |= setup_sigframe(&user, regs, set, &ua_state);
-	if (err == 0) {
+	if (ksig->ka.sa.sa_flags & SA_SIGINFO)
+		err |= copy_siginfo_to_user(&frame->info, &ksig->info);
+
+	if (err == 0)
 		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

Re: [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@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>

> 
> Cc: broonie@kernel.org
> Cc: catalin.marinas@arm.com
> Cc: Dave.Martin@arm.com
> Cc: will@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

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

[Kevin Brodsky]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Tue, Dec 10, 2024 at 04:09:40PM +0000, Kevin Brodsky wrote:

> 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").

> Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")

Presumably we also need a Fixes for whatever introduced the above copy.
Otherwise

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Catalin Marinas]

On Thu, Dec 12, 2024 at 04:47:46PM +0000, Mark Brown wrote:
> On Tue, Dec 10, 2024 at 04:09:40PM +0000, Kevin Brodsky wrote:
> > 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").
> 
> > Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")
> 
> Presumably we also need a Fixes for whatever introduced the above copy.

Hmm, it was like this from the beginning. If we are to do a cc stable to
the oldest maintained LTS, I'd rather have a separate patch for
copy_siginfo_to_user(), it doesn't make much sense to backport a GCS
patch to early kernels.

-- 
Catalin

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

On Thu, Dec 12, 2024 at 05:10:14PM +0000, Catalin Marinas wrote:
> On Thu, Dec 12, 2024 at 04:47:46PM +0000, Mark Brown wrote:

> > > Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")

> > Presumably we also need a Fixes for whatever introduced the above copy.

> Hmm, it was like this from the beginning. If we are to do a cc stable to
> the oldest maintained LTS, I'd rather have a separate patch for
> copy_siginfo_to_user(), it doesn't make much sense to backport a GCS
> patch to early kernels.

Hrm, good point about it triggering backports.  I'm not sure it's worth
doing that TBH.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Catalin Marinas]

On Thu, Dec 12, 2024 at 05:26:47PM +0000, Mark Brown wrote:
> On Thu, Dec 12, 2024 at 05:10:14PM +0000, Catalin Marinas wrote:
> > On Thu, Dec 12, 2024 at 04:47:46PM +0000, Mark Brown wrote:
> > > > Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")
> > > 
> > > Presumably we also need a Fixes for whatever introduced the above copy.
> > 
> > Hmm, it was like this from the beginning. If we are to do a cc stable to
> > the oldest maintained LTS, I'd rather have a separate patch for
> > copy_siginfo_to_user(), it doesn't make much sense to backport a GCS
> > patch to early kernels.
> 
> Hrm, good point about it triggering backports.  I'm not sure it's worth
> doing that TBH.

I had the same thoughts. I think this was mostly triggered by the
POR_EL0 changes when Will raised the point that the kernel might return
with a more privileged value in this sysreg.

-- 
Catalin

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

[Kevin Brodsky]

On 12/12/2024 18:51, Catalin Marinas wrote:
> On Thu, Dec 12, 2024 at 05:26:47PM +0000, Mark Brown wrote:
>> On Thu, Dec 12, 2024 at 05:10:14PM +0000, Catalin Marinas wrote:
>>> On Thu, Dec 12, 2024 at 04:47:46PM +0000, Mark Brown wrote:
>>>>> Fixes: eaf62ce1563b ("arm64/signal: Set up and restore the GCS context for signal handlers")
>>>> Presumably we also need a Fixes for whatever introduced the above copy.
>>> Hmm, it was like this from the beginning. If we are to do a cc stable to
>>> the oldest maintained LTS, I'd rather have a separate patch for
>>> copy_siginfo_to_user(), it doesn't make much sense to backport a GCS
>>> patch to early kernels.
>> Hrm, good point about it triggering backports.  I'm not sure it's worth
>> doing that TBH.
> I had the same thoughts. I think this was mostly triggered by the
> POR_EL0 changes when Will raised the point that the kernel might return
> with a more privileged value in this sysreg.

I don't think backporting would be worth it either, referencing the
original commit implementing signal support felt like overkill. The
copy_siginfo_to_user() issue is such a corner case that I doubt anyone
ever ran into it.

- Kevin

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

[Catalin Marinas]

On Tue, 10 Dec 2024 16:09:40 +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).
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: signal: Ensure signal delivery failure is recoverable
      https://git.kernel.org/arm64/c/a3b4647e2f9a

-- 
Catalin