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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> wrote:
>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>  };
>>  #undef SET
>>  
>> +static void _set_fpu_state(char *fxsave, bool store)
>> +{
>> +    if ( cpu_has_fxsr )
>> +    {
>> +        static union __attribute__((__aligned__(16))) {
>> +            char x[464];
> 
> The final part of the save area isn't being written, yes, but is it
> really worth saving the few bytes of stack space here, rather than
> having the expected 512 as array dimension?

So I didn't actually look into this very much; I mainly just hacked at
it until it seemed to work.  I copied-and-pasted emul_test_init() from
x86_emulate.c (which is where the 464 came from), then copied some
scraps of asm from stackoverflow.

>> +            struct {
>> +                uint32_t other[6];
>> +                uint32_t mxcsr;
>> +                uint32_t mxcsr_mask;
>> +                /* ... */
>> +            };
>> +        } *fxs;
>> +
>> +        fxs = (typeof(fxs)) fxsave;
>> +
>> +        if ( store ) {
> 
> Style.
> 
>> +            char null[512] __attribute__((aligned(16))) = { 0 };
> 
> No need for the 0 and a blank line between declaration and statements
> please.
> 
>> +            asm volatile(" fxrstor %0; "::"m"(*null));
>> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));
> 
> Style again - these want to follow the
> 
>     asm volatile ( "..." :: "m" (...) )
> 
> form. No need for the ; following the instructions.
>
>> +        }
>> +        
>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
> 
> This is pretty confusing, the more with the different variable names
> used which point to the same piece of memory. You basically store back
> into the area you've read from. Is the caller expecting the memory area
> to change? Is this being done other than for convenience to not have
> another instance of scratch space on the stack? Some comment on the
> intentions may be helpful here.

Yes, sorry for the different variable names.  I should have done a
better clean-up of this patch.

As for why it's doing an fxsave after just doing an fxrstor: I had the
idea that what came out via fxsave might not be the same as what was
written via fxrstor (i.e., the instruction would "interpret" the data),
particularly as what went in would be completely random fuzzed state.
The idea behind doing the restore / save was to "sanitize" the state in
the state struct to look more like real input data.

> The function's parameter name being "store" adds to the confusion,
> since what it controls is actually what we call "load" on x86 (or
> "restore" following the insn mnemonics).

I chose 'store' as the argument name before I realized that fxrstor was
"fx restore" and not "fxr store".

Do you think 'write' would be suitable?  Names like "restore" or "load"
make sense if you're thinking about things from the processor's
perspective (as the architects certainly were); but they make less sense
from a programmer's perspective, since (to me anyway) it seems like I'm
writing to or reading from the FPU unit (rather than loading/restoring
or saving).

If you don't like 'write' I'll change it to 'restore'.

> And then - what about YMM register state? Other more exotic registers
> (like the BND* ones) may indeed not be that relevant to fuzz yet.

I can look into that if you want, or if you want to give me some runes
to copy in I'm happy to do that as well.

>> @@ -737,6 +780,17 @@ 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 < 128 )
>> +        {
>> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>> +                return;
>> +            printf("Setting fxsave offset %x\n", offset * 4);
> 
> What's this 32-bit granularity derived from?

Just seemed like a good-sized chunk.  Doing it byte-by-byte seemed to be
"wasting" input on offsets (as in the input you'd have a 2-byte 'offset'
followed by a one-byte bit of data).  This way you have a 2-byte offset
and a 4-byte chunk of data that you write.

Let me know if you think there's a better size for chunks of data to
write.  In any case I'll add a comment in here to let people know that
the size is arbitrary.

>> @@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
>>          s->segments[x86_seg_cs].db = 0;
>>          s->segments[x86_seg_ss].db = 0;
>>      }
>> +
>> +    /* Setting this value seems to cause crashes in fxrstor */
>> +    *((unsigned int *)(s->fxsave) + 6) = 0;
> 
> That's the MXCSR field - instead of storing zero you want to mask with
> mxcsr_mask. To avoid the ugly literal 6 (and to make clear what it is
> that needs adjustment here) it may also be worthwhile widening the
> scope of the type declared in _set_fpu_state() and use it for struct
> fuzz_state's fxsave field.

Got it.

 -George

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