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