[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |