Re: [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

[David Vrabel <david.vrabel@citrix.com>]

On 23/02/16 14:59, Jan Beulich wrote:
>>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> wrote:
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -263,41 +263,24 @@ void xsave(struct vcpu *v, uint64_t mask)
>>  
>>      if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
>>      {
>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>> +        uint64_t bad_fip;
>>  
>> -        if ( cpu_has_xsaveopt || cpu_has_xsaves )
>> -        {
>> -            /*
>> -             * XSAVEOPT/XSAVES may not write the FPU portion even when the
>> -             * respective mask bit is set. For the check further down to work
>> -             * we hence need to put the save image back into the state that
>> -             * it was in right after the previous XSAVEOPT.
>> -             */
>> -            if ( word_size > 0 &&
>> -                 (ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 4 ||
>> -                  ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 2) )
>> -            {
>> -                ptr->fpu_sse.fip.sel = 0;
>> -                ptr->fpu_sse.fdp.sel = 0;
>> -            }
>> -        }
>> +        /*
>> +         * FIP/FDP may not be written in some cases (e.g., if
>> +         * XSAVEOPT/XSAVES is used, or on AMD CPUs if an exception
>> +         * isn't pending).
>> +         *
>> +         * To tell if the hardware writes these fields, make the FIP
>> +         * field non-canonical by flipping the top bit.
>> +         */
>> +        bad_fip = ptr->fpu_sse.fip.addr ^= 1ull << 63;
>>  
>>          XSAVE("0x48,");
>>  
>> -        if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>> -             /*
>> -              * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
>> -              * is pending.
>> -              */
>> -             (!(ptr->fpu_sse.fsw & 0x0080) &&
>> -              boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>> +        /* FIP/FDP not updated? Restore the old FIP value. */
>> +        if ( ptr->fpu_sse.fip.addr == bad_fip )
>>          {
>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>> -            {
>> -                ptr->fpu_sse.fip.sel = fcs;
>> -                ptr->fpu_sse.fdp.sel = fds;
>> -            }
>> +            ptr->fpu_sse.fip.addr ^= 1ull << 63;
>>              return;
>>          }
> 
> While indeed this is a lot more simple, it puts us on thin ice,
> utilizing undocumented behavior: You make us depend on FIP
> actually being a 48-bit register which gets sign-extended to 64
> bits upon saving, and truncated during restore. While all CPUs
> I've tested so far match this requirement, Intel ones (other
> than AMD's) do not match this in behavior for FDP. Since this
> already makes clear that AMD's are buggy (losing relevant
> state, since FPU operations using FS: or GS: may use non-
> canonical virtual addresses, becoming canonical once
> converted to linear ones) and hence need fixing, it would
> remain to be seen whether they wouldn't at once extend both
> FDP and FIP to 64 bits.

I'm not sure what you're concerned about:

a) Executing a FP instruction might load FIP with a non-canonical RIP?

b) All 2^64 addresses might be canonical if the valid virtual address is
64-bits wide?

c) A guest might load arbitrary data into a 64-bit wide FIP register
(which may look like a non-canonical address)?

But whatever, I'll drop this patch.

(I would hope that new AMD processors go down the Intel route and remove
the FCS/FDS registers.)

David