| Message ID | 20241203100322.1626805-1-kevin.brodsky@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: signal: Ensure signal delivery failure is recoverable | expand |
On Tue, Dec 03, 2024 at 10:03:22AM +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> > --- > Cc: broonie@kernel.org > Cc: catalin.marinas@arm.com > Cc: Dave.Martin@arm.com > Cc: will@kernel.org > --- > arch/arm64/kernel/signal.c | 43 +++++++++++++++++++++++++------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c > index 14ac6fdb872b..0adaa165b876 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. > + */ The same is true at the call site of this function; could problems creep back in there? > > 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). Would it be cleaner just to stage the destructive state changes somewhere and apply them right at the end (as with POR_EL0)? > + } > 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,11 @@ 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; > - } > - } > > if (err == 0) > set_handler_user_access_state(); [...] Cheers ---Dave
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
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 >
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
On Tue, Dec 03, 2024 at 05:48:10PM +0100, Kevin Brodsky wrote: > 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) > >>>> { [...] > >>>> + /* > >>>> + * 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. That seems fair. A one-line comment at the call to setup_return() should be enough. I guess we can live with the slightly confusing function naming (since it was already a bit confusing, and this isn't trivial to sort out). So, with the comment, feel free to add: Reviewed-by: Dave Martin <Dave.Martin@arm.com> Cheers ---Dave
On 04/12/2024 12:20, Dave Martin wrote: > That seems fair. A one-line comment at the call to setup_return() > should be enough. I guess we can live with the slightly confusing > function naming (since it was already a bit confusing, and this isn't > trivial to sort out). > > So, with the comment, feel free to add: > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> I'll add that comment in v2 - thanks for the review, very appreciated! - Kevin
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 14ac6fdb872b..0adaa165b876 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,11 @@ 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; - } - } if (err == 0) set_handler_user_access_state();
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> --- Cc: broonie@kernel.org Cc: catalin.marinas@arm.com Cc: Dave.Martin@arm.com Cc: will@kernel.org --- arch/arm64/kernel/signal.c | 43 +++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 15 deletions(-)