Re: [Xen-devel] [PATCH v3 2/2] x86/AMD: Fix handling of x87 exception pointers on Fam17h hardware

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/09/2019 10:00, Jan Beulich wrote:
> On 04.09.2019 19:57, Andrew Cooper wrote:
>> AMD Pre-Fam17h CPUs "optimise" {F,}X{SAVE,RSTOR} by not saving/restoring
>> FOP/FIP/FDP if an x87 exception isn't pending.  This causes an information
>> leak, CVE-2006-1056, and worked around by several OSes, including Xen.  AMD
>> Fam17h CPUs no longer have this leak, and advertise so in a CPUID bit.
>>
>> Introduce the RSTR_FP_ERR_PTRS feature, as specified by AMD, and expose to 
>> all
>> guests by default.  While adjusting libxl's cpuid table, add CLZERO which
>> looks to have been omitted previously.
>>
>> Also introduce an X86_BUG bit to trigger the (F)XRSTOR workaround, and set it
>> on AMD hardware where RSTR_FP_ERR_PTRS is not advertised.  Optimise the
>> conditions for the workaround paths.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> irrespective of a few remarks:
>
>> v3:
>>  * Rename to X86_BUG_FPU_PTRS
> While I did agree to use this name, I'd still like to point out that
> whether or not this is viewed as a bug is a matter of the position
> one takes. I'm pretty sure the AMD engineers originally having decided
> to avoid saving/restoring these value wouldn't have called this a bug,
> but a feature.

I accept that different people might have different opinions on the
matter, but at the point that we and every other large software vendor
has issued a security fix because of it, it can't credibly be called a
feature, irrespective of the original intention.

The change of behaviour on Fam17h is either tacit agreement with this
point, or at least a begrudging acceptance that the only way the
workaround is going to go away is by changing the behaviour of the CPU.

>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -580,6 +580,13 @@ static void init_amd(struct cpuinfo_x86 *c)
>>      }
>>  
>>      /*
>> +     * Older AMD CPUs don't save/load FOP/FIP/FDP unless an FPU exception
>> +     * is pending.  Xen works around this at (F)XRSTOR time.
>> +     */
>> +    if ( !cpu_has(c, X86_FEATURE_RSTR_FP_ERR_PTRS) )
>> +            setup_force_cpu_cap(X86_BUG_FPU_PTRS);
> I think in this file you want to omit the blanks immediately inside
> the if() parentheses.

Oops yes.

>
>> @@ -169,11 +167,11 @@ static inline void fpu_fxsave(struct vcpu *v)
>>                         : "=m" (*fpu_ctxt) : "R" (fpu_ctxt) );
>>  
>>          /*
>> -         * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
>> -         * is pending.
>> +         * Some CPUs don't save/restore FDP/FIP/FOP unless an exception is
>> +         * pending.  In this case, the restore side will arrange safe 
>> values,
>> +         * and there is no point trying to restore FCS/FDS in addition.
>>           */
>> -        if ( !(fpu_ctxt->fsw & 0x0080) &&
>> -             boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>> +        if ( cpu_bug_fpu_ptrs && !(fpu_ctxt->fsw & 0x0080) )
>>              return;
> Could I talk you into s/trying to restore/trying to collect/ for the
> comment? The consumer of the collected data could in principle be
> other than the corresponding restore code, e.g. the insn emulator.
> (hvmemul_put_fpu() has an example of the opposite direction, i.e.
> producing data for the restore logic to consume.)

Ok.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel