Re: [patch 08/41] x86/fpu: Restrict fpstate sanitizing to legacy components

[Andy Lutomirski]

On 6/11/21 12:03 PM, Andy Lutomirski wrote:
> On 6/11/21 9:15 AM, Thomas Gleixner wrote:
>> xstateregs_get() does not longer use fpstate_sanitize_xstate() and the only
> 
> s/does not longer use/no longer uses/
> 
> 		\
>> --- a/arch/x86/kernel/fpu/regset.c
>> +++ b/arch/x86/kernel/fpu/regset.c
>> @@ -11,6 +11,39 @@
>>  
>>  #include <linux/sched/task_stack.h>
>>  
>> +/*
>> + * When executing XSAVEOPT (or other optimized XSAVE instructions), if
> 
> The kernel doesn't use XSAVEOPT any more.  How about:
> 
> When executing XSAVES (or other optimized XSAVE instructions)
> 
>> + * a processor implementation detects that an FPU state component is still
>> + * (or is again) in its initialized state, it may clear the corresponding
>> + * bit in the header.xfeatures field, and can skip the writeout of registers
>> + * to the corresponding memory layout.
> 
> Additionally, copy_xxx_to_xstate() may result in an xsave buffer with a
> bit clear in xfeatures but the corresponding state region not containing
> the state's init value.
> 
>> + *
>> + * This means that when the bit is zero, the state component might still
>> + * contain some previous - non-initialized register state.
> 
> Maybe say what the function does, e.g.:
> 
> This function fills in the init values for the X87 and SSE states if the
> corresponding xfeatures bits are clear.
> 
>> + *
>> + * This is required for the legacy regset functions.
>> + */
>> +static void fpstate_sanitize_legacy(struct fpu *fpu)
>> +{
>> +	struct fxregs_state *fx = &fpu->state.fxsave;
>> +	u64 xfeatures;
>> +
>> +	if (!use_xsaveopt())
>> +		return;
> 
> This is confusing, since we never use xsaveopt.  It's also wrong -- see
> above.  How about just removing it?
> 
>> +
>> +	xfeatures = fpu->state.xsave.header.xfeatures;
>> +
>> +	/* If FP is in init state, reinitialize it */
>> +	if (!(xfeatures & XFEATURE_MASK_FP)) {
>> +		memset(fx, 0, sizeof(*fx));
>> +		fx->cwd = 0x37f;
>> +	}
>> +
>> +	/* If SSE is in init state, clear the storage */
>> +	if (!(xfeatures & XFEATURE_MASK_SSE))
>> +		memset(fx->xmm_space, 0, sizeof(fx->xmm_space));
>> +}
>> +
>>  
> 
> Does this result in the mxcsr_mask and mxcsr fields being correct?
> There is a silly number of special cases there.
> 

The SDM says, in a footnote in XRSTOR:

There is an exception if RFBM[1] = 0 and RFBM[2] = 1. In this case, the
standard form of XRSTOR will load MXCSR from memory, even though MXCSR
is part of state component 1 — SSE. The compacted form of XRSTOR does
not make this exception.

which makes me think that this code has a bug.  Also, the code is
manifest nonsense for a different reason:

if (!FP)
  memset(the whole damn thing, 0);

if (!SSE)
  memset(just part of it, 0);

which means that, if the FP bit is clear but the SSE bit is set, this
thing will clobber the SSE state.

This code is garbage.  So is the architecture that gave rise to this code.

--Andy