Re: [patch 06/41] x86/fpu: Sanitize xstateregs_set()

[Thomas Gleixner]

On Fri, Jun 11 2021 at 11:45, Andy Lutomirski wrote:
> On 6/11/21 9:15 AM, Thomas Gleixner wrote:
>> xstateregs_set() operates on a stopped task and tries to copy the provided
>> buffer into the tasks fpu.state.xsave buffer.
>> 
>> Any error while copying or invalid state detected after copying results in
>> wiping the target tasks FPU state completely including supervisor states.
>> 
>> That's just wrong. The caller supplied invalid data or has a problem with
>> unmapped memory, so there is absolutely no justification to corrupt the
>> target state.
>> 
>> @@ -1146,14 +1146,16 @@ int copy_kernel_to_xstate(struct xregs_s
>>  	 */
>>  	xsave->header.xfeatures |= hdr.xfeatures;
>>  
>> +	/* mxcsr reserved bits must be masked to zero for security reasons. */
>> +	xsave->i387.mxcsr &= mxcsr_feature_mask;
>
> This comment is vague.  At least it should say:
>
> A subsequent XRSTOR(S) will fail if MXCSR has bits set that are not
> accepted by the current CPU.  Mask out unsupported bits.
>
> But a much nicer fix IMO would be to just return an error.

That was the "fix" for CVE-2003-0248, but I forgot the details of this
issue and why the masking was done instead of returning an error. The
problem was not that it caused a #GP, there was some other implication
which I can't remember.

The reason why this masking was done was that the SDM was pretty vague
about the reserved bits and only newer processor generations had an
issue while older ones just ignored them and of course some existing
user space was sloppy and got away with random values in reserved bits
up to the point where newer CPUs got more sensitive about this.

Yes, the comment could get some love and we probably can just error out
today. Those ancient binaries should be in the bit bucket by now.

Thanks,

        tglx