Re: [Xen-devel] [PATCH v4 11/12] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/12/2017 04:38 PM, Jan Beulich wrote:
>>>> On 11.10.17 at 19:52, <george.dunlap@xxxxxxxxxx> wrote:
>> The Intel manual claims that, "If [certain CPUID bits] are set, the
>> processor deprecates FCS and FDS, and the field is saved as 0000h";
>> but experimentally it would be more accurate to say, "the field is
>> occasionally saved as 0000h".  This causes the --rerun checking to
>> trip non-deterministically.  Sanitize them to zero.
> 
> I think we've meanwhile settled on the field being saved as zero
> being a side effect of using 32-bit fxsave plus a context switch in
> the OS kernel.
> 
>> @@ -594,6 +595,75 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>  };
>>  #undef SET
>>  
>> +/*
>> + * This funciton will read or write fxsave to the fpu.  When writing,
>> + * it 'sanitizes' the state: It will mask off the appropriate bits in
>> + * the mxcsr, 'restore' the state to the fpu, then 'save' it again so
>> + * that the data in fxsave reflects what's actually in the FPU.
>> + *
>> + * TODO: Extend state beyond just FPU (ymm registers, &c)
>> + */
>> +static void _set_fpu_state(char *fxsave, bool write)
>> +{
>> +    if ( cpu_has_fxsr )
>> +    {
>> +        static union __attribute__((__aligned__(16))) {
>> +            char x[512];
>> +            struct {
>> +                uint16_t cw, sw;
>> +                uint8_t  tw, _rsvd1;
>> +                uint16_t op;
>> +                uint32_t ip;
>> +                uint16_t cs, _rsvd2;
>> +                uint32_t dp;
>> +                uint16_t ds, _rsvd3;
>> +                uint32_t mxcsr;
>> +                uint32_t mxcsr_mask;
>> +                /* ... */
>> +            };
>> +        } *fxs;
>> +
>> +        fxs = (typeof(fxs))fxsave;
>> +
>> +        if ( write )
>> +        {
>> +            /* 
>> +             * Clear reserved bits to make sure we don't get any
>> +             * exceptions
>> +             */
>> +            fxs->mxcsr &= mxcsr_mask;
>> +
>> +            /*
>> +             * The Intel manual says that on newer models CS/DS are
>> +             * deprecated and that these fields "are saved as 0000h".
>> +             * Experimentally, however, at least on my test box,
>> +             * whether this saved as 0000h or as the previously
>> +             * written value is random; meaning that when run with
>> +             * --rerun, we occasionally detect a "state mismatch" in these
>> +             * bytes.  Instead, simply sanitize them to zero.
>> +             *
>> +             * TODO Check CPUID as specified in the manual before
>> +             * clearing
>> +             */
>> +            fxs->cs = fxs->ds = 0;
> 
> Shouldn't be needed anymore with ...
> 
>> +            asm volatile( "fxrstor %0" :: "m" (*fxs) );
> 
> rex64 (or fxrstor64) used here and ...
> 
>> +        }
>> +
>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
> 
> ... here (of course the alternative here then is fxsave64).
> 
> Also please add blanks before the opening parentheses.
> 
>> @@ -732,6 +806,18 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>              printf("Setting cpu_user_regs offset %x\n", offset);
>>              continue;
>>          }
>> +        offset -= sizeof(struct cpu_user_regs);
>> +
>> +        /* Fuzz fxsave state */
>> +        if ( offset < sizeof(s->fxsave) / 4 )
> 
> You've switched to sizeof() here but ...
> 
>> +        {
>> +            /* 32-bit size is arbitrary; see comment above */
>> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>> +                return;
>> +            printf("Setting fxsave offset %x\n", offset * 4);
>> +            continue;
>> +        }
>> +        offset -= 128;
> 
> ... not here.
> 
>> @@ -1008,6 +1098,16 @@ static void compare_states(struct fuzz_state state[2])
>>          if ( memcmp(&state[0].ops, &state[1].ops, sizeof(state[0].ops)) )
>>              printf("ops differ!\n");
>>  
>> +        if ( memcmp(&state[0].fxsave, &state[1].fxsave, 
>> sizeof(state[0].fxsave)) )
>> +        {
>> +            printf("fxsave differs!\n");
>> +            for ( i = 0;  i < sizeof(state[0].fxsave)/sizeof(unsigned); i++ 
>> )
> 
> Blanks around / again please.
> 
>> +            {
>> +                printf("[%04lu] %08x %08x\n",
> 
> I think I've indicated before that I consider leading zeros on decimal
> numbers misleading. 

Come to think of it I agree with you.

> Could I talk you into using %4lu instead (or
> really %4zu, considering the expression type) in places like this one
> (i.e. also in the earlier patch, where I notice only now the l -> z
> conversion wasn't done consistently either)?

/me looks up what %zu is supposed to do

Sure.

> 
>> +                        i * sizeof(unsigned), ((unsigned 
>> *)&state[0].fxsave)[i], ((unsigned *)&state[1].fxsave)[i]);
> 
> Long line.

Ack.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel