[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 09/11] fuzz/x86emul: update fuzzer
>>> On 01.02.17 at 13:02, <wei.liu2@xxxxxxxxxx> wrote: > @@ -16,26 +17,78 @@ > > #include "x86_emulate.h" > > -static unsigned char data[4096]; > +#define MSR_INDEX_MAX 16 > + > +#define SEG_NUM x86_seg_none > + > +struct input_struct { > + unsigned long cr[5]; > + uint64_t msr[MSR_INDEX_MAX]; > + struct cpu_user_regs regs; > + struct segment_register segments[SEG_NUM]; > + unsigned long options; > + unsigned char data[4096]; > +} input; > +#define DATA_OFFSET offsetof(struct input_struct, data) > static unsigned int data_index; > -static unsigned int data_max; > +static unsigned int data_num; > + > +/* > + * Randomly return success or failure when processing data. If > + * `exception` is false, this function turns _EXCEPTION to _OKAY. > + */ > +int maybe_fail(const char *why, bool exception) static? > +static int _fuzz_rep_read(const char *why, unsigned long *reps) > +{ > + int rc; > + unsigned long bytes_read = 0; > + > + rc = data_read(why, &bytes_read, sizeof(bytes_read)); > + > + if ( bytes_read <= *reps ) > + *reps = bytes_read; > + > + switch ( rc ) > + { > + case X86EMUL_UNHANDLEABLE: > + /* No work is done in this case */ > + *reps = 0; > + break; > + case X86EMUL_EXCEPTION: > + case X86EMUL_RETRY: > + /* Halve the amount in this case */ > + *reps /= 2; > + } Even if not strictly needed at this point in time, adding a break statement here would be nice. > +static int fuzz_invlpg( > + enum x86_segment seg, > + unsigned long offset, > + struct x86_emulate_ctxt *ctxt) > +{ > + return maybe_fail("invlpg", false); > +} > + > +static int fuzz_wbinvd( > + struct x86_emulate_ctxt *ctxt) > +{ > + return maybe_fail("wbinvd", true); > +} Can these two reasonably fail? I wonder if we shouldn't perhaps make the hooks return void. > +static int fuzz_read_segment( > + enum x86_segment seg, > + struct segment_register *reg, > + struct x86_emulate_ctxt *ctxt) > +{ > + int rc; > + > + if ( seg >= SEG_NUM ) > + return X86EMUL_UNHANDLEABLE; > + > + rc = maybe_fail("read_segment", true); > + > + if ( rc == X86EMUL_OKAY ) > + *reg = input.segments[seg]; > + > + return rc; > +} Just like with ->read_cr(), this must not vary in returned state between multiple invocations. > +static int _fuzz_read_msr( > + unsigned int reg, > + uint64_t *val, > + struct x86_emulate_ctxt *ctxt) > +{ > + unsigned int idx; > + > + switch ( reg ) > + { > + case MSR_TSC_AUX: > + case MSR_IA32_TSC: > + return data_read("read_msr", val, sizeof(*val)); Strictly speaking the above applies to TSC_AUX too. And TSC should return monotonically increasing values. I don't think though that producing random output here matters right now. A comment may be worthwhile. > +static int fuzz_read_msr( > + unsigned int reg, > + uint64_t *val, > + struct x86_emulate_ctxt *ctxt) { > + int rc; > + > + rc = maybe_fail("read_msr", true); > + if ( rc != X86EMUL_OKAY ) > + return rc; This, otoh, again needs to strictly follow the revised read_cr() model. > +static void set_swint_support(struct x86_emulate_ctxt *ctxt) > +{ > + unsigned int swint_opt = (input.options >> OPTION_swint_emulation) & 3; > + > + static const enum x86_swint_emulation map[4] = { As (I think) said before - please don't put blank lines between declarations; they're supposed to separate declarations from statements. > +/* > + * 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, force segment... > + * - ...access rights to 0xf3 > + * - ...limits to 0xffff > + * - ...bases to below 1Mb, 16-byte aligned > + * - ...selectors to (base >> 4) > + */ > +void sanitize_input(struct x86_emulate_ctxt *ctxt) { static? And { on its own line. > do { > + setup_fpu_exception_handler(); I'm sorry for having mislead you regarding the comment which was here: As said elsewhere, I was - based on the function's name - assuming this to be a one time action, while in fact this is being done before every emulated insn. Once this becomes a one time thing, it indeed needs moving out of the loop, so leaving a comment here is warranted. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |