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:
 <SJ0PR10MB47208C97D877C27053E546DC9C5C2@SJ0PR10MB4720.namprd10.prod.outlook.com>
Date: Thu, 7 Nov 2024 11:41:15 +0000
From: Rudi Horn <rudi.horn@...cle.com>
To: Dave Hansen <dave.hansen@...el.com>,
        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: Joe Jin <joe.jin@...cle.com>, Jeff Xu <jeffxu@...omium.org>
Subject: Re: [RFC] Restore PKRU to user-defined value after signal handling

Hi all,

> But the suggested fix is just beyond hideous. 

I am new to the kernel mailing list, but I would like to refer to 
https://subspace.kernel.org/etiquette.html#be-terse-but-polite.

> It's yet another reason that the XSAVE architecture complexity hurts more than it helps.

The XSTATE architecture simply guarantees that an XRSTR using the state 
recorded by an XSAVE yields the same processor state. It can prevent dirtying
cache lines by specifying which processor state can just be restored to its 
zero'd state. All we are doing with this change, is maintaining the invariant that
the xfeatures value matches the remaining data stored in the xstate buffer.

> 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?

This is technically sound, but at this point in the code it is making the
assumption that xsave_to_user_sigframe() currently matches the value stored
in the xstate. This is more fragile if any further changes to the xstate are made.

However, we could consider the the kernels representation of XSTATE to be 
a pointer to the xsave buffer and a kernel value of the xfeature field. The
xsave_to_user_sigframe() would then compute and return the xfeatures 
field as you requested, and the xfeatures value should written to the xsave buffer 
at the end of copy_fpregs_to_sigframe, possibly being conditional on if it was dirtied.

Then update_pkru_in_sigframe then just becomes:

/*
 * 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 *xfeatures, u32 pkru)
{
	if (unlikely(!cpu_feature_enabled(X86_FEATURE_OSPKE)))
		return 0;

	if (pkru != 0) {
		/* Mark PKRU XSTATE section as in use. */
		*xfeatures |= XFEATURE_MASK_PKRU;

		return __put_user(pkru,
				  (unsigned int __user *)get_xsave_addr_user(
					  buf, XFEATURE_PKRU));
	}

	return 0;
}

I think maintaining this invariant is a fair step to perform when fiddling
with somewhat architecture-internal data structures.

Thanks,
Rudi

________________________________________
From: Dave Hansen <dave.hansen@...el.com>
Sent: Wednesday, 6 November 2024 20:27
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.