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

Re: [Xen-devel] [PATCH v2 10/12] fuzz/x86emul: update fuzzer



On Tue, Jan 31, 2017 at 06:33:11AM -0700, Jan Beulich wrote:
> >>> On 31.01.17 at 12:08, <wei.liu2@xxxxxxxxxx> wrote:
> > @@ -16,26 +17,79 @@
> >  
> >  #include "x86_emulate.h"
> >  
> > -static unsigned char data[4096];
> > +#include "../../../xen/include/asm-x86/msr-index.h"
> > +
> > +#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.
> > + */
> 
> I'm not sure what coding style is to apply here, but if I consider this
> an extension to the insn emulator harness, which in turn is covered
> by hypervisor style, then the comment here needs /* to go on a
> separate line.
> 

Fixed.

> > +int maybe_fail(const char *why, bool exception)
> > +{
> > +    int rc;
> > +
> > +    if ( data_index + 1 > data_num )
> 
> "data_index >= data_num" would be the more conventional way
> to express this.
> 

Fixed.

> > +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;
> 
> I don't understand this: In the success case, *reps upon return
> indicates the number of iterations done, not the number of
> iterations left. So at least the variable naming deserves
> improvement.
> 

OK, so this should be *reps = bytes_read.


> > +    /* Indicate we haven't done any work if emulation has failed */
> > +    if ( rc != X86EMUL_OKAY )
> > +        *reps = 0;
> 
> That'll result in some code paths never taken. I'd suggest clearing
> to zero only in the UNHANDLEABLE case, and simply halving the
> amount for EXCEPTION or RETRY (granted the latter can't occur
> right now), or even using input data too for determining the new
> value.
> 

No problem. This function is now like:

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:
        *reps = 0;
        break;
    case X86EMUL_EXCEPTION:
    case X86EMUL_RETRY:
        *reps /= 2;
    }
                                                                                
                         
    return rc;
}

And I will do the same to rep_write.

> > +static int fuzz_rep_ins(
> > +    uint16_t src_port,
> > +    enum x86_segment dst_seg,
> > +    unsigned long dst_offset,
> > +    unsigned int bytes_per_rep,
> > +    unsigned long *reps,
> > +    struct x86_emulate_ctxt *ctxt)
> > +{
> > +    return _fuzz_rep_read("rep_in", reps);
> 
> "rep_ins"
> 

Fixed.

> > +static int fuzz_cpuid(
> > +    uint32_t leaf,
> > +    uint32_t subleaf,
> > +    struct cpuid_leaf *res,
> > +    struct x86_emulate_ctxt *ctxt)
> > +{
> > +    return emul_test_cpuid(leaf, subleaf, res, ctxt);
> > +}
> 
> I don't think you need this wrapper anymore. For the purpose of
> SET() below a simple #define will do.
> 

Fixed.

> > +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;
> 
> As said before - this wants to be >= (even more so with
> input.segments only having SEG_NUM elements).
> 

Done, and fixed write_segment too.

> > +    rc = maybe_fail("read_segment", true);
> > +
> > +    if ( rc == X86EMUL_OKAY )
> > +        memcpy(reg, input.segments+seg, sizeof(struct segment_register));
> 
> Please prefer (type safe) structure assignment.
> 
> > +static int fuzz_read_cr(
> > +    unsigned int reg,
> > +    unsigned long *val,
> > +    struct x86_emulate_ctxt *ctxt)
> > +{
> > +    int rc;
> > +
> > +    if ( reg > ARRAY_SIZE(input.cr) )
> > +        return X86EMUL_UNHANDLEABLE;
> 
> >= again.
> 

Done, and fixed write_cr.

> > +enum {
> > +    MSRI_IA32_SYSENTER_CS,
> > +    MSRI_IA32_SYSENTER_ESP,
> > +    MSRI_IA32_SYSENTER_EIP,
> > +    MSRI_EFER,
> > +    MSRI_STAR,
> > +    MSRI_LSTAR,
> > +    MSRI_CSTAR,
> > +    MSRI_SYSCALL_MASK
> > +};
> > +
> > +const static unsigned int msr_index[MSR_INDEX_MAX] = {
> 
> Conventionally static comes first (as not being part of the type).
> 

Done.

> > +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;
> > +    else
> 
> No need for "else".
> 

Done.

> > +static void setup_fpu_exception_handler(void)
> > +{
> > +    /* FIXME - just disable exceptions for now */
> > +    unsigned long a;
> > +
> > +    asm volatile ( "fnclex");
> > +    a=0x37f;                    /* FCW_DEFAULT in Xen */
> > +    asm volatile ( "fldcw %0" :: "m" (a));
> > +    a=0x1f80;                   /* MXCSR_DEFAULT in Xen */
> > +    asm volatile ( "ldmxcsr %0" :: "m" (a) );
> > +}
> 
> While I see that the FCW value has changed, the strange local
> variable is still there. If you really want to keep it, please at least
> add the missing spaces around the = signs. But I'd prefer
> 
>     asm volatile ( "fldcw %0" :: "m" (0x37f /* FCW_DEFAULT in Xen */));
>     asm volatile ( "ldmxcsr %0" :: "m" (0x1f80 /* MXCSR_DEFAULT in Xen */) );
> 

This doesn't work.

x86-insn-emulator-fuzzer.c:445:5: error: memory input 0 is not directly 
addressable

That's why I kept the local variable. But I will add spaces around =.

> And then - doesn't the ABI require these settings to be in effect
> upon program startup anyway?
> 

I'm not sure about this -- reading AMD64 ABI Draft 0.99.8 doesn't reveal
much for me. But having the code arranged like this hasn't caused any
SIGFPE so far.

What do you suggest I do here?

> > +/*
> > + * 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 asked for before - please drop the mentioning of real mode here.

Done.

> 
> > +void sanitize_input(struct x86_emulate_ctxt *ctxt) {
> > +    struct cpu_user_regs *regs = &input.regs;
> > +    unsigned long bitmap = input.options;
> > +
> > +    /* Some hooks can't be disabled. */
> > +    input.options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
> > +
> > +    /* Zero 'private' entries */
> > +    regs->error_code = 0;
> > +    regs->entry_vector = 0;
> > +
> > +    CANONICALIZE_MAYBE(rip);
> > +    CANONICALIZE_MAYBE(rsp);
> > +    CANONICALIZE_MAYBE(rbp);
> > +
> > +    /*
> > +     * CR0.PG can't be set if CR0.PE isn't set.  Set is more interesting, 
> > so
> > +     * set PE if PG is set.
> > +     */
> > +    if ( input.cr[0] & X86_CR0_PG )
> > +        input.cr[0] |= X86_CR0_PE;
> > +
> > +    /*
> > +     * EFLAGS.VM not available in long mode
> > +     */
> > +    if ( long_mode_active(ctxt) )
> > +        regs->rflags &= ~X86_EFLAGS_VM;
> > +
> > +    /*
> > +     * EFLAGS.VM implies 16-bit mode
> > +     */
> 
> Both of these are single line comments.

Done.

> 
> >      do {
> > +        /* FIXME: Until we actually implement SIGFPE handling properly */
> > +        setup_fpu_exception_handler();
> 
> I don't think the comment is appropriate here - all that needs fixing is
> the body of that function (which iirc already has a respective comment).
> 

Deleted this comment.

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