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.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
