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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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?

> 
>> +#define MSR_INDEX_MAX 16
>> +
>> +#define SEG_MAX x86_seg_none
>> +
>> +struct input_struct {
>> +    unsigned cr[5];
> 
> unsigned long?
> 
>> +    uint64_t msr[MSR_INDEX_MAX];
>> +    struct cpu_user_regs regs;
>> +    struct segment_register segments[SEG_MAX+1];
> 
> Either SEG_MAX needs to be x86_seg_none - 1, or it is slightly
> mis-named (should be SEG_NUM) and the +1 here is not needed.
> 
>> +int maybe_fail(const char *why, bool exception)
>> +{
>> +    int rc;
>> +
>> +    if ( data_index + 1 > data_max )
> 
> The naming issue makes this look wrong too - I think you again mean
> data_num or data_amount, not data_max.
> 
>> +        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.

In theory these could be any numbers; it could be 0 -> EXCEPTION, 1 ->
UNHANDLEABLE, anything else -> OKAY.

> 
>> +        else
>> +            rc = X86EMUL_OKAY;
>> +        data_index++;
>> +    }
>> +
>> +    if ( rc == X86EMUL_EXCEPTION && !exception )
>> +        rc = X86EMUL_OKAY;
> 
> This could do with a comment explaining the intention.

This allows the caller to control whether the operation is allowed to
return an EXCEPTION.  If it's not, change any EXCEPTION values into OKAY.

> 
>> +    printf("maybe_fail %s: %d\n", why, rc);
>> +
>> +    return rc;
>> +}
>> +
>>  static int data_read(const char *why, void *dst, unsigned int bytes)
>>  {
>>      unsigned int i;
>> +    int rc;
>>  
>>      if ( data_index + bytes > data_max )
>>          return X86EMUL_EXCEPTION;
>> +    else
>> +        rc = maybe_fail(why, true);
> 
> No need for "else".
> 
>> @@ -48,14 +97,71 @@ static int fuzz_read(
>>      return data_read("read", p_data, bytes);
>>  }
>>  
>> -static int fuzz_fetch(
>> +static int fuzz_read_io(
>> +    unsigned int port,
>> +    unsigned int bytes,
>> +    unsigned long *val,
>> +    struct x86_emulate_ctxt *ctxt)
>> +{
>> +    return data_read("read_io", val, bytes);
>> +}
>> +
>> +static int fuzz_insn_fetch(
>>      unsigned int seg,
>>      unsigned long offset,
>>      void *p_data,
>>      unsigned int bytes,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> -    return data_read("fetch", p_data, bytes);
>> +    return data_read("insn_fetch", p_data, bytes);
> 
> Why is "fetch" not good enough? It looks like there may be a lot of
> output, so any compaction opportunity may be worthwhile to use.

Well the function name has been changed so that I can use the macro
below to enable / disable the hook, rather than having to open-code each
one; and the printf is there to match the function name.

Most of the tests only had a few hundred lines of output, so although
it's a bike shed, I'd prefer "make the printout match the actual name of
the hook".

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

>> +int msr_index[MSR_INDEX_MAX] = {
> 
> static unsigned int (and maybe also const, I didn't check whether it
> gets written to).

No, I think this is a static "reverse mapping" of MSR number to storage
index.

> 
>> +static int fuzz_read_msr_(
> 
> Please use a leading rather than a trailing underscore (unless there's
> a reason for you doing it that way).

I think there was a reason but I can't remember what it was.   I guess
we can try it and see what happens. :-)

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

>> +/* Expects bitmap to be defined */
>> +#define MAYBE_DISABLE_HOOK(h)                          \
>> +    if ( bitmap & (1 << HOOK_##h) )                    \
>> +    {                                                  \
>> +        fuzz_emulops.h = NULL;                         \
>> +        printf("Disabling hook "#h"\n");               \
>> +    }
>> +
>> +static void disable_hooks(void)
>> +{
>> +    unsigned long bitmap = input.options;
>> +
>> +    MAYBE_DISABLE_HOOK(read);
>> +    MAYBE_DISABLE_HOOK(insn_fetch);
> 
> -> read and ->insn_fetch must not be NULL (there are ASSERT()s to
> that effect).

I wanted to have all the information about what "quirks" or requirements
the emulator had empirically in one place.  So have the code to disable
it, but filter out input that would do so in the "sanitize" section.

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

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

 -George


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