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

[Dima Stepanov <dimastep@yandex-team.ru>]

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