If the alternate signal stack is protected by a different pkey than the
current execution stack, copying xsave data to the altsigstack will fail
if its pkey is not enabled. This commit enables all pkeys before xsave,
so that the signal handler accessibility is not dictated by the PKRU
value that the thread sets up. It then writes the original PKRU value
onto the sigframe so that it's restored correctly from sigcontext.

Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
---
 arch/x86/kernel/fpu/signal.c | 11 +++++++++--
 arch/x86/kernel/signal.c     |  3 +++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index dce84cce7cf8..5d52c7fde43b 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -185,8 +185,15 @@ static inline bool save_xstate_epilog(void __user *buf, int ia32_frame,
 static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf,
 					  u32 pkru)
 {
-	if (use_xsave())
-		return xsave_to_user_sigframe(buf);
+	int err = 0;
+
+	if (use_xsave()) {
+		err = xsave_to_user_sigframe(buf);
+		if (!err && cpu_feature_enabled(X86_FEATURE_OSPKE))
+			err = __update_pkru_in_sigframe(buf, pkru);
+		return err;
+	}
+
 	if (use_fxsr())
 		return fxsave_to_user_sigframe((struct fxregs_state __user *) buf);
 	else
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 75dfd05c59aa..c985bdfd855a 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -278,6 +278,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	if (stepping)
 		user_disable_single_step(current);
 
+	pkru = sig_prepare_pkru();
 	failed = (setup_rt_frame(ksig, regs, pkru) < 0);
 	if (!failed) {
 		/*
@@ -295,6 +296,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * Ensure the signal handler starts with the new fpu state.
 		 */
 		fpu__clear_user_states(fpu);
+	} else {
+		write_pkru(pkru);
 	}
 	signal_setup_done(failed, ksig, stepping);
 }
-- 
2.39.3

On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:

> If the alternate signal stack is protected by a different pkey than the
> current execution stack, copying xsave data to the altsigstack will fail
> if its pkey is not enabled. This commit enables all pkeys before
> xsave,

This commit (patch) ....

Also this lacks any justification why this enables all pkeys and how
that is the right thing to do instead of using init_pkru_value which
is what is set by fpu__clear_user_states() before going back to user
space. For signal handling this can be the only valid PKEY state unless
I'm missing something here.

>  static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf,
>  					  u32 pkru)
>  {
> -	if (use_xsave())
> -		return xsave_to_user_sigframe(buf);
> +	int err = 0;
> +
> +	if (use_xsave()) {
> +		err = xsave_to_user_sigframe(buf);
> +		if (!err && cpu_feature_enabled(X86_FEATURE_OSPKE))

The CPU feature check really wants to be in update_pkru_in_sigframe()

> @@ -278,6 +278,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  	if (stepping)
>  		user_disable_single_step(current);
>  
> +	pkru = sig_prepare_pkru();

pkru is defined in the first patch:

> +       u32 pkru = read_pkru();

Why do we need a read and then another read in sig_prepare_pkru()?

Also this lacks a comment what the sig_prepare_pkru() invocation is for ...

>  	failed = (setup_rt_frame(ksig, regs, pkru) < 0);
>  	if (!failed) {
>  		/*
> @@ -295,6 +296,8 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>  		 * Ensure the signal handler starts with the new fpu state.
>  		 */
>  		fpu__clear_user_states(fpu);
> +	} else {
> +		write_pkru(pkru);

... and a corresponding comment why this needs to be restored here.

>  	}
>  	signal_setup_done(failed, ksig, stepping);
>  }

Thanks,

        tglx

> On May 7, 2024, at 9:47 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
> 
>> If the alternate signal stack is protected by a different pkey than the
>> current execution stack, copying xsave data to the altsigstack will fail
>> if its pkey is not enabled. This commit enables all pkeys before
>> xsave,
> 
> This commit (patch) ....
> 
> Also this lacks any justification why this enables all pkeys and how
> that is the right thing to do instead of using init_pkru_value which
> is what is set by fpu__clear_user_states() before going back to user
> space. For signal handling this can be the only valid PKEY state unless
> I'm missing something here.


If the alt sig stack is protected by a different pkey (other than pkey 0), then
this flow would need to enable that, along with the pkey for the thread’s 
stack. Since the code has no way of knowing what pkey the altstack needs,
it enables all for this brief window.

> 
>> static inline int copy_fpregs_to_sigframe(struct xregs_state __user *buf,
>>  u32 pkru)
>> {
>> - if (use_xsave())
>> - return xsave_to_user_sigframe(buf);
>> + int err = 0;
>> +
>> + if (use_xsave()) {
>> + err = xsave_to_user_sigframe(buf);
>> + if (!err && cpu_feature_enabled(X86_FEATURE_OSPKE))
> 
> The CPU feature check really wants to be in update_pkru_in_sigframe()
> 
>> @@ -278,6 +278,7 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>> if (stepping)
>> user_disable_single_step(current);
>> 
>> + pkru = sig_prepare_pkru();
> 
> pkru is defined in the first patch:
> 
>> +       u32 pkru = read_pkru();
> 
> Why do we need a read and then another read in sig_prepare_pkru()?

sig_prepare_pkru() is where the pkru value is updated to make sure the alt stack is
accessible for XSAVE, while capturing the user-defined orig_pkru so that it can be
correctly restored later. Two reads are obviously redundant - I will remove the first one
(introduced in patch 1) here. 

Thanks,
Aruna

On Tue, May 07 2024 at 17:34, Aruna Ramakrishna wrote:
>> On May 7, 2024, at 9:47 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> Also this lacks any justification why this enables all pkeys and how
>> that is the right thing to do instead of using init_pkru_value which
>> is what is set by fpu__clear_user_states() before going back to user
>> space. For signal handling this can be the only valid PKEY state unless
>> I'm missing something here.
>
> If the alt sig stack is protected by a different pkey (other than pkey 0), then
> this flow would need to enable that, along with the pkey for the thread’s 
> stack. Since the code has no way of knowing what pkey the altstack needs,
> it enables all for this brief window.

Again. The flow here is:

handle_signal()
  enable_access_to_altstack()

  ....

  fpu__clear_user_states()
    restore_fpregs_from_init_fpstate(XFEATURE_MASK_USER_RESTORE)
    os_xrstor(&init_fpstate, features_mask)
    pkru_write_default()
      write_pkru(init_pkru_value);  <- Loads the default PKRU value
           
return_to_user_space()

User space resumes with the default PKRU value and the first thing user
space does when entering the signal handler is to push stuff on the
signal stack.

If the signal stack is protected by a key which is not contained in
init_pkru_value then the application segfaults in a non recoverable way,
no?

So arguably it is sufficient to ensure that PKRU has the keys in
init_pkru_value enabled:

    sigpkru = read_pkru() & init_pkru_value;

If user space protects the task stack or the sigalt stack with a key
which is not in init_pkru_value then it does not matter at all whether
it dies in handle_signal() or later when returning to user space, no?

I'm not fundamentaly opposed to enable all keys, but I don't buy this
without a proper explanation why this has been chosen over enabling only
the absolute minimum access rights.

Thanks,

        tglx