* [PATCH v2 0/3] fuzz: Add 9p generic-fuzz configs @ 2021-01-17 23:09 Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 1/3] fuzz: enable dynamic args for " Alexander Bulekov ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Alexander Bulekov @ 2021-01-17 23:09 UTC (permalink / raw) To: qemu-devel; +Cc: Alexander Bulekov Patch 1 enables generic-fuzzer configs to setup resources (such as temp directories) at runtime. Patch 2 adds some documentation about OSS-Fuzz (including the feature added in Patch 1) Patch 3 adds two virtio-9p generic-fuzz configs. Once of these configs leverages the capability added in Patch 1 to create a temp directory for the fuzzer. v2: fix spelling mistakes and replace free() with g_free() Alexander Bulekov (3): fuzz: enable dynamic args for generic-fuzz configs docs/fuzz: add some information about OSS-Fuzz fuzz: add virtio-9p configurations for fuzzing docs/devel/fuzzing.rst | 26 +++++++++++++++++++++++++ tests/qtest/fuzz/generic_fuzz.c | 10 +++++++++- tests/qtest/fuzz/generic_fuzz_configs.h | 21 ++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) -- 2.28.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] fuzz: enable dynamic args for generic-fuzz configs 2021-01-17 23:09 [PATCH v2 0/3] fuzz: Add 9p generic-fuzz configs Alexander Bulekov @ 2021-01-17 23:09 ` Alexander Bulekov 2021-01-18 9:25 ` Thomas Huth 2021-01-17 23:09 ` [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing Alexander Bulekov 2 siblings, 1 reply; 14+ messages in thread From: Alexander Bulekov @ 2021-01-17 23:09 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini For some device configurations, it is useful to configure some resources, and adjust QEMU arguments at runtime, prior to fuzzing. This patch adds an "argfunc" to generic the generic_fuzz_config. When specified, it is responsible for configuring the resources and returning a string containing the corresponding QEMU arguments. This can be useful for targets that rely on e.g.: * a temporary qcow2 image * a temporary directory * an unused TCP port used to bind the VNC server Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- tests/qtest/fuzz/generic_fuzz.c | 10 +++++++++- tests/qtest/fuzz/generic_fuzz_configs.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index be76d47d2d..6adf62a5be 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -936,12 +936,20 @@ static GString *generic_fuzz_cmdline(FuzzTarget *t) static GString *generic_fuzz_predefined_config_cmdline(FuzzTarget *t) { + gchar *args; const generic_fuzz_config *config; g_assert(t->opaque); config = t->opaque; setenv("QEMU_AVOID_DOUBLE_FETCH", "1", 1); - setenv("QEMU_FUZZ_ARGS", config->args, 1); + if (config->argfunc) { + args = config->argfunc(); + setenv("QEMU_FUZZ_ARGS", args, 1); + g_free(args); + } else { + g_assert_nonnull(config->args); + setenv("QEMU_FUZZ_ARGS", config->args, 1); + } setenv("QEMU_FUZZ_OBJECTS", config->objects, 1); return generic_fuzz_cmdline(t); } diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h index 7fed035345..1a133655ee 100644 --- a/tests/qtest/fuzz/generic_fuzz_configs.h +++ b/tests/qtest/fuzz/generic_fuzz_configs.h @@ -16,6 +16,7 @@ typedef struct generic_fuzz_config { const char *name, *args, *objects; + gchar* (*argfunc)(void); /* Result must be freeable by g_free() */ } generic_fuzz_config; const generic_fuzz_config predefined_configs[] = { -- 2.28.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] fuzz: enable dynamic args for generic-fuzz configs 2021-01-17 23:09 ` [PATCH v2 1/3] fuzz: enable dynamic args for " Alexander Bulekov @ 2021-01-18 9:25 ` Thomas Huth 0 siblings, 0 replies; 14+ messages in thread From: Thomas Huth @ 2021-01-18 9:25 UTC (permalink / raw) To: Alexander Bulekov, qemu-devel Cc: Laurent Vivier, Paolo Bonzini, Bandan Das, Stefan Hajnoczi On 18/01/2021 00.09, Alexander Bulekov wrote: > For some device configurations, it is useful to configure some > resources, and adjust QEMU arguments at runtime, prior to fuzzing. This > patch adds an "argfunc" to generic the generic_fuzz_config. When > specified, it is responsible for configuring the resources and returning > a string containing the corresponding QEMU arguments. This can be useful > for targets that rely on e.g.: > * a temporary qcow2 image > * a temporary directory > * an unused TCP port used to bind the VNC server > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > --- > tests/qtest/fuzz/generic_fuzz.c | 10 +++++++++- > tests/qtest/fuzz/generic_fuzz_configs.h | 1 + > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c > index be76d47d2d..6adf62a5be 100644 > --- a/tests/qtest/fuzz/generic_fuzz.c > +++ b/tests/qtest/fuzz/generic_fuzz.c > @@ -936,12 +936,20 @@ static GString *generic_fuzz_cmdline(FuzzTarget *t) > > static GString *generic_fuzz_predefined_config_cmdline(FuzzTarget *t) > { > + gchar *args; > const generic_fuzz_config *config; > g_assert(t->opaque); > > config = t->opaque; > setenv("QEMU_AVOID_DOUBLE_FETCH", "1", 1); > - setenv("QEMU_FUZZ_ARGS", config->args, 1); > + if (config->argfunc) { > + args = config->argfunc(); > + setenv("QEMU_FUZZ_ARGS", args, 1); > + g_free(args); > + } else { > + g_assert_nonnull(config->args); > + setenv("QEMU_FUZZ_ARGS", config->args, 1); > + } > setenv("QEMU_FUZZ_OBJECTS", config->objects, 1); > return generic_fuzz_cmdline(t); > } > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h > index 7fed035345..1a133655ee 100644 > --- a/tests/qtest/fuzz/generic_fuzz_configs.h > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h > @@ -16,6 +16,7 @@ > > typedef struct generic_fuzz_config { > const char *name, *args, *objects; > + gchar* (*argfunc)(void); /* Result must be free Reviewed-by: Thomas Huth <thuth@redhat.com> ... would it make sense to also add a cleanup function pointer here, so that the resources can also be freed cleanly after a test has succeeded (instead of using atexit() like suggested in your third patch)? Well, just an idea, it still can be done in a later patch. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz 2021-01-17 23:09 [PATCH v2 0/3] fuzz: Add 9p generic-fuzz configs Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 1/3] fuzz: enable dynamic args for " Alexander Bulekov @ 2021-01-17 23:09 ` Alexander Bulekov 2021-01-18 15:17 ` Darren Kenny 2021-01-17 23:09 ` [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing Alexander Bulekov 2 siblings, 1 reply; 14+ messages in thread From: Alexander Bulekov @ 2021-01-17 23:09 UTC (permalink / raw) To: qemu-devel Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Stefan Hajnoczi, Paolo Bonzini Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- docs/devel/fuzzing.rst | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/devel/fuzzing.rst b/docs/devel/fuzzing.rst index 6096242d99..8792358854 100644 --- a/docs/devel/fuzzing.rst +++ b/docs/devel/fuzzing.rst @@ -181,6 +181,32 @@ To ensure that these env variables have been configured correctly, we can use:: The output should contain a complete list of matched MemoryRegions. +OSS-Fuzz +-------- +QEMU is continuously fuzzed on `OSS-Fuzz` __(https://github.com/google/oss-fuzz). +By default, the OSS-Fuzz build will try to fuzz every fuzz-target. Since the +generic-fuzz target requires additional information provided in environment +variables, we pre-define some generic-fuzz configs in +``tests/qtest/fuzz/generic_fuzz_configs.h``. Each config must specify: + * ``.name``: To identify the fuzzer config + * ``.args`` OR ``.argfunc``: A string or pointer to a function returning a + string. These strings are used to specify the ``QEMU_FUZZ_ARGS`` + environment variable. ``argfunc`` is useful when the config relies on e.g. + a dynamically created temp directory, or a free tcp/udp port. + * ``.objects``: A string that specifies the ``QEMU_FUZZ_OBJECTS`` environment + variable. + +To fuzz additional devices/device configuration on OSS-Fuzz: + * Send patches for a new device-specific fuzzer + * Send patches for a new generic-fuzz config + +Build details: + * `The basic Dockerfile that sets up the environment for building QEMU's + fuzzers on OSS-Fuzz + <https://github.com/google/oss-fuzz/blob/master/projects/qemu/Dockerfile>`_ + * The script responsible for building the fuzzers: + ``scripts/oss-fuzz/build.sh`` + Implementation Details / Fuzzer Lifecycle ----------------------------------------- -- 2.28.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz 2021-01-17 23:09 ` [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz Alexander Bulekov @ 2021-01-18 15:17 ` Darren Kenny 0 siblings, 0 replies; 14+ messages in thread From: Darren Kenny @ 2021-01-18 15:17 UTC (permalink / raw) To: Alexander Bulekov, qemu-devel Cc: Alexander Bulekov, Bandan Das, Thomas Huth, Stefan Hajnoczi, Paolo Bonzini On Sunday, 2021-01-17 at 18:09:23 -05, Alexander Bulekov wrote: > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > --- > docs/devel/fuzzing.rst | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/docs/devel/fuzzing.rst b/docs/devel/fuzzing.rst > index 6096242d99..8792358854 100644 > --- a/docs/devel/fuzzing.rst > +++ b/docs/devel/fuzzing.rst > @@ -181,6 +181,32 @@ To ensure that these env variables have been configured correctly, we can use:: > > The output should contain a complete list of matched MemoryRegions. > > +OSS-Fuzz > +-------- > +QEMU is continuously fuzzed on `OSS-Fuzz` __(https://github.com/google/oss-fuzz). > +By default, the OSS-Fuzz build will try to fuzz every fuzz-target. Since the > +generic-fuzz target requires additional information provided in environment > +variables, we pre-define some generic-fuzz configs in > +``tests/qtest/fuzz/generic_fuzz_configs.h``. Each config must specify: > + * ``.name``: To identify the fuzzer config > + * ``.args`` OR ``.argfunc``: A string or pointer to a function returning a > + string. These strings are used to specify the ``QEMU_FUZZ_ARGS`` > + environment variable. ``argfunc`` is useful when the config relies on e.g. > + a dynamically created temp directory, or a free tcp/udp port. > + * ``.objects``: A string that specifies the ``QEMU_FUZZ_OBJECTS`` environment > + variable. > + > +To fuzz additional devices/device configuration on OSS-Fuzz: > + * Send patches for a new device-specific fuzzer > + * Send patches for a new generic-fuzz config > + > +Build details: > + * `The basic Dockerfile that sets up the environment for building QEMU's > + fuzzers on OSS-Fuzz > + <https://github.com/google/oss-fuzz/blob/master/projects/qemu/Dockerfile>`_ > + * The script responsible for building the fuzzers: > + ``scripts/oss-fuzz/build.sh`` > + > Implementation Details / Fuzzer Lifecycle > ----------------------------------------- > > -- > 2.28.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-17 23:09 [PATCH v2 0/3] fuzz: Add 9p generic-fuzz configs Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 1/3] fuzz: enable dynamic args for " Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz Alexander Bulekov @ 2021-01-17 23:09 ` Alexander Bulekov 2021-01-18 12:34 ` qemu_oss--- via 2021-01-18 15:36 ` Darren Kenny 2 siblings, 2 replies; 14+ messages in thread From: Alexander Bulekov @ 2021-01-17 23:09 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Christian Schoenebeck, Greg Kurz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini 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. 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-17 23:09 ` [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing Alexander Bulekov @ 2021-01-18 12:34 ` qemu_oss--- via 2021-01-18 15:30 ` Alexander Bulekov 2021-01-18 15:36 ` Darren Kenny 1 sibling, 1 reply; 14+ messages in thread From: qemu_oss--- via @ 2021-01-18 12:34 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Greg Kurz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On Montag, 18. Januar 2021 00:09:24 CET 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. Yes, dealing with signal handlers for that is probably a bit intransparent and would leave a questionable feeling about its reliability. What about __attribute__((destructor)) to auto delete the fuzzer directory, like virtio-9p-test.c does for the same task? > 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 " ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-18 12:34 ` qemu_oss--- via @ 2021-01-18 15:30 ` Alexander Bulekov 2021-01-18 15:40 ` Darren Kenny 0 siblings, 1 reply; 14+ messages in thread From: Alexander Bulekov @ 2021-01-18 15:30 UTC (permalink / raw) To: Christian Schoenebeck Cc: Laurent Vivier, Thomas Huth, qemu-devel, Greg Kurz, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On 210118 1334, Christian Schoenebeck wrote: > On Montag, 18. Januar 2021 00:09:24 CET 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. > > Yes, dealing with signal handlers for that is probably a bit intransparent and > would leave a questionable feeling about its reliability. > > What about __attribute__((destructor)) to auto delete the fuzzer directory, > like virtio-9p-test.c does for the same task? > My worry is that we will end up deleting it while it is still in use. The scenario I am worried about: [parent process ] set up temp directory for virtio-9p [parent process ] initialize QEMU [parent process ] fork() and wait() [child process 1] Run the fuzzing input. [child process 1] Once the input has been executed, call _Exit(). This should skip the atexit()/__attribute((destructor)) handlers. For reasons related to libfuzzer, we need to do this so that libfuzzer doesn't treat each child exit()-ing as a crash. [parent process ] wait() returns. [parent process ] generate a new input.. fork() and wait() [child process 2] Run the fuzzing input. [child process 2] Somewhere we hit an abort(). libfuzzer hooks the abort and dumps the crashing input and stack trace. Since abort() doesn't call exit handlers, it will skip over atexit()/__attribute((destructor)) handlers [parent process ] wait() returns. [parent process ] generate a new input.. fork() and wait() [child process 3] Run the fuzzing input. [child process 3] Somewhere we hit an exit(). This will dump the input/stacktrace and it will run the exit handlers (removing the shared 9p directory) [parent process ] wait() returns. generate a new input.. fork() and wait() [child process 4] ... Now all the subsequent forked children will refer to a shared directory that we already removed. Ideally, we would run the cleanup handler only after the parent process exit()s. I think there are some ways to do this, by placing the atexit() call in a place only reachable by the parent. However, on OSS-Fuzz this shouldn't be a problem as everything is cleaned up automatically anyway.. I am more worried about the fact that files/directories/links that are created by 9p in the child processes, persist across inputs. I think Thomas suggested a way to work-around this for PATCH 1/3. We could have a function that runs in the parent after each wait() returns, that would remove all the files in the temp directory and scrub the extended attributes applied by 9p to the shared dir. -Alex > > 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 " > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-18 15:30 ` Alexander Bulekov @ 2021-01-18 15:40 ` Darren Kenny 2021-01-19 15:12 ` Alexander Bulekov 0 siblings, 1 reply; 14+ messages in thread From: Darren Kenny @ 2021-01-18 15:40 UTC (permalink / raw) To: Alexander Bulekov, Christian Schoenebeck Cc: Laurent Vivier, Thomas Huth, qemu-devel, Greg Kurz, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On Monday, 2021-01-18 at 10:30:33 -05, Alexander Bulekov wrote: > On 210118 1334, Christian Schoenebeck wrote: >> On Montag, 18. Januar 2021 00:09:24 CET 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. >> >> Yes, dealing with signal handlers for that is probably a bit intransparent and >> would leave a questionable feeling about its reliability. >> >> What about __attribute__((destructor)) to auto delete the fuzzer directory, >> like virtio-9p-test.c does for the same task? >> > > My worry is that we will end up deleting it while it is still in use. > The scenario I am worried about: > [parent process ] set up temp directory for virtio-9p > [parent process ] initialize QEMU > [parent process ] fork() and wait() > [child process 1] Run the fuzzing input. > [child process 1] Once the input has been executed, call _Exit(). This > should skip the atexit()/__attribute((destructor)) handlers. For reasons > related to libfuzzer, we need to do this so that libfuzzer doesn't treat > each child exit()-ing as a crash. > [parent process ] wait() returns. > [parent process ] generate a new input.. fork() and wait() > [child process 2] Run the fuzzing input. > [child process 2] Somewhere we hit an abort(). libfuzzer hooks the abort > and dumps the crashing input and stack trace. Since abort() doesn't call > exit handlers, it will skip over atexit()/__attribute((destructor)) handlers > [parent process ] wait() returns. > [parent process ] generate a new input.. fork() and wait() > [child process 3] Run the fuzzing input. > [child process 3] Somewhere we hit an exit(). This will dump the > input/stacktrace and it will run the exit handlers (removing the shared > 9p directory) > [parent process ] wait() returns. generate a new input.. fork() and wait() > [child process 4] ... OK, that answer's my question :) > Now all the subsequent forked children will refer to a shared directory > that we already removed. Ideally, we would run the cleanup handler only > after the parent process exit()s. I think there are some ways to do > this, by placing the atexit() call in a place only reachable by the > parent. However, on OSS-Fuzz this shouldn't be a problem as everything > is cleaned up automatically anyway.. Yep, agreed. > I am more worried about the fact that files/directories/links that are > created by 9p in the child processes, persist across inputs. I think > Thomas suggested a way to work-around this for PATCH 1/3. We could have > a function that runs in the parent after each wait() returns, that would > remove all the files in the temp directory and scrub the extended > attributes applied by 9p to the shared dir. Hmm, that sounds like something to consider, but it may also end up slowing down the execution during the turn-around - guess it depends on how much noise is being generated. Thanks, Darren. > -Alex > > >> > 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 " >> >> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-18 15:40 ` Darren Kenny @ 2021-01-19 15:12 ` Alexander Bulekov 2021-01-19 15:44 ` Darren Kenny 0 siblings, 1 reply; 14+ messages in thread From: Alexander Bulekov @ 2021-01-19 15:12 UTC (permalink / raw) To: Darren Kenny Cc: Laurent Vivier, Thomas Huth, Christian Schoenebeck, qemu-devel, Greg Kurz, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On 210118 1540, Darren Kenny wrote: > On Monday, 2021-01-18 at 10:30:33 -05, Alexander Bulekov wrote: > > On 210118 1334, Christian Schoenebeck wrote: > >> On Montag, 18. Januar 2021 00:09:24 CET 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. > >> > >> Yes, dealing with signal handlers for that is probably a bit intransparent and > >> would leave a questionable feeling about its reliability. > >> > >> What about __attribute__((destructor)) to auto delete the fuzzer directory, > >> like virtio-9p-test.c does for the same task? > >> > > > > My worry is that we will end up deleting it while it is still in use. > > The scenario I am worried about: > > [parent process ] set up temp directory for virtio-9p > > [parent process ] initialize QEMU > > [parent process ] fork() and wait() > > [child process 1] Run the fuzzing input. > > [child process 1] Once the input has been executed, call _Exit(). This > > should skip the atexit()/__attribute((destructor)) handlers. For reasons > > related to libfuzzer, we need to do this so that libfuzzer doesn't treat > > each child exit()-ing as a crash. > > [parent process ] wait() returns. > > [parent process ] generate a new input.. fork() and wait() > > [child process 2] Run the fuzzing input. > > [child process 2] Somewhere we hit an abort(). libfuzzer hooks the abort > > and dumps the crashing input and stack trace. Since abort() doesn't call > > exit handlers, it will skip over atexit()/__attribute((destructor)) handlers > > [parent process ] wait() returns. > > [parent process ] generate a new input.. fork() and wait() > > [child process 3] Run the fuzzing input. > > [child process 3] Somewhere we hit an exit(). This will dump the > > input/stacktrace and it will run the exit handlers (removing the shared > > 9p directory) > > [parent process ] wait() returns. generate a new input.. fork() and wait() > > [child process 4] ... > > OK, that answer's my question :) > > > Now all the subsequent forked children will refer to a shared directory > > that we already removed. Ideally, we would run the cleanup handler only > > after the parent process exit()s. I think there are some ways to do > > this, by placing the atexit() call in a place only reachable by the > > parent. However, on OSS-Fuzz this shouldn't be a problem as everything > > is cleaned up automatically anyway.. > > Yep, agreed. > > > I am more worried about the fact that files/directories/links that are > > created by 9p in the child processes, persist across inputs. I think > > Thomas suggested a way to work-around this for PATCH 1/3. We could have > > a function that runs in the parent after each wait() returns, that would > > remove all the files in the temp directory and scrub the extended > > attributes applied by 9p to the shared dir. > > Hmm, that sounds like something to consider, but it may also end up > slowing down the execution during the turn-around - guess it depends on > how much noise is being generated. > I've ben running the fuzzer for a couple days, and I haven't noticed any issues with unreproducible inputs (yet). Is this something we can add later, if it becomes a problem? -Alex > Thanks, > > Darren. > > > -Alex > > > > > >> > 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 " > >> > >> > >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-19 15:12 ` Alexander Bulekov @ 2021-01-19 15:44 ` Darren Kenny 2021-01-19 16:15 ` qemu_oss--- via 0 siblings, 1 reply; 14+ messages in thread From: Darren Kenny @ 2021-01-19 15:44 UTC (permalink / raw) To: Alexander Bulekov Cc: Laurent Vivier, Thomas Huth, Christian Schoenebeck, qemu-devel, Greg Kurz, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On Tuesday, 2021-01-19 at 10:12:29 -05, Alexander Bulekov wrote: > On 210118 1540, Darren Kenny wrote: >> On Monday, 2021-01-18 at 10:30:33 -05, Alexander Bulekov wrote: >> > On 210118 1334, Christian Schoenebeck wrote: >> >> On Montag, 18. Januar 2021 00:09:24 CET 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. >> >> >> >> Yes, dealing with signal handlers for that is probably a bit intransparent and >> >> would leave a questionable feeling about its reliability. >> >> >> >> What about __attribute__((destructor)) to auto delete the fuzzer directory, >> >> like virtio-9p-test.c does for the same task? >> >> >> > >> > My worry is that we will end up deleting it while it is still in use. >> > The scenario I am worried about: >> > [parent process ] set up temp directory for virtio-9p >> > [parent process ] initialize QEMU >> > [parent process ] fork() and wait() >> > [child process 1] Run the fuzzing input. >> > [child process 1] Once the input has been executed, call _Exit(). This >> > should skip the atexit()/__attribute((destructor)) handlers. For reasons >> > related to libfuzzer, we need to do this so that libfuzzer doesn't treat >> > each child exit()-ing as a crash. >> > [parent process ] wait() returns. >> > [parent process ] generate a new input.. fork() and wait() >> > [child process 2] Run the fuzzing input. >> > [child process 2] Somewhere we hit an abort(). libfuzzer hooks the abort >> > and dumps the crashing input and stack trace. Since abort() doesn't call >> > exit handlers, it will skip over atexit()/__attribute((destructor)) handlers >> > [parent process ] wait() returns. >> > [parent process ] generate a new input.. fork() and wait() >> > [child process 3] Run the fuzzing input. >> > [child process 3] Somewhere we hit an exit(). This will dump the >> > input/stacktrace and it will run the exit handlers (removing the shared >> > 9p directory) >> > [parent process ] wait() returns. generate a new input.. fork() and wait() >> > [child process 4] ... >> >> OK, that answer's my question :) >> >> > Now all the subsequent forked children will refer to a shared directory >> > that we already removed. Ideally, we would run the cleanup handler only >> > after the parent process exit()s. I think there are some ways to do >> > this, by placing the atexit() call in a place only reachable by the >> > parent. However, on OSS-Fuzz this shouldn't be a problem as everything >> > is cleaned up automatically anyway.. >> >> Yep, agreed. >> >> > I am more worried about the fact that files/directories/links that are >> > created by 9p in the child processes, persist across inputs. I think >> > Thomas suggested a way to work-around this for PATCH 1/3. We could have >> > a function that runs in the parent after each wait() returns, that would >> > remove all the files in the temp directory and scrub the extended >> > attributes applied by 9p to the shared dir. >> >> Hmm, that sounds like something to consider, but it may also end up >> slowing down the execution during the turn-around - guess it depends on >> how much noise is being generated. >> > > I've ben running the fuzzer for a couple days, and I haven't noticed any > issues with unreproducible inputs (yet). Is this something we can add > later, if it becomes a problem? Sure, I'm good with that: Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Thanks, Darren. > -Alex > >> Thanks, >> >> Darren. >> >> > -Alex >> > >> > >> >> > 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 " >> >> >> >> >> >> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-19 15:44 ` Darren Kenny @ 2021-01-19 16:15 ` qemu_oss--- via 0 siblings, 0 replies; 14+ messages in thread From: qemu_oss--- via @ 2021-01-19 16:15 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Thomas Huth, Alexander Bulekov, Greg Kurz, Darren Kenny, Bandan Das, Stefan Hajnoczi, Paolo Bonzini On Dienstag, 19. Januar 2021 16:44:31 CET Darren Kenny wrote: > On Tuesday, 2021-01-19 at 10:12:29 -05, Alexander Bulekov wrote: > > On 210118 1540, Darren Kenny wrote: > >> On Monday, 2021-01-18 at 10:30:33 -05, Alexander Bulekov wrote: > >> > On 210118 1334, Christian Schoenebeck wrote: > >> >> On Montag, 18. Januar 2021 00:09:24 CET 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. > >> >> > >> >> Yes, dealing with signal handlers for that is probably a bit > >> >> intransparent and would leave a questionable feeling about its > >> >> reliability. > >> >> > >> >> What about __attribute__((destructor)) to auto delete the fuzzer > >> >> directory, > >> >> like virtio-9p-test.c does for the same task? > >> > > >> > My worry is that we will end up deleting it while it is still in use. > >> > The scenario I am worried about: > >> > [parent process ] set up temp directory for virtio-9p > >> > [parent process ] initialize QEMU > >> > [parent process ] fork() and wait() > >> > [child process 1] Run the fuzzing input. > >> > [child process 1] Once the input has been executed, call _Exit(). This > >> > should skip the atexit()/__attribute((destructor)) handlers. For > >> > reasons > >> > related to libfuzzer, we need to do this so that libfuzzer doesn't > >> > treat > >> > each child exit()-ing as a crash. > >> > [parent process ] wait() returns. > >> > [parent process ] generate a new input.. fork() and wait() > >> > [child process 2] Run the fuzzing input. > >> > [child process 2] Somewhere we hit an abort(). libfuzzer hooks the > >> > abort > >> > and dumps the crashing input and stack trace. Since abort() doesn't > >> > call > >> > exit handlers, it will skip over atexit()/__attribute((destructor)) > >> > handlers [parent process ] wait() returns. > >> > [parent process ] generate a new input.. fork() and wait() > >> > [child process 3] Run the fuzzing input. > >> > [child process 3] Somewhere we hit an exit(). This will dump the > >> > input/stacktrace and it will run the exit handlers (removing the shared > >> > 9p directory) > >> > [parent process ] wait() returns. generate a new input.. fork() and > >> > wait() > >> > [child process 4] ... > >> > >> OK, that answer's my question :) > >> > >> > Now all the subsequent forked children will refer to a shared directory > >> > that we already removed. Ideally, we would run the cleanup handler only > >> > after the parent process exit()s. I think there are some ways to do > >> > this, by placing the atexit() call in a place only reachable by the > >> > parent. However, on OSS-Fuzz this shouldn't be a problem as everything > >> > is cleaned up automatically anyway.. > >> > >> Yep, agreed. > >> > >> > I am more worried about the fact that files/directories/links that are > >> > created by 9p in the child processes, persist across inputs. I think > >> > Thomas suggested a way to work-around this for PATCH 1/3. We could have > >> > a function that runs in the parent after each wait() returns, that > >> > would > >> > remove all the files in the temp directory and scrub the extended > >> > attributes applied by 9p to the shared dir. > >> > >> Hmm, that sounds like something to consider, but it may also end up > >> slowing down the execution during the turn-around - guess it depends on > >> how much noise is being generated. > > > > I've ben running the fuzzer for a couple days, and I haven't noticed any > > issues with unreproducible inputs (yet). Is this something we can add > > later, if it becomes a problem? > > Sure, I'm good with that: > > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > > Thanks, > > Darren. Same with me: Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com> Best regards, Christian Schoenebeck ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-17 23:09 ` [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing Alexander Bulekov 2021-01-18 12:34 ` qemu_oss--- via @ 2021-01-18 15:36 ` Darren Kenny 2021-01-18 15:44 ` Alexander Bulekov 1 sibling, 1 reply; 14+ messages in thread From: Darren Kenny @ 2021-01-18 15:36 UTC (permalink / raw) To: Alexander Bulekov, qemu-devel Cc: Laurent Vivier, Thomas Huth, Christian Schoenebeck, Greg Kurz, Alexander Bulekov, Bandan Das, Stefan Hajnoczi, Paolo Bonzini 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? 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing 2021-01-18 15:36 ` Darren Kenny @ 2021-01-18 15:44 ` Alexander Bulekov 0 siblings, 0 replies; 14+ messages in thread From: Alexander Bulekov @ 2021-01-18 15:44 UTC (permalink / raw) To: Darren Kenny Cc: Laurent Vivier, Thomas Huth, Christian Schoenebeck, qemu-devel, Greg Kurz, Bandan Das, Stefan Hajnoczi, Paolo Bonzini 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-01-19 19:05 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-17 23:09 [PATCH v2 0/3] fuzz: Add 9p generic-fuzz configs Alexander Bulekov 2021-01-17 23:09 ` [PATCH v2 1/3] fuzz: enable dynamic args for " Alexander Bulekov 2021-01-18 9:25 ` Thomas Huth 2021-01-17 23:09 ` [PATCH v2 2/3] docs/fuzz: add some information about OSS-Fuzz Alexander Bulekov 2021-01-18 15:17 ` Darren Kenny 2021-01-17 23:09 ` [PATCH v2 3/3] fuzz: add virtio-9p configurations for fuzzing Alexander Bulekov 2021-01-18 12:34 ` qemu_oss--- via 2021-01-18 15:30 ` Alexander Bulekov 2021-01-18 15:40 ` Darren Kenny 2021-01-19 15:12 ` Alexander Bulekov 2021-01-19 15:44 ` Darren Kenny 2021-01-19 16:15 ` qemu_oss--- via 2021-01-18 15:36 ` Darren Kenny 2021-01-18 15:44 ` Alexander Bulekov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.