| Message ID | 20210114221748.503565-1-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuzz: Add virtio-9p configurations for fuzzing | expand |
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.
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 "
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
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. >
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 " >
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 > >
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 "
Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- tests/qtest/fuzz/generic_fuzz_configs.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)