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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.01.17 at 13:02, <george.dunlap@xxxxxxxxxx> wrote:
> On 26/01/17 11:28, Jan Beulich wrote:
>>>>> On 25.01.17 at 16:44, <wei.liu2@xxxxxxxxxx> wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
>>> +++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
>>> @@ -16,26 +16,75 @@
>>>  
>>>  #include "x86_emulate.h"
>>>  
>>> -static unsigned char data[4096];
>>> +#include "../../../xen/include/asm-x86/msr-index.h"
>>> +
>>> +#ifndef offsetof
>>> +#define offsetof(t,m) ((size_t)&(((t *)0)->m))
>>> +#endif
>> 
>> Why can't you include the standard header for this?
> 
> [Responding because this is code I wrote]
> 
> Because I didn't know where it was.  What's the best thing to include to
> get this?

The standard says stddef.h.

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

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

>>> +static void setup_fpu_exception_handler(void)
>>> +{
>>> +    /* FIXME - just disable exceptions for now */
>>> +    unsigned long a;
>>> +
>>> +    asm volatile ( "fnclex");
>>> +    a=0x3f;
>>> +    asm volatile ( "fldcw %0" :: "m" (a));
>>> +    a=0x1f80;
>>> +    asm volatile ( "ldmxcsr %0" :: "m" (a) );
>> 
>> I don't see what you need "a" for in this function. And I also wonder
>> how the FCW value was chosen (the MXCSR one is a commonly used
>> value).
> 
> I just copied it from somewhere.  Could you suggest something better?

The FCW_DEFAULT (0x037f) value from the hypervisor.

>>> +/*
>>> + * Constrain input to architecturally-possible states where
>>> + * the emulator relies on these
>>> + *
>>> + * In general we want the emulator to be as absolutely robust as
>>> + * possible; which means that we want to minimize the number of things
>>> + * it assumes about the input state.  Tesing this means minimizing and
>>> + * removing as much of the input constraints as possible.
>>> + *
>>> + * So we only add constraints that (in general) have been proven to
>>> + * cause crashes in the emulator.
>>> + *
>>> + * For future reference: other constraints which might be necessary at
>>> + * some point:
>>> + *
>>> + * - EFER.LMA => !EFLAGS.NT
>>> + * - In VM86 mode (and real mode?), force segment...
>> 
>> As per commit 05118b1596 real mode should not be included here.
>> 
>>> + *  - ...access rights to 0xf3
>>> + *  - ...limits to 0xffff
>>> + *  - ...bases to below 1Mb, 16-byte aligned
>>> + *  - ...selectors to (base >> 4)
>> 
>> Most of this does not match the implementation (which only clear
>> DB for CS and SS).
> 
> You mean that the code here doesn't actually do this?  Yes, that's what
> the comment says -- "For future reference, other constraints which may
> be necessary at some point" -- that is, restrictions which exist in the
> architecture but do not (yet) exist in this code.

Oh, yes, I'm sorry. When coming back up here from below (where
the respective code is) I didn't go back far enough in the comment
to note the "For future reference:".

>>> + */
>>> +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.

Jan

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