Re: [Xen-devel] [PATCH 5/7] fuzz/x86emul: update fuzzer

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 26/01/17 14:33, Jan Beulich wrote:
>>>> +        return X86EMUL_EXCEPTION;
>>>> +    else
>>>> +    {
>>>> +        if ( input.data[data_index] > 0xc )
>>>> +            rc = X86EMUL_EXCEPTION;
>>>> +        else if ( input.data[data_index] > 0x8 )
>>>> +            rc = X86EMUL_UNHANDLEABLE;
>>>
>>> How were these numbers chosen?
>>
>> The idea here is to give the fuzzer a way of (blindly) changing whether
>> the operation succeeds or how it fails in a "modular" way, by just
>> "consuming" one byte from the input.  The numbers are meant to make
>> random values roughly 50% succeed, 25% generate an exception, and 25%
>> return unhandleable.
> 
> But the vast majority of cases would fall in the exception group. Did
> you perhaps mean 0xc0 and 0x80 then? In any event a comment
> would be nice to clarify the intention.

Oh, yes -- that should definitely be 0xc0 and 0x80. :-)

>>>> +static int fuzz_cpuid(
>>>> +    uint32_t leaf,
>>>> +    uint32_t subleaf,
>>>> +    struct cpuid_leaf *res,
>>>> +    struct x86_emulate_ctxt *ctxt)
>>>> +{
>>>> +    int rc;
>>>> +
>>>> +    rc = maybe_fail("cpuid", true);
>>>> +
>>>> +    if ( rc == X86EMUL_OKAY )
>>>> +        emul_test_cpuid(leaf, subleaf, res, ctxt);
>>>> +
>>>> +    return rc;
>>>> +}
>>>
>>> Careful here: ->cpuid() is documented to only be allowed to fail with
>>> X86EMUL_EXCEPTION.
>>
>> But at the moment, the emulator seems to function properly even if you
>> return UNHANDLEABLE.  This is probably more robust than otherwise.
> 
> Hmm, yes and no. Considering it being documented that way,
> someone adding an ASSERT() to that effect would be a legitimate
> thing to do, yet would result in aborts for suitable input here.

Yes.  And I think that makes sense for how I was running it initially,
where the main person running it knows the delta between what is
"officially" required and what is actually required.  But that probably
needs revisiting.

I still think that knowing empirically what the emulation code will
accept (as opposed to what it's supposed to accept) is a useful thing,
but false positives aren't free.  If we had a list somewhere of the
places we deviate from the spec, it would be easier to determine if a
particular crash was a false positive or not.

I'll leave it for Wei now to decide whether he wants to document
deviations or just follow the spec in this instance.

>>>> + */
>>>> +void sanitize_input(struct x86_emulate_ctxt *ctxt) {
>>>> +    struct cpu_user_regs *regs = &input.regs;
>>>> +    unsigned long bitmap = input.options;
>>>> +
>>>> +    input.options &=
>>>> +        ~((1<<HOOK_read)|
>>>> +          (1<<HOOK_write)|
>>>> +          (1<<HOOK_cmpxchg)|
>>>> +          (1<<HOOK_insn_fetch));
>>>
>>> Ah, this addresses one of the earlier remarks. However, ->write
>>> and ->cmpxchg have become optional a little while ago.
>>
>> And yet, empirically, the emulator crashes if you don't have them.  If
>> this isn't expected, we should submit some patches.
> 
> Is that the case now, or was that when you wrote this code
> (before 4.8 went out)? As said, the change isn't all that old.

Oh, right -- I interpreted "a little while ago" as >6 months. :-)  Yes,
if it's supposed to handle not having write and cmpxchg, then we should
test not having them.

 -George


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