[PATCH] fuzz: Add virtio-9p configurations for fuzzing

[PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Alexander Bulekov]

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index 7fed035345..ffdb590c58 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
         .name = "virtio-mouse",
         .args = "-machine q35 -nodefaults -device virtio-mouse",
         .objects = "virtio*",
+    },{
+        .name = "virtio-9p",
+        .args = "-machine q35 -nodefaults "
+        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
+        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
+        .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

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Darren Kenny]

Hi Alex,

On Thursday, 2021-01-14 at 17:17:48 -05, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

In general this look good, so:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

but I do have a question below...

> ---
>  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
> index 7fed035345..ffdb590c58 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
>          .name = "virtio-mouse",
>          .args = "-machine q35 -nodefaults -device virtio-mouse",
>          .objects = "virtio*",
> +    },{
> +        .name = "virtio-9p",
> +        .args = "-machine q35 -nodefaults "
> +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
> +        .objects = "virtio*",

I wonder about the use of "/tmp" rather than maybe some generated name
using mkdtemp() - I also realise that the ability to generate this and
plug it in here probably doesn't exist either, hence not holding you to
it for this patch. Also the fact that in OSS-Fuzz this is run in limited
containers.

Have you observed any changes to "/tmp" while this is running? My
concerns may be unfounded since I don't really know what state things
are in while this is executed with no running OS.

Thanks,

Darren.

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Greg Kurz]

On Thu, 14 Jan 2021 17:17:48 -0500
Alexander Bulekov <alxndr@bu.edu> wrote:

> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---

No changelog at all ? 

>  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
> index 7fed035345..ffdb590c58 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
>          .name = "virtio-mouse",
>          .args = "-machine q35 -nodefaults -device virtio-mouse",
>          .objects = "virtio*",
> +    },{
> +        .name = "virtio-9p",
> +        .args = "-machine q35 -nodefaults "
> +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",

Sharing a general purpose directory like "/tmp" is definitely not a
recommended practice. This is typically the kind of thing that I'd
like to see documented in the changelog to help me understand ;-)

What operations does the fuzz test perform on the device ?

> +        .objects = "virtio*",
> +    },{
> +        .name = "virtio-9p-synth",
> +        .args = "-machine q35 -nodefaults "
> +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> +        "-fsdev synth,id=hshare",
> +        .objects = "virtio*",

Not sure this is super useful since the only known use case for
the synth fsdev driver is running the virtio-9p qtest, but
it looks fine anyway.

>      },{
>          .name = "e1000",
>          .args = "-M q35 -nodefaults "

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Christian Schoenebeck via]

On Freitag, 15. Januar 2021 13:23:08 CET Greg Kurz wrote:
> On Thu, 14 Jan 2021 17:17:48 -0500
> 
> Alexander Bulekov <alxndr@bu.edu> wrote:
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> 
> No changelog at all ?

Yeah, that's indeed quite short. :)

> >  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> > b/tests/qtest/fuzz/generic_fuzz_configs.h index 7fed035345..ffdb590c58
> > 100644
> > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
> > 
> >          .name = "virtio-mouse",
> >          .args = "-machine q35 -nodefaults -device virtio-mouse",
> >          .objects = "virtio*",
> > 
> > +    },{
> > +        .name = "virtio-9p",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
> 
> Sharing a general purpose directory like "/tmp" is definitely not a
> recommended practice. This is typically the kind of thing that I'd
> like to see documented in the changelog to help me understand ;-)

For the (non fuzzing) 9p 'local' tests Peter wanted either an auto generated 
(e.g. mkdtemp()) or at least a hard coded test path that allows a person to 
easily identify where the data came from. So I guess that applies to fuzzing 
as well, i.e. something like "/tmp/qemu-fuzz/9pfs/" at least.

Also note that the existing (non fuzzing) 9p 'local' tests auto generate  
individual test pathes with mkdtemp() already. This was necessary there, 
because tests are often run by "make -jN ..." in which case tests were 
accessing concurrently the same single test directory before. Probably less of 
a problem here, but you might consider using the same approach that
virtio-9p-test.c already does.

Also note that 'security_model=none' is maybe Ok as a starting point for 
fuzzing, but a realistic 9p config is rather 'security_model=xattr', because 
'security_model=none' requires the qemu process to be run as root to avoid 
permission denied errors with any minor operation. I would expect these 
fuzzing tests to mostly error out with permission denied errors early instead 
of entering relevant execution pathes.

> What operations does the fuzz test perform on the device ?
> 
> > +        .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p-synth",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev synth,id=hshare",
> > +        .objects = "virtio*",
> 
> Not sure this is super useful since the only known use case for
> the synth fsdev driver is running the virtio-9p qtest, but
> it looks fine anyway.

Yeah, that's ok. Maybe it raises the chance to enter some execution pathes 
here and there. So I would keep the 'synth' driver config.

> 
> >      },{
> >      
> >          .name = "e1000",
> >          .args = "-M q35 -nodefaults "

Nice to see fuzzing coming for 9p BTW!

Best regards,
Christian Schoenebeck

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Alexander Bulekov]

On 210115 1033, Darren Kenny wrote:
> Hi Alex,
> 
> On Thursday, 2021-01-14 at 17:17:48 -05, Alexander Bulekov wrote:
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> 
> In general this look good, so:
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> 
> but I do have a question below...
> 
> > ---
> >  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
> > index 7fed035345..ffdb590c58 100644
> > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
> >          .name = "virtio-mouse",
> >          .args = "-machine q35 -nodefaults -device virtio-mouse",
> >          .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
> > +        .objects = "virtio*",
> 
> I wonder about the use of "/tmp" rather than maybe some generated name
> using mkdtemp() - I also realise that the ability to generate this and
> plug it in here probably doesn't exist either, hence not holding you to
> it for this patch. Also the fact that in OSS-Fuzz this is run in limited
> containers.
> 
> Have you observed any changes to "/tmp" while this is running? My
> concerns may be unfounded since I don't really know what state things
> are in while this is executed with no running OS.
> 

So far, I haven't seen any files created, however this might be due to
the fact that I used security_model=none. In any case, I agree that this
is not a nice solution. I'll think of a way to use mkdtemp() for v2.
-Alex

> Thanks,
> 
> Darren.
> 

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Alexander Bulekov]

On 210115 1323, Greg Kurz wrote:
> On Thu, 14 Jan 2021 17:17:48 -0500
> Alexander Bulekov <alxndr@bu.edu> wrote:
> 
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> 
> No changelog at all ? 
> 
> >  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
> > index 7fed035345..ffdb590c58 100644
> > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
> >          .name = "virtio-mouse",
> >          .args = "-machine q35 -nodefaults -device virtio-mouse",
> >          .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
> 
> Sharing a general purpose directory like "/tmp" is definitely not a
> recommended practice. This is typically the kind of thing that I'd
> like to see documented in the changelog to help me understand ;-)

Hi Greg,
Yes it is not a great solution. The fuzzers in this file are mainly
configured to run on OSS-Fuzz (https://github.com/google/oss-fuzz),
where fuzzers are executed in individual containers, and there shouldn't
be anything sensitive in /tmp/. In v2, I'll use a safer solution.

> 
> What operations does the fuzz test perform on the device ?

The generic-fuzzer will interact with the Port IO/MMIO and PCI Config
Space regions associated with the virtio-9p device. When the
device tries to access some guest memory using DMA, the fuzzer will
place some fuzzed data at the corresponding location. For many devices,
this is sufficient to achieve high coverage. If this doesn't work well
for the virtio-9p, we can add a tailored fuzzer based on the libqos
interface, in the future.

> 
> > +        .objects = "virtio*",
> > +    },{
> > +        .name = "virtio-9p-synth",
> > +        .args = "-machine q35 -nodefaults "
> > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > +        "-fsdev synth,id=hshare",
> > +        .objects = "virtio*",
> 
> Not sure this is super useful since the only known use case for
> the synth fsdev driver is running the virtio-9p qtest, but
> it looks fine anyway.

My hope here was is that this configureation will cut down on syscalls
(improve fuzzing speed) and avoid leaky state (files left in the /tmp/
directory between individual fuzzer inputs).
-Alex

> 
> >      },{
> >          .name = "e1000",
> >          .args = "-M q35 -nodefaults "
> 

Re: [PATCH] fuzz: Add virtio-9p configurations for fuzzing

[Alexander Bulekov]

On 210115 1351, Christian Schoenebeck wrote:
> On Freitag, 15. Januar 2021 13:23:08 CET Greg Kurz wrote:
> > On Thu, 14 Jan 2021 17:17:48 -0500
> > 
> > Alexander Bulekov <alxndr@bu.edu> wrote:
> > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > > ---
> > 
> > No changelog at all ?
> 
> Yeah, that's indeed quite short. :)
> 
> > >  tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> > > b/tests/qtest/fuzz/generic_fuzz_configs.h index 7fed035345..ffdb590c58
> > > 100644
> > > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > > @@ -59,6 +59,18 @@ const generic_fuzz_config predefined_configs[] = {
> > > 
> > >          .name = "virtio-mouse",
> > >          .args = "-machine q35 -nodefaults -device virtio-mouse",
> > >          .objects = "virtio*",
> > > 
> > > +    },{
> > > +        .name = "virtio-9p",
> > > +        .args = "-machine q35 -nodefaults "
> > > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > > +        "-fsdev local,id=hshare,path=/tmp/,security_model=none",
> > 
> > Sharing a general purpose directory like "/tmp" is definitely not a
> > recommended practice. This is typically the kind of thing that I'd
> > like to see documented in the changelog to help me understand ;-)
> 
> For the (non fuzzing) 9p 'local' tests Peter wanted either an auto generated 
> (e.g. mkdtemp()) or at least a hard coded test path that allows a person to 
> easily identify where the data came from. So I guess that applies to fuzzing 
> as well, i.e. something like "/tmp/qemu-fuzz/9pfs/" at least.
> 
> Also note that the existing (non fuzzing) 9p 'local' tests auto generate  
> individual test pathes with mkdtemp() already. This was necessary there, 
> because tests are often run by "make -jN ..." in which case tests were 
> accessing concurrently the same single test directory before. Probably less of 
> a problem here, but you might consider using the same approach that
> virtio-9p-test.c already does.
> 
> Also note that 'security_model=none' is maybe Ok as a starting point for 
> fuzzing, but a realistic 9p config is rather 'security_model=xattr', because 
> 'security_model=none' requires the qemu process to be run as root to avoid 
> permission denied errors with any minor operation. I would expect these 
> fuzzing tests to mostly error out with permission denied errors early instead 
> of entering relevant execution pathes.
> 

Ah. That's good to know. I just copied the security_model from the bug
report for the recent CVE (https://bugs.launchpad.net/qemu/+bug/1911666)
I'll switch to mapped-xattr, in v2
-Alex

> > What operations does the fuzz test perform on the device ?
> > 
> > > +        .objects = "virtio*",
> > > +    },{
> > > +        .name = "virtio-9p-synth",
> > > +        .args = "-machine q35 -nodefaults "
> > > +        "-device virtio-9p,fsdev=hshare,mount_tag=hshare "
> > > +        "-fsdev synth,id=hshare",
> > > +        .objects = "virtio*",
> > 
> > Not sure this is super useful since the only known use case for
> > the synth fsdev driver is running the virtio-9p qtest, but
> > it looks fine anyway.
> 
> Yeah, that's ok. Maybe it raises the chance to enter some execution pathes 
> here and there. So I would keep the 'synth' driver config.
> 
> > 
> > >      },{
> > >      
> > >          .name = "e1000",
> > >          .args = "-M q35 -nodefaults "
> 
> Nice to see fuzzing coming for 9p BTW!
> 
> Best regards,
> Christian Schoenebeck
> 
>