[PATCH v1 0/2] fuzz: add virtio-blk fuzz target

[PATCH v1 0/2] fuzz: add virtio-blk fuzz target

[Dima Stepanov]

Based on the current virtio-scsi-fuzz target implement new virtio-blk
fuzz target. Use a virtio_blk_test.c file as a reference for the block
device initialization.

Also make a small update to the documentation to fix build/run command
lines after meson and build changes in QEMU.

Dima Stepanov (2):
  fuzz: add virtio-blk fuzz target
  docs/fuzz: update make and run command lines

 docs/devel/fuzzing.txt             |   6 +-
 tests/qtest/fuzz/meson.build       |   1 +
 tests/qtest/fuzz/virtio_blk_fuzz.c | 234 +++++++++++++++++++++++++++++++++++++
 3 files changed, 238 insertions(+), 3 deletions(-)
 create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c

-- 
2.7.4

[PATCH v1 1/2] fuzz: add virtio-blk fuzz target

[Dima Stepanov]

The virtio-blk fuzz target sets up and fuzzes the available virtio-blk
queues. The implementation is based on two files:
  - tests/qtest/fuzz/virtio_scsi_fuzz.c
  - tests/qtest/virtio_blk_test.c

Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
---
 tests/qtest/fuzz/meson.build       |   1 +
 tests/qtest/fuzz/virtio_blk_fuzz.c | 234 +++++++++++++++++++++++++++++++++++++
 2 files changed, 235 insertions(+)
 create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c

diff --git a/tests/qtest/fuzz/meson.build b/tests/qtest/fuzz/meson.build
index b31ace7..3b923dc 100644
--- a/tests/qtest/fuzz/meson.build
+++ b/tests/qtest/fuzz/meson.build
@@ -5,6 +5,7 @@ specific_fuzz_ss.add(files('fuzz.c', 'fork_fuzz.c', 'qos_fuzz.c',
 specific_fuzz_ss.add(when: 'CONFIG_I440FX', if_true: files('i440fx_fuzz.c'))
 specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio_net_fuzz.c'))
 specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: files('virtio_scsi_fuzz.c'))
+specific_fuzz_ss.add(files('virtio_blk_fuzz.c'))
 
 fork_fuzz = declare_dependency(
   link_args: config_host['FUZZ_EXE_LDFLAGS'].split() +
diff --git a/tests/qtest/fuzz/virtio_blk_fuzz.c b/tests/qtest/fuzz/virtio_blk_fuzz.c
new file mode 100644
index 0000000..623a756
--- /dev/null
+++ b/tests/qtest/fuzz/virtio_blk_fuzz.c
@@ -0,0 +1,234 @@
+/*
+ * virtio-blk Fuzzing Target
+ *
+ * Copyright Red Hat Inc., 2020
+ *
+ * Based on virtio-scsi-fuzz target.
+ *
+ * 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 "tests/qtest/libqos/libqtest.h"
+#include "tests/qtest/libqos/virtio-blk.h"
+#include "tests/qtest/libqos/virtio.h"
+#include "tests/qtest/libqos/virtio-pci.h"
+#include "standard-headers/linux/virtio_ids.h"
+#include "standard-headers/linux/virtio_pci.h"
+#include "standard-headers/linux/virtio_blk.h"
+#include "fuzz.h"
+#include "fork_fuzz.h"
+#include "qos_fuzz.h"
+
+#define TEST_IMAGE_SIZE         (64 * 1024 * 1024)
+#define PCI_SLOT                0x02
+#define PCI_FN                  0x00
+
+#define MAX_NUM_QUEUES 64
+
+/* Based on tests/qtest/virtio-blk-test.c. */
+typedef struct {
+    int num_queues;
+    QVirtQueue *vq[MAX_NUM_QUEUES + 2];
+} QVirtioBlkQueues;
+
+static QVirtioBlkQueues *qvirtio_blk_init(QVirtioDevice *dev, uint64_t mask)
+{
+    QVirtioBlkQueues *vs;
+    uint64_t features;
+
+    vs = g_new0(QVirtioBlkQueues, 1);
+
+    features = qvirtio_get_features(dev);
+    if (!mask) {
+        mask = ~((1u << VIRTIO_RING_F_INDIRECT_DESC) |
+                (1u << VIRTIO_RING_F_EVENT_IDX) |
+                (1u << VIRTIO_BLK_F_SCSI));
+    }
+    mask |= ~QVIRTIO_F_BAD_FEATURE;
+    features &= mask;
+    qvirtio_set_features(dev, features);
+
+    vs->num_queues = 1;
+    vs->vq[0] = qvirtqueue_setup(dev, fuzz_qos_alloc, 0);
+
+    qvirtio_set_driver_ok(dev);
+
+    return vs;
+}
+
+static void virtio_blk_fuzz(QTestState *s, QVirtioBlkQueues* queues,
+        const unsigned char *Data, size_t Size)
+{
+    /*
+     * Data is a sequence of random bytes. We split them up into "actions",
+     * followed by data:
+     * [vqa][dddddddd][vqa][dddd][vqa][dddddddddddd] ...
+     * The length of the data is specified by the preceding vqa.length
+     */
+    typedef struct vq_action {
+        uint8_t queue;
+        uint8_t length;
+        uint8_t write;
+        uint8_t next;
+        uint8_t kick;
+    } vq_action;
+
+    /* Keep track of the free head for each queue we interact with */
+    bool vq_touched[MAX_NUM_QUEUES + 2] = {0};
+    uint32_t free_head[MAX_NUM_QUEUES + 2];
+
+    QGuestAllocator *t_alloc = fuzz_qos_alloc;
+
+    QVirtioBlk *blk = fuzz_qos_obj;
+    QVirtioDevice *dev = blk->vdev;
+    QVirtQueue *q;
+    vq_action vqa;
+    while (Size >= sizeof(vqa)) {
+        /* Copy the action, so we can normalize length, queue and flags */
+        memcpy(&vqa, Data, sizeof(vqa));
+
+        Data += sizeof(vqa);
+        Size -= sizeof(vqa);
+
+        vqa.queue = vqa.queue % queues->num_queues;
+        /* Cap length at the number of remaining bytes in data */
+        vqa.length = vqa.length >= Size ? Size : vqa.length;
+        vqa.write = vqa.write & 1;
+        vqa.next = vqa.next & 1;
+        vqa.kick = vqa.kick & 1;
+
+        q = queues->vq[vqa.queue];
+
+        /* Copy the data into ram, and place it on the virtqueue */
+        uint64_t req_addr = guest_alloc(t_alloc, vqa.length);
+        qtest_memwrite(s, req_addr, Data, vqa.length);
+        if (vq_touched[vqa.queue] == 0) {
+            vq_touched[vqa.queue] = 1;
+            free_head[vqa.queue] = qvirtqueue_add(s, q, req_addr, vqa.length,
+                    vqa.write, vqa.next);
+        } else {
+            qvirtqueue_add(s, q, req_addr, vqa.length, vqa.write , vqa.next);
+        }
+
+        if (vqa.kick) {
+            qvirtqueue_kick(s, dev, q, free_head[vqa.queue]);
+            free_head[vqa.queue] = 0;
+        }
+        Data += vqa.length;
+        Size -= vqa.length;
+    }
+    /* In the end, kick each queue we interacted with */
+    for (int i = 0; i < MAX_NUM_QUEUES + 2; i++) {
+        if (vq_touched[i]) {
+            qvirtqueue_kick(s, dev, queues->vq[i], free_head[i]);
+        }
+    }
+}
+
+static void virtio_blk_fork_fuzz(QTestState *s,
+        const unsigned char *Data, size_t Size)
+{
+    QVirtioBlk *blk = fuzz_qos_obj;
+    static QVirtioBlkQueues *queues;
+    if (!queues) {
+        queues = qvirtio_blk_init(blk->vdev, 0);
+    }
+    if (fork() == 0) {
+        virtio_blk_fuzz(s, queues, Data, Size);
+        flush_events(s);
+        _Exit(0);
+    } else {
+        flush_events(s);
+        wait(NULL);
+    }
+}
+
+static void virtio_blk_with_flag_fuzz(QTestState *s,
+        const unsigned char *Data, size_t Size)
+{
+    QVirtioBlk *blk = fuzz_qos_obj;
+    static QVirtioBlkQueues *queues;
+
+    if (fork() == 0) {
+        if (Size >= sizeof(uint64_t)) {
+            queues = qvirtio_blk_init(blk->vdev, *(uint64_t *)Data);
+            virtio_blk_fuzz(s, queues,
+                             Data + sizeof(uint64_t), Size - sizeof(uint64_t));
+            flush_events(s);
+        }
+        _Exit(0);
+    } else {
+        flush_events(s);
+        wait(NULL);
+    }
+}
+
+static void virtio_blk_pre_fuzz(QTestState *s)
+{
+    qos_init_path(s);
+    counter_shm_init();
+}
+
+static void drive_destroy(void *path)
+{
+    unlink(path);
+    g_free(path);
+}
+
+static char *drive_create(void)
+{
+    int fd, ret;
+    char *t_path = g_strdup("/tmp/qtest.XXXXXX");
+
+    /* Create a temporary raw image */
+    fd = mkstemp(t_path);
+    g_assert_cmpint(fd, >=, 0);
+    ret = ftruncate(fd, TEST_IMAGE_SIZE);
+    g_assert_cmpint(ret, ==, 0);
+    close(fd);
+
+    g_test_queue_destroy(drive_destroy, t_path);
+    return t_path;
+}
+
+static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
+{
+    char *tmp_path = drive_create();
+
+    g_string_append_printf(cmd_line,
+                           " -drive if=none,id=drive0,file=%s,"
+                           "format=raw,auto-read-only=off ",
+                           tmp_path);
+
+    return arg;
+}
+
+static void register_virtio_blk_fuzz_targets(void)
+{
+    fuzz_add_qos_target(&(FuzzTarget){
+                .name = "virtio-blk-fuzz",
+                .description = "Fuzz the virtio-blk virtual queues, forking "
+                                "for each fuzz run",
+                .pre_vm_init = &counter_shm_init,
+                .pre_fuzz = &virtio_blk_pre_fuzz,
+                .fuzz = virtio_blk_fork_fuzz,},
+                "virtio-blk",
+                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
+                );
+
+    fuzz_add_qos_target(&(FuzzTarget){
+                .name = "virtio-blk-flags-fuzz",
+                .description = "Fuzz the virtio-blk virtual queues, forking "
+                "for each fuzz run (also fuzzes the virtio flags)",
+                .pre_vm_init = &counter_shm_init,
+                .pre_fuzz = &virtio_blk_pre_fuzz,
+                .fuzz = virtio_blk_with_flag_fuzz,},
+                "virtio-blk",
+                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
+                );
+}
+
+fuzz_target_init(register_virtio_blk_fuzz_targets);
-- 
2.7.4

Re: [PATCH v1 1/2] fuzz: add virtio-blk fuzz target

[Alexander Bulekov]

On 201007 1647, Dima Stepanov wrote:
> The virtio-blk fuzz target sets up and fuzzes the available virtio-blk
> queues. The implementation is based on two files:
>   - tests/qtest/fuzz/virtio_scsi_fuzz.c
>   - tests/qtest/virtio_blk_test.c
> 
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  tests/qtest/fuzz/meson.build       |   1 +
>  tests/qtest/fuzz/virtio_blk_fuzz.c | 234 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 235 insertions(+)
>  create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c
> 
> diff --git a/tests/qtest/fuzz/meson.build b/tests/qtest/fuzz/meson.build
> index b31ace7..3b923dc 100644
> --- a/tests/qtest/fuzz/meson.build
> +++ b/tests/qtest/fuzz/meson.build
> @@ -5,6 +5,7 @@ specific_fuzz_ss.add(files('fuzz.c', 'fork_fuzz.c', 'qos_fuzz.c',
>  specific_fuzz_ss.add(when: 'CONFIG_I440FX', if_true: files('i440fx_fuzz.c'))
>  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio_net_fuzz.c'))
>  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: files('virtio_scsi_fuzz.c'))
> +specific_fuzz_ss.add(files('virtio_blk_fuzz.c'))

Hi Dima,
For consistency, maybe
specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio_blk_fuzz.c'))

>  
>  fork_fuzz = declare_dependency(
>    link_args: config_host['FUZZ_EXE_LDFLAGS'].split() +
> diff --git a/tests/qtest/fuzz/virtio_blk_fuzz.c b/tests/qtest/fuzz/virtio_blk_fuzz.c
> new file mode 100644
> index 0000000..623a756
> --- /dev/null
> +++ b/tests/qtest/fuzz/virtio_blk_fuzz.c
> @@ -0,0 +1,234 @@
> +/*
> + * virtio-blk Fuzzing Target
> + *
> + * Copyright Red Hat Inc., 2020
> + *
> + * Based on virtio-scsi-fuzz target.
> + *
> + * 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 "tests/qtest/libqos/libqtest.h"
> +#include "tests/qtest/libqos/virtio-blk.h"
> +#include "tests/qtest/libqos/virtio.h"
> +#include "tests/qtest/libqos/virtio-pci.h"
> +#include "standard-headers/linux/virtio_ids.h"
> +#include "standard-headers/linux/virtio_pci.h"
> +#include "standard-headers/linux/virtio_blk.h"
> +#include "fuzz.h"
> +#include "fork_fuzz.h"
> +#include "qos_fuzz.h"
> +
> +#define TEST_IMAGE_SIZE         (64 * 1024 * 1024)
> +#define PCI_SLOT                0x02
> +#define PCI_FN                  0x00
> +
> +#define MAX_NUM_QUEUES 64
> +
> +/* Based on tests/qtest/virtio-blk-test.c. */
> +typedef struct {
> +    int num_queues;
> +    QVirtQueue *vq[MAX_NUM_QUEUES + 2];
> +} QVirtioBlkQueues;
> +
> +static QVirtioBlkQueues *qvirtio_blk_init(QVirtioDevice *dev, uint64_t mask)
> +{
> +    QVirtioBlkQueues *vs;
> +    uint64_t features;
> +
> +    vs = g_new0(QVirtioBlkQueues, 1);
> +
> +    features = qvirtio_get_features(dev);
> +    if (!mask) {
> +        mask = ~((1u << VIRTIO_RING_F_INDIRECT_DESC) |
> +                (1u << VIRTIO_RING_F_EVENT_IDX) |
> +                (1u << VIRTIO_BLK_F_SCSI));
> +    }
> +    mask |= ~QVIRTIO_F_BAD_FEATURE;
> +    features &= mask;
> +    qvirtio_set_features(dev, features);
> +
> +    vs->num_queues = 1;
> +    vs->vq[0] = qvirtqueue_setup(dev, fuzz_qos_alloc, 0);
> +
> +    qvirtio_set_driver_ok(dev);
> +
> +    return vs;
> +}
> +
> +static void virtio_blk_fuzz(QTestState *s, QVirtioBlkQueues* queues,
> +        const unsigned char *Data, size_t Size)
> +{
> +    /*
> +     * Data is a sequence of random bytes. We split them up into "actions",
> +     * followed by data:
> +     * [vqa][dddddddd][vqa][dddd][vqa][dddddddddddd] ...
> +     * The length of the data is specified by the preceding vqa.length
> +     */
> +    typedef struct vq_action {
> +        uint8_t queue;
> +        uint8_t length;
> +        uint8_t write;
> +        uint8_t next;
> +        uint8_t kick;
> +    } vq_action;
> +
> +    /* Keep track of the free head for each queue we interact with */
> +    bool vq_touched[MAX_NUM_QUEUES + 2] = {0};
> +    uint32_t free_head[MAX_NUM_QUEUES + 2];
> +
> +    QGuestAllocator *t_alloc = fuzz_qos_alloc;
> +
> +    QVirtioBlk *blk = fuzz_qos_obj;
> +    QVirtioDevice *dev = blk->vdev;
> +    QVirtQueue *q;
> +    vq_action vqa;
> +    while (Size >= sizeof(vqa)) {
> +        /* Copy the action, so we can normalize length, queue and flags */
> +        memcpy(&vqa, Data, sizeof(vqa));
> +
> +        Data += sizeof(vqa);
> +        Size -= sizeof(vqa);
> +
> +        vqa.queue = vqa.queue % queues->num_queues;
> +        /* Cap length at the number of remaining bytes in data */
> +        vqa.length = vqa.length >= Size ? Size : vqa.length;
> +        vqa.write = vqa.write & 1;
> +        vqa.next = vqa.next & 1;
> +        vqa.kick = vqa.kick & 1;
> +
> +        q = queues->vq[vqa.queue];
> +
> +        /* Copy the data into ram, and place it on the virtqueue */
> +        uint64_t req_addr = guest_alloc(t_alloc, vqa.length);
> +        qtest_memwrite(s, req_addr, Data, vqa.length);
> +        if (vq_touched[vqa.queue] == 0) {
> +            vq_touched[vqa.queue] = 1;
> +            free_head[vqa.queue] = qvirtqueue_add(s, q, req_addr, vqa.length,
> +                    vqa.write, vqa.next);
> +        } else {
> +            qvirtqueue_add(s, q, req_addr, vqa.length, vqa.write , vqa.next);
> +        }
> +
> +        if (vqa.kick) {
> +            qvirtqueue_kick(s, dev, q, free_head[vqa.queue]);
> +            free_head[vqa.queue] = 0;
> +        }
> +        Data += vqa.length;
> +        Size -= vqa.length;
> +    }
> +    /* In the end, kick each queue we interacted with */
> +    for (int i = 0; i < MAX_NUM_QUEUES + 2; i++) {
> +        if (vq_touched[i]) {
> +            qvirtqueue_kick(s, dev, queues->vq[i], free_head[i]);
> +        }
> +    }
> +}
> +
> +static void virtio_blk_fork_fuzz(QTestState *s,
> +        const unsigned char *Data, size_t Size)
> +{
> +    QVirtioBlk *blk = fuzz_qos_obj;
> +    static QVirtioBlkQueues *queues;
> +    if (!queues) {
> +        queues = qvirtio_blk_init(blk->vdev, 0);
> +    }
> +    if (fork() == 0) {
> +        virtio_blk_fuzz(s, queues, Data, Size);
> +        flush_events(s);
> +        _Exit(0);
> +    } else {
> +        flush_events(s);
> +        wait(NULL);
> +    }
> +}
> +
> +static void virtio_blk_with_flag_fuzz(QTestState *s,
> +        const unsigned char *Data, size_t Size)
> +{
> +    QVirtioBlk *blk = fuzz_qos_obj;
> +    static QVirtioBlkQueues *queues;
> +
> +    if (fork() == 0) {
> +        if (Size >= sizeof(uint64_t)) {
> +            queues = qvirtio_blk_init(blk->vdev, *(uint64_t *)Data);
> +            virtio_blk_fuzz(s, queues,
> +                             Data + sizeof(uint64_t), Size - sizeof(uint64_t));
> +            flush_events(s);
> +        }
> +        _Exit(0);
> +    } else {
> +        flush_events(s);
> +        wait(NULL);
> +    }
> +}
> +
> +static void virtio_blk_pre_fuzz(QTestState *s)
> +{
> +    qos_init_path(s);
> +    counter_shm_init();
> +}
> +
> +static void drive_destroy(void *path)
> +{
> +    unlink(path);
> +    g_free(path);
> +}
> +
> +static char *drive_create(void)
> +{
> +    int fd, ret;
> +    char *t_path = g_strdup("/tmp/qtest.XXXXXX");
> +
> +    /* Create a temporary raw image */
> +    fd = mkstemp(t_path);
> +    g_assert_cmpint(fd, >=, 0);
> +    ret = ftruncate(fd, TEST_IMAGE_SIZE);
> +    g_assert_cmpint(ret, ==, 0);
> +    close(fd);
> +
> +    g_test_queue_destroy(drive_destroy, t_path);
> +    return t_path;
> +}
> +

I tested this out and it works with multi-process fuzzing under -jobs=4
-workers=4 (this initialization happens after libfuzzer has already
forked the processes). This seems like an interesting alternative to
using fake null-co:// files. 
I wonder if some state might leak as these disks are filled with fuzzer
data.

Nit: these disk files remain after the fuzzer exists. It looks
like the libfuzzer people suggest simply using atexit() to perform
cleanup: https://reviews.llvm.org/D45762
The is that the only way I have found to terminate the fuzzer is with
SIGKILL, where atexit is skipped. QEMU installs some signal handlers in
os-posix.c:os_setup_signal_handling to notify the main_loop that the
qemu was killed. Since we replace qemu_main_loop by manually running
main_loop_wait, we don't check main_loop_should_exit().

I sent a patch to disable QEMU's signal handlers for the fuzzer.
Message-Id: <20201013152920.448335-1-alxndr@bu.edu>

With an atexit() call to clean up the temporary images:
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

> +static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
> +{
> +    char *tmp_path = drive_create();
> +
> +    g_string_append_printf(cmd_line,
> +                           " -drive if=none,id=drive0,file=%s,"
> +                           "format=raw,auto-read-only=off ",
> +                           tmp_path);
> +
> +    return arg;
> +}
> +
> +static void register_virtio_blk_fuzz_targets(void)
> +{
> +    fuzz_add_qos_target(&(FuzzTarget){
> +                .name = "virtio-blk-fuzz",
> +                .description = "Fuzz the virtio-blk virtual queues, forking "
> +                                "for each fuzz run",
> +                .pre_vm_init = &counter_shm_init,
> +                .pre_fuzz = &virtio_blk_pre_fuzz,
> +                .fuzz = virtio_blk_fork_fuzz,},
> +                "virtio-blk",
> +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> +                );
> +
> +    fuzz_add_qos_target(&(FuzzTarget){
> +                .name = "virtio-blk-flags-fuzz",
> +                .description = "Fuzz the virtio-blk virtual queues, forking "
> +                "for each fuzz run (also fuzzes the virtio flags)",
> +                .pre_vm_init = &counter_shm_init,
> +                .pre_fuzz = &virtio_blk_pre_fuzz,
> +                .fuzz = virtio_blk_with_flag_fuzz,},
> +                "virtio-blk",
> +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> +                );
> +}
> +
> +fuzz_target_init(register_virtio_blk_fuzz_targets);
> -- 
> 2.7.4
> 

Re: [PATCH v1 1/2] fuzz: add virtio-blk fuzz target

[Dima Stepanov]

On Tue, Oct 13, 2020 at 11:30:52AM -0400, Alexander Bulekov wrote:
> On 201007 1647, Dima Stepanov wrote:
> > The virtio-blk fuzz target sets up and fuzzes the available virtio-blk
> > queues. The implementation is based on two files:
> >   - tests/qtest/fuzz/virtio_scsi_fuzz.c
> >   - tests/qtest/virtio_blk_test.c
> > 
> > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > ---
> >  tests/qtest/fuzz/meson.build       |   1 +
> >  tests/qtest/fuzz/virtio_blk_fuzz.c | 234 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 235 insertions(+)
> >  create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c
> > 
> > diff --git a/tests/qtest/fuzz/meson.build b/tests/qtest/fuzz/meson.build
> > index b31ace7..3b923dc 100644
> > --- a/tests/qtest/fuzz/meson.build
> > +++ b/tests/qtest/fuzz/meson.build
> > @@ -5,6 +5,7 @@ specific_fuzz_ss.add(files('fuzz.c', 'fork_fuzz.c', 'qos_fuzz.c',
> >  specific_fuzz_ss.add(when: 'CONFIG_I440FX', if_true: files('i440fx_fuzz.c'))
> >  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio_net_fuzz.c'))
> >  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: files('virtio_scsi_fuzz.c'))
> > +specific_fuzz_ss.add(files('virtio_blk_fuzz.c'))
> 
> Hi Dima,
> For consistency, maybe
> specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio_blk_fuzz.c'))
Good point, will update it.

> 
...
> > +
> > +static char *drive_create(void)
> > +{
> > +    int fd, ret;
> > +    char *t_path = g_strdup("/tmp/qtest.XXXXXX");
> > +
> > +    /* Create a temporary raw image */
> > +    fd = mkstemp(t_path);
> > +    g_assert_cmpint(fd, >=, 0);
> > +    ret = ftruncate(fd, TEST_IMAGE_SIZE);
> > +    g_assert_cmpint(ret, ==, 0);
> > +    close(fd);
> > +
> > +    g_test_queue_destroy(drive_destroy, t_path);
> > +    return t_path;
> > +}
> > +
> 
> I tested this out and it works with multi-process fuzzing under -jobs=4
> -workers=4 (this initialization happens after libfuzzer has already
> forked the processes). This seems like an interesting alternative to
> using fake null-co:// files. 
> I wonder if some state might leak as these disks are filled with fuzzer
> data.
Yes, i've also chosen between the fake null device and temporary file.
Tried this approach, just to see what will happen ). It seems to me that
slightly different paths can be triggered in this case and it is closer
to real usage.
But indeed, mb some state can leak, this is interesting.

> 
> Nit: these disk files remain after the fuzzer exists. It looks
> like the libfuzzer people suggest simply using atexit() to perform
> cleanup: https://reviews.llvm.org/D45762
> The is that the only way I have found to terminate the fuzzer is with
> SIGKILL, where atexit is skipped. QEMU installs some signal handlers in
> os-posix.c:os_setup_signal_handling to notify the main_loop that the
> qemu was killed. Since we replace qemu_main_loop by manually running
> main_loop_wait, we don't check main_loop_should_exit().
Got it! Thanks for sharing this is good to know ).

No other comments mixed in below.

Dima.
> 
> I sent a patch to disable QEMU's signal handlers for the fuzzer.
> Message-Id: <20201013152920.448335-1-alxndr@bu.edu>
> 
> With an atexit() call to clean up the temporary images:
> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> 
> > +static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
> > +{
> > +    char *tmp_path = drive_create();
> > +
> > +    g_string_append_printf(cmd_line,
> > +                           " -drive if=none,id=drive0,file=%s,"
> > +                           "format=raw,auto-read-only=off ",
> > +                           tmp_path);
> > +
> > +    return arg;
> > +}
> > +
> > +static void register_virtio_blk_fuzz_targets(void)
> > +{
> > +    fuzz_add_qos_target(&(FuzzTarget){
> > +                .name = "virtio-blk-fuzz",
> > +                .description = "Fuzz the virtio-blk virtual queues, forking "
> > +                                "for each fuzz run",
> > +                .pre_vm_init = &counter_shm_init,
> > +                .pre_fuzz = &virtio_blk_pre_fuzz,
> > +                .fuzz = virtio_blk_fork_fuzz,},
> > +                "virtio-blk",
> > +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> > +                );
> > +
> > +    fuzz_add_qos_target(&(FuzzTarget){
> > +                .name = "virtio-blk-flags-fuzz",
> > +                .description = "Fuzz the virtio-blk virtual queues, forking "
> > +                "for each fuzz run (also fuzzes the virtio flags)",
> > +                .pre_vm_init = &counter_shm_init,
> > +                .pre_fuzz = &virtio_blk_pre_fuzz,
> > +                .fuzz = virtio_blk_with_flag_fuzz,},
> > +                "virtio-blk",
> > +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> > +                );
> > +}
> > +
> > +fuzz_target_init(register_virtio_blk_fuzz_targets);
> > -- 
> > 2.7.4
> > 

Re: [PATCH v1 1/2] fuzz: add virtio-blk fuzz target

[Dima Stepanov]

On Wed, Oct 14, 2020 at 10:29:41AM +0300, Dima Stepanov wrote:
> On Tue, Oct 13, 2020 at 11:30:52AM -0400, Alexander Bulekov wrote:
> > On 201007 1647, Dima Stepanov wrote:
> > > The virtio-blk fuzz target sets up and fuzzes the available virtio-blk
> > > queues. The implementation is based on two files:
> > >   - tests/qtest/fuzz/virtio_scsi_fuzz.c
> > >   - tests/qtest/virtio_blk_test.c
> > > 
> > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > > ---
> > >  tests/qtest/fuzz/meson.build       |   1 +
> > >  tests/qtest/fuzz/virtio_blk_fuzz.c | 234 +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 235 insertions(+)
> > >  create mode 100644 tests/qtest/fuzz/virtio_blk_fuzz.c
> > > 
> > > diff --git a/tests/qtest/fuzz/meson.build b/tests/qtest/fuzz/meson.build
> > > index b31ace7..3b923dc 100644
> > > --- a/tests/qtest/fuzz/meson.build
> > > +++ b/tests/qtest/fuzz/meson.build
> > > @@ -5,6 +5,7 @@ specific_fuzz_ss.add(files('fuzz.c', 'fork_fuzz.c', 'qos_fuzz.c',
> > >  specific_fuzz_ss.add(when: 'CONFIG_I440FX', if_true: files('i440fx_fuzz.c'))
> > >  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_NET', if_true: files('virtio_net_fuzz.c'))
> > >  specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_SCSI', if_true: files('virtio_scsi_fuzz.c'))
> > > +specific_fuzz_ss.add(files('virtio_blk_fuzz.c'))
> > 
> > Hi Dima,
> > For consistency, maybe
> > specific_fuzz_ss.add(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio_blk_fuzz.c'))
> Good point, will update it.
> 
> > 
> ...
> > > +
> > > +static char *drive_create(void)
> > > +{
> > > +    int fd, ret;
> > > +    char *t_path = g_strdup("/tmp/qtest.XXXXXX");
> > > +
> > > +    /* Create a temporary raw image */
> > > +    fd = mkstemp(t_path);
> > > +    g_assert_cmpint(fd, >=, 0);
> > > +    ret = ftruncate(fd, TEST_IMAGE_SIZE);
> > > +    g_assert_cmpint(ret, ==, 0);
> > > +    close(fd);
> > > +
> > > +    g_test_queue_destroy(drive_destroy, t_path);
> > > +    return t_path;
> > > +}
> > > +
> > 
> > I tested this out and it works with multi-process fuzzing under -jobs=4
> > -workers=4 (this initialization happens after libfuzzer has already
> > forked the processes). This seems like an interesting alternative to
> > using fake null-co:// files. 
> > I wonder if some state might leak as these disks are filled with fuzzer
> > data.
> Yes, i've also chosen between the fake null device and temporary file.
> Tried this approach, just to see what will happen ). It seems to me that
> slightly different paths can be triggered in this case and it is closer
> to real usage.
> But indeed, mb some state can leak, this is interesting.
> 
> > 
> > Nit: these disk files remain after the fuzzer exists. It looks
> > like the libfuzzer people suggest simply using atexit() to perform
> > cleanup: https://reviews.llvm.org/D45762
> > The is that the only way I have found to terminate the fuzzer is with
> > SIGKILL, where atexit is skipped. QEMU installs some signal handlers in
> > os-posix.c:os_setup_signal_handling to notify the main_loop that the
> > qemu was killed. Since we replace qemu_main_loop by manually running
> > main_loop_wait, we don't check main_loop_should_exit().
> Got it! Thanks for sharing this is good to know ).
> 
> No other comments mixed in below.
> 
> Dima.
> > 
> > I sent a patch to disable QEMU's signal handlers for the fuzzer.
> > Message-Id: <20201013152920.448335-1-alxndr@bu.edu>
Sorry, i couldn't find a patch you've pointed out above. Could you share
some link to it? Also, am i correct that it is a general change for the
QEMU fuzzing, so all the fuzzing targets will automatically reuse it?

> > 
> > With an atexit() call to clean up the temporary images:
> > Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> > 
> > > +static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
> > > +{
> > > +    char *tmp_path = drive_create();
> > > +
> > > +    g_string_append_printf(cmd_line,
> > > +                           " -drive if=none,id=drive0,file=%s,"
> > > +                           "format=raw,auto-read-only=off ",
> > > +                           tmp_path);
> > > +
> > > +    return arg;
> > > +}
> > > +
> > > +static void register_virtio_blk_fuzz_targets(void)
> > > +{
> > > +    fuzz_add_qos_target(&(FuzzTarget){
> > > +                .name = "virtio-blk-fuzz",
> > > +                .description = "Fuzz the virtio-blk virtual queues, forking "
> > > +                                "for each fuzz run",
> > > +                .pre_vm_init = &counter_shm_init,
> > > +                .pre_fuzz = &virtio_blk_pre_fuzz,
> > > +                .fuzz = virtio_blk_fork_fuzz,},
> > > +                "virtio-blk",
> > > +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> > > +                );
> > > +
> > > +    fuzz_add_qos_target(&(FuzzTarget){
> > > +                .name = "virtio-blk-flags-fuzz",
> > > +                .description = "Fuzz the virtio-blk virtual queues, forking "
> > > +                "for each fuzz run (also fuzzes the virtio flags)",
> > > +                .pre_vm_init = &counter_shm_init,
> > > +                .pre_fuzz = &virtio_blk_pre_fuzz,
> > > +                .fuzz = virtio_blk_with_flag_fuzz,},
> > > +                "virtio-blk",
> > > +                &(QOSGraphTestOptions){.before = virtio_blk_test_setup}
> > > +                );
> > > +}
> > > +
> > > +fuzz_target_init(register_virtio_blk_fuzz_targets);
> > > -- 
> > > 2.7.4
> > > 

Re: [PATCH v1 1/2] fuzz: add virtio-blk fuzz target

[Darren Kenny]

Hi Dima,

On Wednesday, 2020-10-14 at 10:39:01 +03, Dima Stepanov wrote:
> On Wed, Oct 14, 2020 at 10:29:41AM +0300, Dima Stepanov wrote:
>> On Tue, Oct 13, 2020 at 11:30:52AM -0400, Alexander Bulekov wrote:
>> > On 201007 1647, Dima Stepanov wrote:
...

>> > 
>> > I sent a patch to disable QEMU's signal handlers for the fuzzer.
>> > Message-Id: <20201013152920.448335-1-alxndr@bu.edu>
> Sorry, i couldn't find a patch you've pointed out above. Could you share
> some link to it? Also, am i correct that it is a general change for the
> QEMU fuzzing, so all the fuzzing targets will automatically reuse it?
>

The patch Alex is referring to is:

- https://lore.kernel.org/qemu-devel/20201013152920.448335-1-alxndr@bu.edu/

Thanks,

Darren.

[PATCH v1 2/2] docs/fuzz: update make and run command lines

[Dima Stepanov]

After meson and some other build changes the qemu fuzz target should be
build as:
  make qemu-fuzz-i386
And also update the run path command line.

Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
---
 docs/devel/fuzzing.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt
index 96d71c9..7846b9e 100644
--- a/docs/devel/fuzzing.txt
+++ b/docs/devel/fuzzing.txt
@@ -32,15 +32,15 @@ such as out-of-bounds accesses, use-after-frees, double-frees etc.
 
 Fuzz targets are built similarly to system/softmmu:
 
-    make i386-softmmu/fuzz
+    make qemu-fuzz-i386
 
-This builds ./i386-softmmu/qemu-fuzz-i386
+This builds ./build/qemu-fuzz-i386
 
 The first option to this command is: --fuzz-target=FUZZ_NAME
 To list all of the available fuzzers run qemu-fuzz-i386 with no arguments.
 
 For example:
-    ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=virtio-scsi-fuzz
+    ./build/qemu-fuzz-i386 --fuzz-target=virtio-scsi-fuzz
 
 Internally, libfuzzer parses all arguments that do not begin with "--".
 Information about these is available by passing -help=1
-- 
2.7.4

Re: [PATCH v1 2/2] docs/fuzz: update make and run command lines

[Alexander Bulekov]

On 201007 1647, Dima Stepanov wrote:
> After meson and some other build changes the qemu fuzz target should be
> build as:
>   make qemu-fuzz-i386
> And also update the run path command line.
> 
> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> ---
>  docs/devel/fuzzing.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt
> index 96d71c9..7846b9e 100644
> --- a/docs/devel/fuzzing.txt
> +++ b/docs/devel/fuzzing.txt
> @@ -32,15 +32,15 @@ such as out-of-bounds accesses, use-after-frees, double-frees etc.
>  
>  Fuzz targets are built similarly to system/softmmu:
>  
> -    make i386-softmmu/fuzz
> +    make qemu-fuzz-i386
>  
> -This builds ./i386-softmmu/qemu-fuzz-i386
> +This builds ./build/qemu-fuzz-i386
>  
>  The first option to this command is: --fuzz-target=FUZZ_NAME
>  To list all of the available fuzzers run qemu-fuzz-i386 with no arguments.
>  
>  For example:
> -    ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=virtio-scsi-fuzz
> +    ./build/qemu-fuzz-i386 --fuzz-target=virtio-scsi-fuzz
>  

TIL that in-tree configures now happen in ./build/

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

Thanks

>  Internally, libfuzzer parses all arguments that do not begin with "--".
>  Information about these is available by passing -help=1
> -- 
> 2.7.4
>