Re: [PATCH v3 2/4] x86/pkeys: Add helper functions to update PKRU on sigframe

[Thomas Gleixner <tglx@linutronix.de>]

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