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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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