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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

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

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

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

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

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

> +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 */) );

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

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

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

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

Jan

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