Re: [Xen-devel] [PATCH v3 09/11] fuzz/x86emul: update fuzzer

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Feb 02, 2017 at 05:20:56AM -0700, Jan Beulich wrote:
> >>> 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?

Done.

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

Done.

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

Both can generate exceptions in some cases according to manual, so I
think it makes sense to leave them as-is, provided they fit in the model
you want to use (like in_realmode).

Of course if you end up changing the hooks to return void, I'm fine with
just removing these two hooks from fuzzer altogether.

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

Fixed for both read_segment and write_segment.

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

Right, I will add the following:

        /*                                                                      
                         
         * TSC should return monotonically increasing values, but               
                         
         * returning random values is fine in fuzzer.                           
                         
         */

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

Fixed. And then merge _fuzz_read_msr and fuzz_read_msr.

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

Fixed.

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

Done.

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

No problem, I add back the original comment.

Wei.

> Jan

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