From 7cee363bc2eff06068db0dc3e59cbc5f1906067e Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Fri, 17 Jul 2020 08:57:42 +0200 Subject: [PATCH 01/10] scripts/oss-fuzz: Limit target list to i386-softmmu The build.sh script only copies qemu-fuzz-i386 to the destination folder, so we can speed up the compilation step quite a bit by not compiling the other targets here. Signed-off-by: Thomas Huth --- scripts/oss-fuzz/build.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh index f5cee3d67e..a07b3022e8 100755 --- a/scripts/oss-fuzz/build.sh +++ b/scripts/oss-fuzz/build.sh @@ -68,7 +68,7 @@ mkdir -p "$DEST_DIR/lib/" # Copy the shared libraries here # Build once to get the list of dynamic lib paths, and copy them over ../configure --disable-werror --cc="$CC" --cxx="$CXX" \ - --extra-cflags="$EXTRA_CFLAGS" + --extra-cflags="$EXTRA_CFLAGS" --target-list="i386-softmmu" if ! make CONFIG_FUZZ=y CFLAGS="$LIB_FUZZING_ENGINE" "-j$(nproc)" \ i386-softmmu/fuzz; then From bcbad8b05c7f9072cadd3d3ebef2992196b73801 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Fri, 17 Jul 2020 12:35:23 -0400 Subject: [PATCH 02/10] fuzz: Fix leak when assembling datadir path string We freed the string containing the final datadir path, but did not free the path to the executable's directory that we get from g_path_get_dirname(). Fix that. Reported-by: Thomas Huth Signed-off-by: Alexander Bulekov Message-Id: <20200717163523.1591-1-alxndr@bu.edu> Signed-off-by: Thomas Huth --- tests/qtest/fuzz/fuzz.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c index 6bc17ef313..031594a686 100644 --- a/tests/qtest/fuzz/fuzz.c +++ b/tests/qtest/fuzz/fuzz.c @@ -143,7 +143,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp) { char *target_name; - char *dir; + char *bindir, *datadir; bool serialize = false; /* Initialize qgraph and modules */ @@ -164,11 +164,13 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp) * location of the executable. Using this we add exec_dir/pc-bios to * the datadirs. */ - dir = g_build_filename(g_path_get_dirname(**argv), "pc-bios", NULL); - if (g_file_test(dir, G_FILE_TEST_IS_DIR)) { - qemu_add_data_dir(dir); + bindir = g_path_get_dirname(**argv); + datadir = g_build_filename(bindir, "pc-bios", NULL); + g_free(bindir); + if (g_file_test(datadir, G_FILE_TEST_IS_DIR)) { + qemu_add_data_dir(datadir); } - g_free(dir); + g_free(datadir); } else if (*argc > 1) { /* The target is specified as an argument */ target_name = (*argv)[1]; if (!strstr(target_name, "--fuzz-target=")) { From 48eac1019769ebc4647ba380a828c25d8014be37 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Thu, 16 Jul 2020 12:33:30 -0400 Subject: [PATCH 03/10] gitlab-ci.yml: Add oss-fuzz build tests This tries to build and run the fuzzers with the same build-script used by oss-fuzz. This doesn't guarantee that the builds on oss-fuzz will also succeed, since oss-fuzz provides its own compiler and fuzzer vars, but it can catch changes that are not compatible with the the ./scripts/oss-fuzz/build.sh script. The strange way of finding fuzzer binaries stems from the method used by oss-fuzz: https://github.com/google/oss-fuzz/blob/master/infra/base-images/base-runner/targets_list Signed-off-by: Alexander Bulekov Message-Id: <20200720073223.22945-1-thuth@redhat.com> [thuth: Tweak the "script" to make it work, exclude slirp test, etc.] Signed-off-by: Thomas Huth --- .gitlab-ci.yml | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 41597c3603..362e5ee755 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -164,22 +164,20 @@ build-clang: ppc-softmmu s390x-softmmu arm-linux-user MAKE_CHECK_ARGS: check -build-fuzzer: +build-oss-fuzz: <<: *native_build_job_definition variables: IMAGE: fedora script: - - mkdir build - - cd build - - ../configure --cc=clang --cxx=clang++ --enable-fuzzing - --enable-sanitizers --target-list=x86_64-softmmu - - make -j"$JOBS" all check-build x86_64-softmmu/fuzz - - make check - - for fuzzer in i440fx-qos-fork-fuzz i440fx-qos-noreset-fuzz - i440fx-qtest-reboot-fuzz virtio-scsi-flags-fuzz virtio-scsi-fuzz ; do - echo Testing ${fuzzer} ... ; - x86_64-softmmu/qemu-fuzz-x86_64 --fuzz-target=${fuzzer} -runs=1000 - || exit 1 ; + - mkdir build-oss-fuzz + - CC="clang" CXX="clang++" CFLAGS="-fsanitize=address" + ./scripts/oss-fuzz/build.sh + - for fuzzer in $(find ./build-oss-fuzz/DEST_DIR/ -executable -type f + | grep -v slirp); do + grep "LLVMFuzzerTestOneInput" ${fuzzer} > /dev/null 2>&1 || continue ; + echo Testing ${fuzzer} ... ; + ASAN_OPTIONS="fast_unwind_on_malloc=0" + "${fuzzer}" -runs=1000 -seed=1 || exit 1 ; done build-tci: From dd0162653c11de58331506beb8b3d85c8923149c Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Mon, 6 Jul 2020 15:55:31 -0400 Subject: [PATCH 04/10] fuzz: build without AddressSanitizer, by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We already have a nice --enable-sanitizers option to enable AddressSanitizer. There is no reason to duplicate and force this functionality in --enable-fuzzing. In the future, if more sanitizers are added to --enable-sanitizers, it might be impossible to build with both --enable-sanitizers and --enable-fuzzing, since not all sanitizers are compatible with libFuzzer. In that case, we could enable ASAN with --extra-cflags="-fsanitize=address" Signed-off-by: Alexander Bulekov Message-Id: <20200706195534.14962-2-alxndr@bu.edu> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth [thuth: Added missing $CFLAGS] Signed-off-by: Thomas Huth --- configure | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/configure b/configure index 33cee41f9c..4bd80ed507 100755 --- a/configure +++ b/configure @@ -6337,7 +6337,7 @@ fi # checks for fuzzer if test "$fuzzing" = "yes" ; then write_c_fuzzer_skeleton - if compile_prog "$CPU_CFLAGS -Werror -fsanitize=address,fuzzer" ""; then + if compile_prog "$CPU_CFLAGS -Werror -fsanitize=fuzzer" ""; then have_fuzzer=yes fi fi @@ -7893,11 +7893,11 @@ if test "$have_mlockall" = "yes" ; then fi if test "$fuzzing" = "yes" ; then if test "$have_fuzzer" = "yes"; then - FUZZ_LDFLAGS=" -fsanitize=address,fuzzer" - FUZZ_CFLAGS=" -fsanitize=address,fuzzer" - CFLAGS="$CFLAGS -fsanitize=address,fuzzer-no-link" + FUZZ_LDFLAGS=" -fsanitize=fuzzer" + FUZZ_CFLAGS=" -fsanitize=fuzzer" + CFLAGS="$CFLAGS -fsanitize=fuzzer-no-link" else - error_exit "Your compiler doesn't support -fsanitize=address,fuzzer" + error_exit "Your compiler doesn't support -fsanitize=fuzzer" exit 1 fi fi From ee16da12d7035bffb1c990c794de8fb1a96815d7 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Mon, 6 Jul 2020 15:55:32 -0400 Subject: [PATCH 05/10] docs/fuzz: describe building fuzzers with enable-sanitizers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Alexander Bulekov Message-Id: <20200706195534.14962-3-alxndr@bu.edu> Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Thomas Huth Signed-off-by: Thomas Huth --- docs/devel/fuzzing.txt | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt index db5641de74..12bf6aa0ca 100644 --- a/docs/devel/fuzzing.txt +++ b/docs/devel/fuzzing.txt @@ -23,9 +23,12 @@ AddressSanitizer mmaps ~20TB of memory, as part of its detection. This results in a large page-map, and a much slower fork(). To build the fuzzers, install a recent version of clang: -Configure with (substitute the clang binaries with the version you installed): +Configure with (substitute the clang binaries with the version you installed). +Here, enable-sanitizers, is optional but it allows us to reliably detect bugs +such as out-of-bounds accesses, use-after-frees, double-frees etc. - CC=clang-8 CXX=clang++-8 /path/to/configure --enable-fuzzing + CC=clang-8 CXX=clang++-8 /path/to/configure --enable-fuzzing \ + --enable-sanitizers Fuzz targets are built similarly to system/softmmu: From 19a91e4af86c578420e9fdfe2efdc3b3b3826222 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Mon, 6 Jul 2020 15:55:33 -0400 Subject: [PATCH 06/10] docs/fuzz: add information about useful libFuzzer flags Signed-off-by: Alexander Bulekov Message-Id: <20200706195534.14962-4-alxndr@bu.edu> Signed-off-by: Thomas Huth --- docs/devel/fuzzing.txt | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt index 12bf6aa0ca..6d18115239 100644 --- a/docs/devel/fuzzing.txt +++ b/docs/devel/fuzzing.txt @@ -48,6 +48,43 @@ Information about these is available by passing -help=1 Now the only thing left to do is wait for the fuzzer to trigger potential crashes. +== Useful libFuzzer flags == + +As mentioned above, libFuzzer accepts some arguments. Passing -help=1 will list +the available arguments. In particular, these arguments might be helpful: + +$CORPUS_DIR/ : Specify a directory as the last argument to libFuzzer. libFuzzer +stores each "interesting" input in this corpus directory. The next time you run +libFuzzer, it will read all of the inputs from the corpus, and continue fuzzing +from there. You can also specify multiple directories. libFuzzer loads existing +inputs from all specified directories, but will only write new ones to the +first one specified. + +-max_len=4096 : specify the maximum byte-length of the inputs libFuzzer will +generate. + +-close_fd_mask={1,2,3} : close, stderr, or both. Useful for targets that +trigger many debug/error messages, or create output on the serial console. + +-jobs=4 -workers=4 : These arguments configure libFuzzer to run 4 fuzzers in +parallel (4 fuzzing jobs in 4 worker processes). Alternatively, with only +-jobs=N, libFuzzer automatically spawns a number of workers less than or equal +to half the available CPU cores. Replace 4 with a number appropriate for your +machine. Make sure to specify a $CORPUS_DIR, which will allow the parallel +fuzzers to share information about the interesting inputs they find. + +-use_value_profile=1 : For each comparison operation, libFuzzer computes +(caller_pc&4095) | (popcnt(Arg1 ^ Arg2) << 12) and places this in the coverage +table. Useful for targets with "magic" constants. If Arg1 came from the fuzzer's +input and Arg2 is a magic constant, then each time the Hamming distance +between Arg1 and Arg2 decreases, libFuzzer adds the input to the corpus. + +-shrink=1 : Tries to make elements of the corpus "smaller". Might lead to +better coverage performance, depending on the target. + +Note that libFuzzer's exact behavior will depend on the version of +clang and libFuzzer used to build the device fuzzers. + == Adding a new fuzzer == Coverage over virtual devices can be improved by adding additional fuzzers. Fuzzers are kept in tests/qtest/fuzz/ and should be added to From 09a14f586c315b01411dc1ef1bfe99b034b302de Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Mon, 6 Jul 2020 15:55:34 -0400 Subject: [PATCH 07/10] docs/fuzz: add instructions for generating a coverage report Signed-off-by: Alexander Bulekov Message-Id: <20200706195534.14962-5-alxndr@bu.edu> [thuth: Replaced --enable-sanitizers with --enable-fuzzing] Signed-off-by: Thomas Huth --- docs/devel/fuzzing.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/devel/fuzzing.txt b/docs/devel/fuzzing.txt index 6d18115239..96d71c94d7 100644 --- a/docs/devel/fuzzing.txt +++ b/docs/devel/fuzzing.txt @@ -85,6 +85,25 @@ better coverage performance, depending on the target. Note that libFuzzer's exact behavior will depend on the version of clang and libFuzzer used to build the device fuzzers. +== Generating Coverage Reports == +Code coverage is a crucial metric for evaluating a fuzzer's performance. +libFuzzer's output provides a "cov: " column that provides a total number of +unique blocks/edges covered. To examine coverage on a line-by-line basis we +can use Clang coverage: + + 1. Configure libFuzzer to store a corpus of all interesting inputs (see + CORPUS_DIR above) + 2. ./configure the QEMU build with: + --enable-fuzzing \ + --extra-cflags="-fprofile-instr-generate -fcoverage-mapping" + 3. Re-run the fuzzer. Specify $CORPUS_DIR/* as an argument, telling libfuzzer + to execute all of the inputs in $CORPUS_DIR and exit. Once the process + exits, you should find a file, "default.profraw" in the working directory. + 4. Execute these commands to generate a detailed HTML coverage-report: + llvm-profdata merge -output=default.profdata default.profraw + llvm-cov show ./path/to/qemu-fuzz-i386 -instr-profile=default.profdata \ + --format html -output-dir=/path/to/output/report + == Adding a new fuzzer == Coverage over virtual devices can be improved by adding additional fuzzers. Fuzzers are kept in tests/qtest/fuzz/ and should be added to From 6184e5fb4221ec5dd6f0c27d05a8e575b81eb89b Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Tue, 21 Jul 2020 07:36:09 +0200 Subject: [PATCH 08/10] MAINTAINERS: Extend the device fuzzing section The file docs/devel/fuzzing.txt should be in this section, too, and add myself as a reviewer (since I often take the fuzzer patches through the qtest-next tree, I should be notified on patches, too). Message-Id: <20200721053926.17197-1-thuth@redhat.com> Signed-off-by: Thomas Huth --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 5e8616821a..3395abd4e1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2449,9 +2449,11 @@ M: Alexander Bulekov R: Paolo Bonzini R: Bandan Das R: Stefan Hajnoczi +R: Thomas Huth S: Maintained F: tests/qtest/fuzz/ F: scripts/oss-fuzz/ +F: docs/devel/fuzzing.txt Register API M: Alistair Francis From 2b0650205b71c2aa8bf6f877a8333ef25bf288b2 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jul 2020 16:04:39 +0200 Subject: [PATCH 09/10] msf2: Unbreak device-list-properties for "msf-soc" Watch this: $ qemu-system-aarch64 -M ast2600-evb -S -display none -qmp stdio {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 5}, "package": "v5.0.0-2464-g3a9163af4e"}, "capabilities": ["oob"]}} {"execute": "qmp_capabilities"} {"return": {}} {"execute": "device-list-properties", "arguments": {"typename": "msf2-soc"}} Unsupported NIC model: ftgmac100 armbru@dusky:~/work/images$ echo $? 1 This is what breaks "make check SPEED=slow". Root cause is m2sxxx_soc_initfn()'s messing with nd_table[] via qemu_check_nic_model(). That's wrong. We fixed the exact same bug for device "allwinner-a10" in commit 8aabc5437b "hw/arm/allwinner-a10: Do not use nd_table in instance_init function". Fix this instance the same way: move the offending code to m2sxxx_soc_realize(), where it's less wrong, and add a FIXME comment. Fixes: 05b7374a58 ("msf2: Add EMAC block to SmartFusion2 SoC") Signed-off-by: Markus Armbruster Message-Id: <20200715140440.3540942-2-armbru@redhat.com> Reviewed-by: Alistair Francis Reviewed-by: Thomas Huth Signed-off-by: Thomas Huth --- hw/arm/msf2-soc.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c index 16bb7c9916..33ea7df342 100644 --- a/hw/arm/msf2-soc.c +++ b/hw/arm/msf2-soc.c @@ -82,10 +82,6 @@ static void m2sxxx_soc_initfn(Object *obj) } object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC); - if (nd_table[0].used) { - qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC); - qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); - } } static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) @@ -187,6 +183,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) g_free(bus_name); } + /* FIXME use qdev NIC properties instead of nd_table[] */ + if (nd_table[0].used) { + qemu_check_nic_model(&nd_table[0], TYPE_MSS_EMAC); + qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); + } dev = DEVICE(&s->emac); object_property_set_link(OBJECT(&s->emac), "ahb-bus", OBJECT(get_system_memory()), &error_abort); From 7ad36e2e241bd924f774a1f9fb208c102da58e50 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 15 Jul 2020 16:04:40 +0200 Subject: [PATCH 10/10] hw: Mark nd_table[] misuse in realize methods FIXME nd_table[] contains NIC configuration for boards to pick up. Device code has no business looking there. Several devices do it anyway. Two of them already have a suitable FIXME comment: "allwinner-a10" and "msf2-soc". Copy it to the others: "allwinner-h3", "xlnx-versal", "xlnx,zynqmp", "sparc32-ledma", "riscv.sifive.u.soc". Signed-off-by: Markus Armbruster Message-Id: <20200715140440.3540942-3-armbru@redhat.com> Reviewed-by: Alistair Francis Reviewed-by: Niek Linnenbank Reviewed-by: Thomas Huth Signed-off-by: Thomas Huth --- hw/arm/allwinner-h3.c | 1 + hw/arm/xlnx-versal.c | 1 + hw/arm/xlnx-zynqmp.c | 1 + hw/dma/sparc32_dma.c | 1 + hw/riscv/sifive_u.c | 1 + 5 files changed, 5 insertions(+) diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c index 8e09468e86..ff92ded82c 100644 --- a/hw/arm/allwinner-h3.c +++ b/hw/arm/allwinner-h3.c @@ -358,6 +358,7 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp) "sd-bus"); /* EMAC */ + /* FIXME use qdev NIC properties instead of nd_table[] */ if (nd_table[0].used) { qemu_check_nic_model(&nd_table[0], TYPE_AW_SUN8I_EMAC); qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c index ead038b971..e3aa4bd1e5 100644 --- a/hw/arm/xlnx-versal.c +++ b/hw/arm/xlnx-versal.c @@ -160,6 +160,7 @@ static void versal_create_gems(Versal *s, qemu_irq *pic) object_initialize_child(OBJECT(s), name, &s->lpd.iou.gem[i], TYPE_CADENCE_GEM); dev = DEVICE(&s->lpd.iou.gem[i]); + /* FIXME use qdev NIC properties instead of nd_table[] */ if (nd->used) { qemu_check_nic_model(nd, "cadence_gem"); qdev_set_nic_properties(dev, nd); diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c index 772cfa3771..5855e5d5bf 100644 --- a/hw/arm/xlnx-zynqmp.c +++ b/hw/arm/xlnx-zynqmp.c @@ -455,6 +455,7 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp) for (i = 0; i < XLNX_ZYNQMP_NUM_GEMS; i++) { NICInfo *nd = &nd_table[i]; + /* FIXME use qdev NIC properties instead of nd_table[] */ if (nd->used) { qemu_check_nic_model(nd, TYPE_CADENCE_GEM); qdev_set_nic_properties(DEVICE(&s->gem[i]), nd); diff --git a/hw/dma/sparc32_dma.c b/hw/dma/sparc32_dma.c index 9459178866..bcd1626fbd 100644 --- a/hw/dma/sparc32_dma.c +++ b/hw/dma/sparc32_dma.c @@ -341,6 +341,7 @@ static void sparc32_ledma_device_realize(DeviceState *dev, Error **errp) DeviceState *d; NICInfo *nd = &nd_table[0]; + /* FIXME use qdev NIC properties instead of nd_table[] */ qemu_check_nic_model(nd, TYPE_LANCE); d = qdev_new(TYPE_LANCE); diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 19a976c9a6..e5682c38a9 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -714,6 +714,7 @@ static void sifive_u_soc_realize(DeviceState *dev, Error **errp) } sysbus_mmio_map(SYS_BUS_DEVICE(&s->otp), 0, memmap[SIFIVE_U_OTP].base); + /* FIXME use qdev NIC properties instead of nd_table[] */ if (nd->used) { qemu_check_nic_model(nd, TYPE_CADENCE_GEM); qdev_set_nic_properties(DEVICE(&s->gem), nd);