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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> George Dunlap <george.dunlap@xxxxxxxxxx> 10/05/17 7:08 PM >>>
>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.

Oh, so it looks like I'm guilty here, as I think it was me who wrote it that
way there. I have to admit I don't really see why I wanted to save on stack
consumption there. In any event I'm then fine for you to leave it that way,
so the two places remain in sync (but I would also be fine if you changed
it here, and I'd then try to remember to clean it up on the other side).

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

Okay, that's what I had guessed. As said, please put this in a comment, the
more that you've realized this doesn't work all by itself (due to the MXCSR 
field
causing #GP when not sanitized _before_ doing the fxrstor). And the restore
from null then is to pre-init any (theoretical) fields the subsequent restore 
may
not touch at all?

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

"write" is fine, I think, as would be "ro" or "readonly".

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

As that's not as simple as FXSAVE/FXRSTOR (due to first needing to
discover area sizes) it's perhaps best to simply leave a TODO comment for
now.

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

Well, ideally individual pieces would be taken all-or-nothing, but due to the
varying sizes this would be rather cumbersome. So with the comment about
this being arbitrary add, I think this will be fine for the time being.

Jan


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