Re: [Xen-devel] [PATCH v2 10/13] fuzz/x86_emulate: Make input more compact

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.17 at 17:04, <george.dunlap@xxxxxxxxxx> wrote:
> On 10/04/2017 09:26 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>>> @@ -53,6 +53,15 @@ struct fuzz_state
>>>  };
>>>  #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>>>  
>>> +bool opt_compact;
>>> +
>>> +unsigned int fuzz_minimal_input_size(void)
>>> +{
>>> +    if ( opt_compact )
>>> +        return sizeof(unsigned long) + 1;
>> 
>> What is this value choice based on / derived from? Oh, judging from
>> code further down it may be one more than the size of the options
>> field, in which case it should be sizeof(...->options) here.
> 
> What about renaming DATA_OFFSET to DATA_SIZE_FULL, and adding
> DATA_SIZE_COMPACT?
> 
> Then is could be:
> 
>     return (opt_compact ? DATA_SIZE_COMPACT : DATA_SIZE_FULL) + 1;

Looks fine.

>>> +            exit(-1);
>>> +        return;
>>> +    }
>>> +
>>> +    /* Modify only select bits of state */
>>> +
>>> +    /* Always read 'options' */
>>> +    if ( !input_read(s, &s->options, sizeof(s->options)) )
>>> +        return;
>>> +    
>>> +    while(1) {
>> 
>> Style. And for compatibility (read: no warnings) with as wide a range
>> of compilers as possible, generally for ( ; ; ) is better to use.
> 
> I can do that; but would you mind explaining?  What kinds of compilers
> don't like while(1)?

In various projects of my own I have on (and targeting) Windows
I see this with almost every compiler I happen to use there. Hence
I've started to avoid the construct many years ago.

>>> +        {
>>> +            if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
>>> +                return;
>>> +            printf("Setting MSR i%d (%x) to %lx\n", offset,
>>> +                   msr_index[offset], s->msr[offset]);
>>> +            continue;
>>> +        }
>>> +
>>> +        offset -= MSR_INDEX_MAX;
>>> +
>>> +        /* segments[]? */
>>> +        if ( offset < SEG_NUM )
>>> +        {
>>> +            if ( !input_read(s, s->segments + offset, 
>>> sizeof(*s->segments)) )
>>> +                return;
>>> +            printf("Setting Segment %d\n", offset);
>>> +            continue;
>>> +            
>>> +        }
>>> +
>>> +        offset -= SEG_NUM;
>>> +
>>> +        /* regs? */
>>> +        if ( offset < sizeof(struct cpu_user_regs)
>>> +             && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
>>> +        {
>>> +            if ( !input_read(s, ((char *)ctxt->regs) + offset, 
>>> sizeof(uint64_t)) )
>>> +                return;
>>> +            printf("Setting cpu_user_regs offset %x\n", offset);
>>> +            continue;
>>> +        }
>>> +
>>> +        /* None of the above -- take that as "start emulating" */
>>> +        
>>> +        return;
>>> +    }
>> 
>> Having come here I wonder whether the use of "byte" in the description
>> is right, and you mean "uint8_t offset" above, as you're far from
>> consuming the entire 256 value range.
> 
> Isn't cpu_user_regs larger than 256 bytes?  And in any case, the offset
> will become larger than 256 bytes one we include the FPU state.

Oh, of course. I've somehow stopped summing at the point 
SEG_NUM is being subtracted.

>> Additionally, was the order of elements here chosen for any specific
>> reason? It would seem to me that elements having a more significant
>> effect on emulation may be worth filling first, and I'm not convinced
>> the "all CRs, all MSRs, all SREGs, all GPRs" order matches that.
> 
> I'm not aware of any particular order; it's probably some combination of
> "the order they were in the cpu_regs struct" and "the order in which I
> found it useful to add them".  Given that the input will be more or less
> random, I don't think the order in the struct will have too much of an
> impact on the order in which AFL explores them.

Well, yes, except for very small input (which will leave "higher"
parts unrandomized).

> If you have an alternative suggestion for an order you think would be
> more logical I'm happy to rearrange the structure.

Generally I'd expect GPRs to be most relevant to change value,
but them going first might be sort of ugly, as they're quite big.
For the others I'd say SREGs, CRs, then MSRs. But if it doesn't
really matter (i.e. if small input isn't of much concern, as you
suggest above), you may as well leave things the way they are.

Jan

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