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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wei.liu2@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 2/2] x86/AMD: Fix handling of x87 exception pointers on Fam17h hardware
Date: Mon, 2 Sep 2019 15:15:27 +0100	[thread overview]
Message-ID: <d273e361-bcf9-c136-166b-b860c13e767e@citrix.com> (raw)
In-Reply-To: <e9de2d38-3266-b4c1-de73-cf9d0aef95c7@suse.com>

On 29/08/2019 13:56, Jan Beulich wrote:
> On 19.08.2019 20:26, 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
>> workaround path by dropping the data-dependent unpredictable conditions which
>> will evalute to true for all 64bit OSes and most 32bit ones.
> I definitely don't buy the "all 64bit OSes" part here: Anyone doing
> full 80-bit FP operations will have to use the FPU, and hence may
> want to have some unmasked exceptions.

And all 0 people doing that is still 0.

Yes I'm being a little facetious, but there is exceedingly little
software which uses 80-bit FPU operations these days, as it has been
superseded by SSE.

>  I'm also not sure why you
> call them "unpredictable": If all (or most) cases match, the branch
> there could be pretty well predicted (subject of course to capacity).

Data-dependent branches which have no correlation to pattern history, of
which this is an example, are frequently mispredicted because they are
inherently unstable.

In this case, you're trading off the fact that an unmasked exception is
basically never pending, against the cost of mispredicts in the context
switch path.

> All in all I'd prefer if the conditions remained in place; my minimal
> request would be for there to be a comment why there's no evaluation
> of FSW/FCW.
>
>> --- a/xen/arch/x86/i387.c
>> +++ b/xen/arch/x86/i387.c
>> @@ -43,20 +43,17 @@ static inline void fpu_fxrstor(struct vcpu *v)
>>      const typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
>>  
>>      /*
>> -     * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
>> +     * Some CPUs don't save/restore FDP/FIP/FOP unless an exception
> Are there any non-AMD CPUs known to have this issue? If not, is
> there a particular reason you don't say "Some AMD CPUs ..."?

I'm not aware of any, but leaving it as "Some AMD" might become stale if
others do surface.

Information about which CPUs are affected should exclusively be
determined by the logic which sets cpu_bug_fpu_ptr_leak, which won't be
stale.

>>       * is pending. Clear the x87 state here by setting it to fixed
>>       * values. The hypervisor data segment can be sometimes 0 and
>>       * sometimes new user value. Both should be ok. Use the FPU saved
>>       * data block as a safe address because it should be in L1.
>>       */
>> -    if ( !(fpu_ctxt->fsw & ~fpu_ctxt->fcw & 0x003f) &&
>> -         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>> -    {
>> +    if ( cpu_bug_fpu_ptr_leak )
>>          asm volatile ( "fnclex\n\t"
>>                         "ffree %%st(7)\n\t" /* clear stack tag */
>>                         "fildl %0"          /* load to clear state */
>>                         : : "m" (*fpu_ctxt) );
> If here and in the respective xsave instance you'd use alternatives
> patching, I wouldn't mind the use of a X86_BUG_* for this (as made
> possible by patch 1).

a) this should probably be a static branch if/when we gain that
infrastructure, but ...

> But as said before, just like for synthetic
> features I strongly think we should use simple boolean variables
> when using them only in if()-s. Use of the feature(/bug) machinery
> is needed only to not further complicate alternatives patching.

... b)

I see I'm going to have to repeat myself, which is time I can't really
afford to waste.

x86_capabilities is not, and has never been, "just for alternatives". 
It is also not how it is currently used in Xen.

I also don't agree with the general suggestion because amongst other
things, there is a factor of 8 storage difference between one extra bit
in x86_capabilities[] and using scattered booleans.

This series, and a number of related series, have been overdue for more
than a year now, partly because of speculative preemption, but also
partly because of attempted scope creep such as this.  Scope creep is
having a catastrophic effect on the productivity of submissions to Xen,
and most not continue like this the Xen community is to survive.

>
>> @@ -169,11 +166,10 @@ 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.  The restore code fills in suitable defaults.
>>           */
>> -        if ( !(fpu_ctxt->fsw & 0x0080) &&
>> -             boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
>> +        if ( cpu_bug_fpu_ptr_leak && !(fpu_ctxt->fsw & 0x0080) )
>>              return;
> The comment addition seems a little unmotivated:

Well.  Judging by your reply, it is "too complicated for even Andrew to
follow", so absolutely needs to be clearer.

>  The code here isn't
> about leaking data, but about having valid data to consume (down
> from here).

Ok - I see that now.

>  With this, keying the return to cpu_bug_* also doesn't
> look very nice, but I admit I can't suggest a better alternative
> (other than leaving the vendor check in place and checking the
> X86_FEATURE_RSTR_FP_ERR_PTRS bit).
>
> An option might be to give the construct a different name, without
> "leak" in it (NO_FP_ERR_PTRS?).

This name also isn't ideal, because its not always that there are no
error pointers.

X86_BUG_FPU_PTRS might be best, as it is neutral as to precisely what is
buggy with them.

~Andrew

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