[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

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

> +        else
> +            rc = X86EMUL_OKAY;
> +        data_index++;
> +    }
> +
> +    if ( rc == X86EMUL_EXCEPTION && !exception )
> +        rc = X86EMUL_OKAY;

This could do with a comment explaining the intention.

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

> +static int fuzz_rep_movs(
> +    enum x86_segment src_seg,
> +    unsigned long src_offset,
> +    enum x86_segment dst_seg,
> +    unsigned long dst_offset,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc;
> +    unsigned long bytes_read;
> +    rc = data_read("rep_ins", &bytes_read, sizeof(bytes_read));

"rep_movs"

> +static int fuzz_rep_stos(
> +    void *p_data,
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return maybe_fail("rep_stos", true);
>  }

Perhaps better moved next to rep_movs().

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

> +static int fuzz_read_segment(
> +    enum x86_segment seg,
> +    struct segment_register *reg,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc;
> +
> +    if ( seg > SEG_MAX )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    rc = maybe_fail("read_segment", true);
> +
> +    if ( rc == X86EMUL_OKAY )
> +        memcpy(reg, input.segments+seg, sizeof(struct segment_register));
> +
> +    return rc;
> +}

This continues from the earlier comment: x86_seg_none must not
make it here (i.e. should by included in the early bailing).

> +static int fuzz_write_segment(
> +    enum x86_segment seg,
> +    const struct segment_register *reg,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc;
> +
> +    if ( seg > SEG_MAX )
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    rc = maybe_fail("write_segment", true);
> +
> +    if ( rc == X86EMUL_OKAY )
> +        memcpy(input.segments+seg, reg, sizeof(struct segment_register));
> +
> +    return rc;
> +}

Even more so here then.

> +static int fuzz_read_cr(
> +    unsigned int reg,
> +    unsigned long *val,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc;
> +
> +    if ( reg > 5 )

ARRAY_SIZE()

> +static int fuzz_write_cr(
> +    unsigned int reg,
> +    unsigned long val,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    int rc;
> +
> +    if ( reg > 5 )

Same here.

> +int msr_index[MSR_INDEX_MAX] = {

static unsigned int (and maybe also const, I didn't check whether it
gets written to).

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

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

> +static bool long_mode_active(struct x86_emulate_ctxt *ctxt)
> +{
> +    uint64_t val;
> +
> +    if ( fuzz_read_msr_(MSR_EFER, &val, ctxt) != X86EMUL_OKAY )
> +        return false;
> +
> +    return !!(val & EFER_LMA);

No need for !! here.

> +enum {
> +    HOOK_read,
> +    HOOK_insn_fetch,
> +    HOOK_write,
> +    HOOK_cmpxchg,
> +    HOOK_rep_ins,
> +    HOOK_rep_outs,
> +    HOOK_rep_movs,
> +    HOOK_rep_stos,
> +    HOOK_read_segment,
> +    HOOK_write_segment,
> +    HOOK_read_io,
> +    HOOK_write_io,
> +    HOOK_read_cr,
> +    HOOK_write_cr,
> +    HOOK_read_dr,
> +    HOOK_write_dr,
> +    HOOK_read_msr,
> +    HOOK_write_msr,
> +    HOOK_wbinvd,
> +    HOOK_cpuid,
> +    HOOK_inject_hw_exception,
> +    HOOK_inject_sw_interrupt,
> +    HOOK_get_fpu,
> +    HOOK_put_fpu,
> +    HOOK_invlpg,
> +    HOOK_vmfunc, /* 26 */
> +    OPTION_swint_emulation = 27, /* Two bits */
> +    CANONICALIZE_rip = 29,


    HOOK_vmfunc,
    OPTION_swint_emulation, /* Two bits */
    CANONICALIZE_rip = OPTION_swint_emulation + 2,

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

> +static void set_swint_support(struct x86_emulate_ctxt *ctxt)
> +{
> +    int swint_opt = (input.options >> OPTION_swint_emulation) & 3;

unsigned int

> +    int map[4] = {

static const enum x86_swint_emulation

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

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

Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.