Re: [RFC] Restore PKRU to user-defined value after signal handling

[Dave Hansen <dave.hansen@intel.com>]

On 11/6/24 10:33, Aruna Ramakrishna wrote:
>  static inline int update_pkru_in_sigframe(struct xregs_state __user *buf, u32 pkru)
>  {
> +       int err = 0;
> +
>         if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
>                 return 0;
> -       return __put_user(pkru, (unsigned int __user *)get_xsave_addr_user(buf, XFEATURE_PKRU));

Let me try to summarize that whole email:

The existing code updates the PKRU value in the XSAVE buffer.  But it
does not update ->xfeatures[PKRU].  If ->xfeatures[PKRU]==0, then XRSTOR
will ignore the data that __put_user() put in place.

How does ->xfeatures[PKRU] end up set to 0?  On AMD, a WRPKRU(0) sets
PKRU=0 *and* XINUSE[PKRU]=0.  Intel doesn't do that.  Either behavior is
architecturally permitted.

Did I miss anything?

But the suggested fix is just beyond hideous.  Can't we just use the
mask that xsave_to_user_sigframe() generated instead of reading it back
out of userspace three seconds after it is written?

static inline int update_pkru_in_sigframe(..., u32 mask)
{
	u32 xinuse;
	int err;

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

	/* Ensure XRSTOR picks up the new PKRU value from the buffer: */
	xinuse = (mask & xfeatures_in_use()) | XFEATURE_MASK_PKRU;

	err =  __put_user(xinuse, &buf->header.xfeatures);
	if (err)
		return err;

        return ... existing code here;
}

This probably means moving update_pkru_in_sigframe() to the end of
xsave_to_user_sigframe() instead of calling it after, though.

But either way, this is all horrific.  It's yet another reason that the
XSAVE architecture complexity hurts more than it helps.  We want PKRU
written out here, dammit.  We shouldn't have to ask the hardware to
write it out, and _then_ go back and do it ourselves.