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

From: Dave Hansen
Date: Mon Oct 25 2021 - 16:08:10 EST


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?