A process can disable access to the alternate signal stack and still
expect signals to be delivered correctly. handle_signal() updates the
PKRU value to enable access to the atlstack, and makes sure that the
value on the sigframe is the user-defined PKRU value so that it is
correctly restored. However, in sigreturn(), restore_altstack() needs
read access to the alt stack. But the PKRU is already restored from the
sigframe (in restore_sigcontext()) which will disable access to the
alt stack, resulting in a SIGSEGV.

Fix this by restoring altstack before restoring PKRU.

Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
---
 arch/x86/kernel/signal_32.c | 4 ++--
 arch/x86/kernel/signal_64.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index 68f2bfd7d6e7..c7a489f0d943 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -160,10 +160,10 @@ SYSCALL32_DEFINE0(rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (!ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
+	if (restore_altstack32(&frame->uc.uc_stack))
 		goto badframe;
 
-	if (restore_altstack32(&frame->uc.uc_stack))
+	if (!ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext))
 		goto badframe;
 
 	return regs->ax;
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 6b189de005b5..2d053333fcf5 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -260,13 +260,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 	set_current_blocked(&set);
 
-	if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
+	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
-	if (restore_signal_shadow_stack())
+	if (!restore_sigcontext(regs, &frame->uc.uc_mcontext, uc_flags))
 		goto badframe;
 
-	if (restore_altstack(&frame->uc.uc_stack))
+	if (restore_signal_shadow_stack())
 		goto badframe;
 
 	return regs->ax;
-- 
2.39.3

Can we we move this patch to the first of the series? this can be an
independent patch, the problem not only affect PKRU, but also other
scenarios, as Rick pointed out in [1]

[1] https://lore.kernel.org/lkml/d0162c76c25bc8e1c876aebe8e243ff2e6862359.camel@intel.com/

-Jeff

> On Jun 10, 2024, at 2:44 PM, jeffxu@chromium.org wrote:
> 
> Can we we move this patch to the first of the series? this can be an
> independent patch, the problem not only affect PKRU, but also other
> scenarios, as Rick pointed out in [1]
> 
> [1] https://lore.kernel.org/lkml/d0162c76c25bc8e1c876aebe8e243ff2e6862359.camel@intel.com/
> 
> -Jeff

For this patch set, the issue with rt_sigreturn() is only exposed after patch 3/5 - i.e., when
copy_fpregs_to_sigframe() calls update_pkru_in_sigframe() to update the PKRU value to
user-specified PKRU that might disable pkey 0 access, thus breaking restore_altstack().
So it seemed logical to me to have this fix as patch 4/5 of this series. I’m not strongly
opposed to moving this to patch 1/5, but this ordering is easier to understand (I think).
But if this patch needs to be broken out of this patchset and submitted independently
(for the other scenarios you mentioned) - I can do that.

Thanks,
Aruna

On Tue, Jun 11, 2024 at 7:08 AM Aruna Ramakrishna
<aruna.ramakrishna@oracle.com> wrote:
>
>
>
> > On Jun 10, 2024, at 2:44 PM, jeffxu@chromium.org wrote:
> >
> > Can we we move this patch to the first of the series? this can be an
> > independent patch, the problem not only affect PKRU, but also other
> > scenarios, as Rick pointed out in [1]
> >
> > [1] https://lore.kernel.org/lkml/d0162c76c25bc8e1c876aebe8e243ff2e6862359.camel@intel.com/
> >
> > -Jeff
>
> For this patch set, the issue with rt_sigreturn() is only exposed after patch 3/5 - i.e., when
> copy_fpregs_to_sigframe() calls update_pkru_in_sigframe() to update the PKRU value to
> user-specified PKRU that might disable pkey 0 access, thus breaking restore_altstack().
> So it seemed logical to me to have this fix as patch 4/5 of this series. I’m not strongly
> opposed to moving this to patch 1/5, but this ordering is easier to understand (I think).
> But if this patch needs to be broken out of this patchset and submitted independently
> (for the other scenarios you mentioned) - I can do that.
>
My main thought  is that Rick mentioned this can be a cross-arch changes [1]
and we can start with x86/64 and other arches might follow.

[1] https://lore.kernel.org/lkml/20231107182251.91276-1-rick.p.edgecombe@intel.com/

Thanks
-Jeff

> Thanks,
> Aruna