linux-kernel - Re: [PATCH 15/23] x86/fpu: Add sanity checks for XFD

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 for Android: free password hash cracker in your pocket

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

Message-ID: <88cb75d3-01b9-38ea-e29f-b8fefb548573@intel.com>
Date:   Mon, 25 Oct 2021 12:57:50 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        linux-kernel@...r.kernel.org
Cc:     x86@...nel.org, dave.hansen@...ux.intel.com, arjan@...ux.intel.com,
        ravi.v.shankar@...el.com
Subject: Re: [PATCH 15/23] x86/fpu: Add sanity checks for XFD

On 10/25/21 11:13 AM, Thomas Gleixner wrote:
> On Mon, Oct 25 2021 at 11:33, Mika Penttilä wrote:
>> On 22.10.2021 1.55, Chang S. Bae wrote:
>>> +#ifdef CONFIG_X86_DEBUG_FPU
>>> +/*
>>> + * Ensure that a subsequent XSAVE* or XRSTOR* instruction with RFBM=@...k
>>> + * can safely operate on the @fpstate buffer.
>>> + */
>>> +static bool xstate_op_valid(struct fpstate *fpstate, u64 mask, bool rstor)
>>> +{
>>> +	u64 xfd = __this_cpu_read(xfd_state);
>>> +
>>> +	if (fpstate->xfd == xfd)
>>> +		return true;
>>> +
>>> +	/* For current's fpstate the XFD state must be correct. */
>>> +	if (fpstate->xfd == current->thread.fpu.fpstate->xfd)
>>> +		return false;
>>> +
>> Should this return true or is the comment confusing?
> Comment might be confusing. The logic here is:
> 
> If fpstate->xfd equal xfd then it's valid.
> 
> So the next check is whether fpstate is the same as current's
> fpstate. If that's the case then the result is invalid because for
> current's fpstate the first condition should be true. But if it is not
> true then the state is not valid.

Maybe I'm just a dummy today, but I spent way too much time looking at
that line of code.  Would a slightly longer comment help.  Perhaps:

	/*
	 * The XFD MSR doesn't match the passed-in fpstate.
 	 * Make sure it at least matches current's fpstate.
	 * If not, XFD was set up wrong and is invalid.
	 */

Below is an even more exhaustive look at it.  Feel free to ignore.

--

In most cases, we are doing a normal old context switch.  XFD is already
set up by this point and the @fpstate is pointed to by
current->thread.fpu.  fpstate->xfd==xfd, life is good and we skip the
rest of the checks.

If they don't match, we have some more checks to do.  Here's a
bit-by-bit walk through it.  Just like the hardware MSR at this point,
assume that only single bit can be set in any of the xfd's.

 MSR | fpstate | cur->fpstate | valid
-------------------------------------
   0 |	   0   |      	  x   |    1  // MSR matches @fpstate
   0 |	   1   |          0   |    1  // MSR matches cur->fpstate
   0 |	   1   |          1   |    0  <- *** MSR matches nothing!
   1 |	   0   |          0   |    0  <- *** MSR matches nothing!
   1 |	   0   |          1   |    1  // MSR matches cur->fpstate
   1 |	   1   |          x   |    1  // MSR matches @fpstate

 (BTW, "valid" here means either return true from earlier, or
  falling through to the later xstate_op_valid() checks)

If a bit is *CLEAR* in the MSR, then it better be *CLEAR* in either
fpstate->xfd or current->...fpstate->xfd.

If a bit is *SET*   in the MSR, then it better be *SET*   in either
fpstate->xfd or current->...fpstate->xfd.

It's bad if the MSR doesn't match *either* fpstate->xfd or
current->...fpstate->xfd.  Because, if it does not match, how did we get
here?  What random fpstate was the MSR set up to work with?