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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

> @@ -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.)

Jan

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