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

[Dave Hansen]

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=@mask
>>> + * 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?