This patch adds helper functions that will update PKRU value on the
sigframe after XSAVE.

These functions will be called in a later patch; this patch does not
change any behavior as yet.

Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
---
 arch/x86/kernel/fpu/signal.c | 26 ++++++++++++++++++++++++++
 arch/x86/kernel/fpu/xstate.c | 13 +++++++++++++
 arch/x86/kernel/fpu/xstate.h |  1 +
 arch/x86/kernel/signal.c     | 12 ++++++++++++
 4 files changed, 52 insertions(+)

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index 7654f368accd..dce84cce7cf8 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -63,6 +63,32 @@ static inline bool check_xstate_in_sigframe(struct fxregs_state __user *fxbuf,
 	return true;
 }
 
+/*
+ * Update the value of PKRU register that was already pushed
+ * onto the signal frame.
+ */
+static inline int
+__update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)
+{
+	int err = -EFAULT;
+	struct _fpx_sw_bytes fx_sw;
+	struct pkru_state *pk = NULL;
+
+	if (unlikely(!check_xstate_in_sigframe((void __user *) buf, &fx_sw)))
+		goto out;
+
+	pk = get_xsave_addr_user(buf, XFEATURE_PKRU);
+	if (!pk || !user_write_access_begin(buf, sizeof(struct xregs_state)))
+		goto out;
+	unsafe_put_user(pkru, (unsigned int __user *) pk, uaccess_end);
+
+	err = 0;
+uaccess_end:
+	user_access_end();
+out:
+	return err;
+}
+
 /*
  * Signal frame handlers.
  */
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 33a214b1a4ce..8395a3633709 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -992,6 +992,19 @@ void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr)
 	return __raw_xsave_addr(xsave, xfeature_nr);
 }
 
+/*
+ * Given an xstate feature nr, calculate where in the xsave
+ * buffer the state is. The xsave buffer should be in standard
+ * format, not compacted (e.g. user mode signal frames).
+ */
+void *get_xsave_addr_user(struct xregs_state __user *xsave, int xfeature_nr)
+{
+	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
+		return NULL;
+
+	return (void *)xsave + xstate_offsets[xfeature_nr];
+}
+
 #ifdef CONFIG_ARCH_HAS_PKEYS
 
 /*
diff --git a/arch/x86/kernel/fpu/xstate.h b/arch/x86/kernel/fpu/xstate.h
index 19ca623ffa2a..6511797940ad 100644
--- a/arch/x86/kernel/fpu/xstate.h
+++ b/arch/x86/kernel/fpu/xstate.h
@@ -55,6 +55,7 @@ extern void fpu__init_cpu_xstate(void);
 extern void fpu__init_system_xstate(unsigned int legacy_size);
 
 extern void *get_xsave_addr(struct xregs_state *xsave, int xfeature_nr);
+extern void *get_xsave_addr_user(struct xregs_state *xsave, int xfeature_nr);
 
 static inline u64 xfeatures_mask_supervisor(void)
 {
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index d1c84b7f6852..75dfd05c59aa 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -225,6 +225,18 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs, u32 pkru)
 	}
 }
 
+/*
+ * Enable all pkeys to ensure that both the current stack and the alternate
+ * signal stack are always writeable.
+ */
+static inline u32 sig_prepare_pkru(void)
+{
+	u32 orig_pkru = read_pkru();
+
+	write_pkru(0);
+	return orig_pkru;
+}
+
 static void
 handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 {
-- 
2.39.3

On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
> This patch adds helper functions that will update PKRU value on the

git grep 'This patch' Documentation/process/

Also please explain WHY this is needed and not just what.

> sigframe after XSAVE.

...

> +/*
> + * Update the value of PKRU register that was already pushed
> + * onto the signal frame.
> + */
> +static inline int
> +__update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)

No line break and why does this need two underscores in the function name?

> +{
> +	int err = -EFAULT;
> +	struct _fpx_sw_bytes fx_sw;
> +	struct pkru_state *pk = NULL;

Why assign NULL to pk?

Also this wants to have a

	if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
     		return 0;

Instead of doing it at the call site.

> +	if (unlikely(!check_xstate_in_sigframe((void __user *) buf, &fx_sw)))

What is this check for?

More interesting: How is this check supposed to succeed at all?

copy_fpstate_to_sigframe()
  ....
  copy_fpregs_to_sigframe()
    xsave_to_user_sigframe();
    __update_pkru_in_sigframe();
  save_xstate_epilog();

check_xstate_in_sigframe() validates the full frame including what
save_xstate_epilog() writes afterwards. So this clearly cannot work.

> +		goto out;

What's wrong with 'return -EFAULT;'?

> +	pk = get_xsave_addr_user(buf, XFEATURE_PKRU);
> +	if (!pk || !user_write_access_begin(buf, sizeof(struct xregs_state)))
> +		goto out;

Why user_write_access_begin()?

    1) The access to the FPU frame on the stack has been validated
       already in copy_fpstate_to_sigframe() _before_
       copy_fpregs_to_sigframe() is invoked.

    2) This does not require the nospec_barrier() as this is not a user
       controlled potentially malicious access.

> +	unsafe_put_user(pkru, (unsigned int __user *) pk, uaccess_end);

This type case would need __force to be valid for make C=1.

But that's not required at all because get_xsave_addr_user() should
return a user pointer in the first place.

> +
> +	err = 0;
> +uaccess_end:
> +	user_access_end();
> +out:
> +	return err;

So none of the above voodoo is required at all.

       return __put_user(pkru, get_xsave_addr_user(buf, XFEATURE_PKRU));

Is all what's needed, no?

> +/*
> + * Given an xstate feature nr, calculate where in the xsave
> + * buffer the state is. The xsave buffer should be in standard
> + * format, not compacted (e.g. user mode signal frames).
> + */
> +void *get_xsave_addr_user(struct xregs_state __user *xsave, int xfeature_nr)

void __user *

> +{
> +	if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
> +		return NULL;
> +
> +	return (void *)xsave + xstate_offsets[xfeature_nr];

  return (void __user *)....

Thanks,

        tglx

> On May 7, 2024, at 9:16 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
>> This patch adds helper functions that will update PKRU value on the
> 
> git grep 'This patch' Documentation/process/
> 
> Also please explain WHY this is needed and not just what.


Patch 1/4 has the “why”. Should I expand more in that commit message?
Or are you specifically asking about why the helper functions are needed;
I can certainly elaborate on that here.

Thanks very much for your detailed review. I will make the required corrections.

Thanks,
Aruna
 
> 
>> sigframe after XSAVE.
> 
> ...
> 
>> +/*
>> + * Update the value of PKRU register that was already pushed
>> + * onto the signal frame.
>> + */
>> +static inline int
>> +__update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)
> 
> No line break and why does this need two underscores in the function name?
> 
>> +{
>> + int err = -EFAULT;
>> + struct _fpx_sw_bytes fx_sw;
>> + struct pkru_state *pk = NULL;
> 
> Why assign NULL to pk?
> 
> Also this wants to have a
> 
> if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
>      return 0;
> 
> Instead of doing it at the call site.
> 
>> + if (unlikely(!check_xstate_in_sigframe((void __user *) buf, &fx_sw)))
> 
> What is this check for?
> 
> More interesting: How is this check supposed to succeed at all?
> 
> copy_fpstate_to_sigframe()
>  ....
>  copy_fpregs_to_sigframe()
>    xsave_to_user_sigframe();
>    __update_pkru_in_sigframe();
>  save_xstate_epilog();
> 
> check_xstate_in_sigframe() validates the full frame including what
> save_xstate_epilog() writes afterwards. So this clearly cannot work.
> 
>> + goto out;
> 
> What's wrong with 'return -EFAULT;'?
> 
>> + pk = get_xsave_addr_user(buf, XFEATURE_PKRU);
>> + if (!pk || !user_write_access_begin(buf, sizeof(struct xregs_state)))
>> + goto out;
> 
> Why user_write_access_begin()?
> 
>    1) The access to the FPU frame on the stack has been validated
>       already in copy_fpstate_to_sigframe() _before_
>       copy_fpregs_to_sigframe() is invoked.
> 
>    2) This does not require the nospec_barrier() as this is not a user
>       controlled potentially malicious access.
> 
>> + unsafe_put_user(pkru, (unsigned int __user *) pk, uaccess_end);
> 
> This type case would need __force to be valid for make C=1.
> 
> But that's not required at all because get_xsave_addr_user() should
> return a user pointer in the first place.
> 
>> +
>> + err = 0;
>> +uaccess_end:
>> + user_access_end();
>> +out:
>> + return err;
> 
> So none of the above voodoo is required at all.
> 
>       return __put_user(pkru, get_xsave_addr_user(buf, XFEATURE_PKRU));
> 
> Is all what's needed, no?
> 
>> +/*
>> + * Given an xstate feature nr, calculate where in the xsave
>> + * buffer the state is. The xsave buffer should be in standard
>> + * format, not compacted (e.g. user mode signal frames).
>> + */
>> +void *get_xsave_addr_user(struct xregs_state __user *xsave, int xfeature_nr)
> 
> void __user *
> 
>> +{
>> + if (WARN_ON_ONCE(!xfeature_enabled(xfeature_nr)))
>> + return NULL;
>> +
>> + return (void *)xsave + xstate_offsets[xfeature_nr];
> 
>  return (void __user *)....
> 
> Thanks,
> 
>        tglx