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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/06/2017 07:10 AM, Jan Beulich wrote:
>>>> 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).

Well I don't think this function really looks much at all like
emul_test_init(); and I think it makes sense to keep this bit and the
"null" load below identical, so I'll change it to 512.

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

Yes; as I said, those two instructions were copied-and-pasted from
stackoverflow (or some other website); and I seem to recall them saying
that architecturally, if a certain amount of the "load data" was zero,
that the unit would simply do a full reset.

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

OK.

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

OK.

 -George

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