Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing

[Alexander Bulekov]

On 210118 1536, Darren Kenny wrote:
> On Sunday, 2021-01-17 at 18:09:24 -05, Alexander Bulekov wrote:
> > virtio-9p devices are often used to expose a virtual-filesystem to the
> > guest. There have been some bugs reported in this device, such as
> > CVE-2018-19364, and CVE-2021-20181. We should fuzz this device
> >
> > This patch adds two virtio-9p configurations:
> >  * One with the widely used -fsdev local driver. This driver leaks some
> >    state in the form of files/directories created in the shared dir.
> >  * One with the synth driver. While it is not used in the real world, this
> >    driver won't leak leak state between fuzz inputs.
> >
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> > CC: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > CC: Greg Kurz <groug@kaod.org>
> >
> > I considered adding an atexit handler to remove the temp directory,
> > however I am worried that there might be some error that results in a
> > call to exit(), rather than abort(), which will cause problems for
> > future fork()-ed fuzzers. I don't think there are such calls in the 9p
> > code, however there might be something in the APIs used by 9p. As this
> > code is primarily for ephemeral OSS-Fuzz conainers, this shouldn't be
> > too much of an issue.
> 
> As I understand it, this creation of a new directory should happen a lot
> less than the amount of actual executions of the target, since it is
> only run on the first 'parent' target process executed, prior to the
> process cloning operations that the fork execution performs - or any
> time that 'parent' target process is renewed, after several thousand
> cloned processes.
> 
> Is that correct? Or am I misunderstanding when the
> generic_fuzzer_virtio_9p_args() function is run?

Correct. It happens once: before we initialize QEMU in the parent
fuzzing process. There are two questions:
1. What is the best way to remove the directory after the parent process
   exits(and not after child processes exit())?
2. Should we do any cleanup on the temp directory between input
   executions.
-Alex

> 
> Thanks,
> 
> Darren.
> 
> >  tests/qtest/fuzz/generic_fuzz_configs.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h 
> > b/tests/qtest/fuzz/generic_fuzz_configs.h
> > index 1a133655ee..f99657cdbc 100644
> > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > @@ -19,6 +19,16 @@ typedef struct generic_fuzz_config {
> >      gchar* (*argfunc)(void); /* Result must be freeable by g_free() */
> >  } generic_fuzz_config;
> >  
> > +static inline gchar *generic_fuzzer_virtio_9p_args(void){
> > +    char tmpdir[] = "/tmp/qemu-fuzz.XXXXXX";
> > +    g_assert_nonnull(mkdtemp(tmpdir));
> > +
> > +    return g_strdup_printf("-machine q35 -nodefaults "
> > +    "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +    "-fsdev local,id=hshare,path=%s,security_model=mapped-xattr,"
> > +    "writeout=immediate,fmode=0600,dmode=0700", tmpdir);
> > +}
> > +
> >  const generic_fuzz_config predefined_configs[] = {
> >      {
> >          .name = "virtio-net-pci-slirp",
> > @@ -60,6 +70,16 @@ const generic_fuzz_config predefined_configs[] = {
> >          .name = "virtio-mouse",
> >          .args = "-machine q35 -nodefaults -device virtio-mouse",
> >          .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p",
> > +        .argfunc = generic_fuzzer_virtio_9p_args,
> > +        .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p-synth",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev synth,id=hshare",
> > +        .objects = "virtio*",
> >      },{
> >          .name = "e1000",
> >          .args = "-M q35 -nodefaults "
> > -- 
> > 2.28.0