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

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

[Dima Stepanov]

v1 -> v2:
  - Update meson build with the CONFIG_VIRTIO_BLK case
  - Add "Reviewed-by: Alexander Bulekov" for both patches

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 v2 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>
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
---
 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 5162321..8af6848 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(when: 'CONFIG_VIRTIO_BLK', if_true: files('virtio_blk_fuzz.c'))
 specific_fuzz_ss.add(files('generic_fuzz.c'))
 
 fork_fuzz = declare_dependency(
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 v2 1/2] fuzz: add virtio-blk fuzz target

[Thomas Huth]

On 09/11/2020 12.25, 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>
> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  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

Thanks, I can add this to my next qtest-related pull request.
Note: I just spotted your patch by accident ... please put me on CC: if you
want me to merge qtest/fuzzer related patches.

 Thomas

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

[Dima Stepanov]

On Mon, Nov 09, 2020 at 01:24:20PM +0100, Thomas Huth wrote:
> On 09/11/2020 12.25, 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>
> > Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> >  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
> 
> Thanks, I can add this to my next qtest-related pull request.
> Note: I just spotted your patch by accident ... please put me on CC: if you
> want me to merge qtest/fuzzer related patches.

Hi Thomas,

Thanks for handling it. Got it! Will add you to CC for the next fuzzing
pull requests.

Dima.

> 
>  Thomas
> 

[PATCH v2 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>
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
---
 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 03585c1..590900c 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 v2 2/2] docs/fuzz: update make and run command lines

[Thomas Huth]

On 09/11/2020 12.25, 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>
> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  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 03585c1..590900c 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

This conflicts with Alexander's "docs/fuzz: update fuzzing documentation
post-meson" patch ... which one should be used? One is better for people who
do in-tree builds, the other is better for people who do out-of-tree builds
... I'll go with Alexander's version for now for my next pull-request, since
the text in this file says "/path/to/configure" which rather indicate
out-of-tree builds, but in the long run, it would maybe be good to add some
clarifying sentences here...

 Thomas

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

[Alexander Bulekov]

On 201109 1320, Thomas Huth wrote:
> On 09/11/2020 12.25, 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>
> > Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> >  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 03585c1..590900c 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
> 
> This conflicts with Alexander's "docs/fuzz: update fuzzing documentation
> post-meson" patch ... which one should be used? One is better for people who
> do in-tree builds, the other is better for people who do out-of-tree builds

Ah my bad - I forgot that Dima also sent in fixes to these instructions.

> ... I'll go with Alexander's version for now for my next pull-request, since
> the text in this file says "/path/to/configure" which rather indicate
> out-of-tree builds, but in the long run, it would maybe be good to add some
> clarifying sentences here...
> 

Sounds good - I'll add more context/clarification to these instructions
later.

- Alex

>  Thomas
> 
>