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

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

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