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

[Christian Schoenebeck]

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