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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4d484280-3bed-453f-b2f6-0619df4e9914@intel.com>
Date: Wed, 6 Nov 2024 11:27:05 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Aruna Ramakrishna <aruna.ramakrishna@...cle.com>,
 Thomas Gleixner <tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
 "x86@...nel.org" <x86@...nel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc: Rudi Horn <rudi.horn@...cle.com>, Joe Jin <joe.jin@...cle.com>,
 Jeff Xu <jeffxu@...omium.org>
Subject: Re: [RFC] Restore PKRU to user-defined value after signal handling

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.