Re: [PATCH v4 15/20] fuzz: add fuzzer skeleton

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5553 bytes --]

On Wed, Oct 30, 2019 at 02:50:00PM +0000, Oleinik, Alexander wrote:
> diff --git a/tests/fuzz/fuzz.c b/tests/fuzz/fuzz.c
> new file mode 100644
> index 0000000000..0e38f81c48
> --- /dev/null
> +++ b/tests/fuzz/fuzz.c
> @@ -0,0 +1,177 @@
> +/*
> + * fuzzing driver
> + *
> + * Copyright Red Hat Inc., 2019
> + *
> + * Authors:
> + *  Alexander Bulekov   <alxndr@bu.edu>

Bulekov instead of Oleinik?

> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include <stdio.h>
> +#include <stdlib.h>

stdio.h and stdlib.h are already included by qemu/osdep.h.

> +/* Executed for each fuzzing-input */
> +int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size)
> +{
> +    if (fuzz_target->fuzz) {

Will this ever be NULL?

> +        fuzz_target->fuzz(fuzz_qts, Data, Size);
> +    }
> +    return 0;
> +}
> +
> +/* Executed once, prior to fuzzing */
> +int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
> +{
> +
> +    char *target_name;
> +
> +    /* Initialize qgraph and modules */
> +    qos_graph_init();
> +    module_call_init(MODULE_INIT_FUZZ_TARGET);
> +    module_call_init(MODULE_INIT_QOM);
> +    module_call_init(MODULE_INIT_LIBQOS);
> +
> +    if (*argc <= 1) {
> +        usage(**argv);
> +    }
> +
> +    /* Identify the fuzz target */
> +    target_name = (*argv)[1];
> +    if (!strstr(target_name, "--fuzz-target=")) {
> +        usage(**argv);
> +    }
> +
> +    target_name += strlen("--fuzz-target=");
> +
> +    fuzz_target = fuzz_get_target(target_name);
> +    if (!fuzz_target) {
> +        usage(**argv);
> +    }
> +
> +    fuzz_qts = qtest_setup();
> +
> +    if (!fuzz_target) {

This is dead code since fuzz_target was already checked above.  Please
remove this if statement.

> +        fprintf(stderr, "Error: Fuzz fuzz_target name %s not found\n",
> +                target_name);
> +        usage(**argv);
> +    }
> +
> +    if (fuzz_target->pre_vm_init) {
> +        fuzz_target->pre_vm_init();
> +    }
> +
> +    /* Run QEMU's softmmu main with the fuzz-target dependent arguments */
> +    char *init_cmdline = fuzz_target->get_init_cmdline(fuzz_target);

Where is init_cmdline freed or should this be const char *?

> +    wordexp_t result;
> +    wordexp(init_cmdline, &result, 0);

What is the purpose of word expansion here?

> +
> +    qemu_init(result.we_wordc, result.we_wordv, NULL);
> +
> +    if (fuzz_target->pre_fuzz) {
> +        fuzz_target->pre_fuzz(fuzz_qts);
> +    }
> +
> +    return 0;
> +}
> diff --git a/tests/fuzz/fuzz.h b/tests/fuzz/fuzz.h
> new file mode 100644
> index 0000000000..b569b622d7
> --- /dev/null
> +++ b/tests/fuzz/fuzz.h
> @@ -0,0 +1,66 @@
> +/*
> + * fuzzing driver
> + *
> + * Copyright Red Hat Inc., 2019
> + *
> + * Authors:
> + *  Alexander Bulekov   <alxndr@bu.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef FUZZER_H_
> +#define FUZZER_H_
> +
> +#include "qemu/osdep.h"
> +#include "qemu/units.h"
> +#include "qapi/error.h"
> +#include "exec/memory.h"
> +#include "tests/libqtest.h"
> +
> +

Some documentation would be nice:

/**
 * A libfuzzer fuzzing target
 *
 * The QEMU fuzzing binary is built with all available targets, each
 * with a unique @name that can be specified on the command-line to
 * select which target should run.
 *
 * A target must implement ->fuzz() to process a random input.  If QEMU
 * crashes in ->fuzz() then libfuzzer will record a failure.
 *
 * Fuzzing targets are registered with fuzz_add_target():
 *
 *   static const FuzzTarget fuzz_target = {
 *       .name = "my-device-fifo",
 *       .description = "Fuzz the FIFO buffer registers of my-device",
 *       ...
 *   };
 *
 *   static void register_fuzz_target(void)
 *   {
 *       fuzz_add_target(&fuzz_target);
 *   }
 *   fuzz_target_init(register_fuzz_target);
 */

> +typedef struct FuzzTarget {
> +    const char *name;         /* command-line option(FUZZ_TARGET) for the target */
> +    const char *description;  /* help text */
> +

If any of the function pointers can be NULL, please document this.

> +    /* returns the arg-list that is passed to qemu/softmmu init() */
> +    char* (*get_init_cmdline)(struct FuzzTarget *);

Does the caller need to call g_free() on the returned string?  Please
document this.

> +
> +    /*
> +     * will run once, prior to running qemu/softmmu init.
> +     * eg: set up shared-memory for communication with the child-process
> +     */
> +    void(*pre_vm_init)(void);
> +
> +    /*
> +     * will run once, prior to to the fuzz-loop.

s/to to/to/

> +     * eg: detect the memory map
> +     */
> +    void(*pre_fuzz)(QTestState *);

Please also mention that QEMU has been initialized at this point.

> +
> +    /*
> +     * accepts and executes an input from libfuzzer. this is repeatedly
> +     * executed during the fuzzing loop. Its should handle setup, input
> +     * execution and cleanup
> +     */
> +    void(*fuzz)(QTestState *, const unsigned char *, size_t);
> +
> +} FuzzTarget;
> +
> +void flush_events(QTestState *);
> +void reboot(QTestState *);
> +
> +/*
> + * makes a copy of *target and adds it to the target-list.
> + * i.e. fine to set up target on the caller's stack
> + */
> +void fuzz_add_target(FuzzTarget *target);

"makes a copy of *target" -> does this mean the argument type can be
const FuzzTarget *target?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]