Re: [PATCH 07/13] qtest/fuzz: make range overlap check more readable
On 240722 0007, Yao Xingtao wrote:
> use ranges_overlap() instead of open-coding the overlap check to improve
> the readability of the code.
>
> Signed-off-by: Yao Xingtao
Reviewed-by: Alexander Bulekov
Thank you
> ---
> tests/qtest/fuzz/generic_fuzz.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index ec842e03c5e6..d107a496da63 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -11,6 +11,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/range.h"
>
> #include
>
> @@ -211,7 +212,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len,
> MemoryRegion *mr)
> i < dma_regions->len && (avoid_double_fetches || qtest_log_enabled);
> ++i) {
> region = g_array_index(dma_regions, address_range, i);
> -if (addr < region.addr + region.size && addr + len > region.addr) {
> +if (ranges_overlap(addr, len, region.addr, region.size)) {
> double_fetch = true;
> if (addr < region.addr
> && avoid_double_fetches) {
> --
> 2.41.0
>
Re: [PATCH] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi
This fixes the almost-immediate timeout issue for me on the
virtio_net_fuzz target, but I'm not sure why this works or if it is
fixing the right problem:
qtest_probe_child is designed to run from a libqtest process which
uses waitpid on the PID of the child (qemu) process (stored in
QTestState->qemu_pid) . With qemu-fuzz we do not have a separate
libqtest and qemu process:
(gdb) p s->qemu_pid
$1 = 0
So we are calling waitpid with pid = 0. From the man-page:
"0 meaning wait for any child process whose process group ID is equal to
that of the calling process at the time of the call to waitpid()."
And we are calling it with WNOHANG. So I would expect that this almost
always returns 0 unless some adjacent thread has changed state
(libfuzzer uses extra threads to manage timeouts).
I'm happy that the fuzzer works again, and am happy to leave a review,
but I would like to first understand what the behavior of
qtest_probe_child here is, since it isn't really designed to work with
the fuzzer.
On 240523 1328, Dmitry Frolov wrote:
> If QTestState was already CLOSED due to error, calling qtest_clock_step()
> afterwards makes no sense and only raises false-crash with message:
> "assertion timer != NULL failed".
>
> Signed-off-by: Dmitry Frolov
> ---
> tests/qtest/fuzz/virtio_net_fuzz.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c
> b/tests/qtest/fuzz/virtio_net_fuzz.c
> index e239875e3b..2f57a8ddd8 100644
> --- a/tests/qtest/fuzz/virtio_net_fuzz.c
> +++ b/tests/qtest/fuzz/virtio_net_fuzz.c
> @@ -81,6 +81,9 @@ static void virtio_net_fuzz_multi(QTestState *s,
> /* Run the main loop */
> qtest_clock_step(s, 100);
> flush_events(s);
> +if (!qtest_probe_child(s)) {
> +return;
> +}
>
> /* Wait on used descriptors */
> if (check_used && !vqa.rx) {
> --
> 2.43.0
>
Re: [PATCH] tests/qtest/fuzz: fix memleak in qos_fuzz.c
Reviewed-by: Alexander Bulekov
On 240521 1331, Dmitry Frolov wrote:
> Found with fuzzing for qemu-8.2, but also relevant for master
>
> Signed-off-by: Dmitry Frolov
> ---
> tests/qtest/fuzz/qos_fuzz.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c
> index b71e945c5f..d3839bf999 100644
> --- a/tests/qtest/fuzz/qos_fuzz.c
> +++ b/tests/qtest/fuzz/qos_fuzz.c
> @@ -180,6 +180,7 @@ static void walk_path(QOSGraphNode *orig_path, int len)
>
> fuzz_path_vec = path_vec;
> } else {
> +g_string_free(cmd_line, true);
> g_free(path_vec);
> }
>
> --
> 2.43.0
>
[PATCH] fuzz: disable leak-detection for oss-fuzz builds
When we are building for OSS-Fuzz, we want to ensure that the fuzzer
targets are actually created, regardless of leaks. Leaks will be
detected by the subsequent tests of the individual fuzz-targets.
Signed-off-by: Alexander Bulekov
---
scripts/oss-fuzz/build.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
index 5238f83343..7398298173 100755
--- a/scripts/oss-fuzz/build.sh
+++ b/scripts/oss-fuzz/build.sh
@@ -92,6 +92,7 @@ make install DESTDIR=$DEST_DIR/qemu-bundle
rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/bin
rm -rf $DEST_DIR/qemu-bundle/opt/qemu-oss-fuzz/libexec
+export ASAN_OPTIONS=detect_leaks=0
targets=$(./qemu-fuzz-i386 | grep generic-fuzz | awk '$1 ~ /\*/ {print $2}')
base_copy="$DEST_DIR/qemu-fuzz-i386-target-$(echo "$targets" | head -n 1)"
--
2.45.1
Re: [PATCH] fuzz: specify audiodev for usb-audio
On 240527 1007, Alexander Bulekov wrote:
> On 240527 0734, Thomas Huth wrote:
> > On 27/05/2024 06.07, Alexander Bulekov wrote:
> > > Fixes test-failure on Fedora 40 CI.
> > >
> > > Reported-by: Thomas Huth
> > > Signed-off-by: Alexander Bulekov
> > > ---
> > > tests/qtest/fuzz/generic_fuzz_configs.h | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> > > b/tests/qtest/fuzz/generic_fuzz_configs.h
> > > index 4d7c8ca4ec..ef0ad95712 100644
> > > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > > @@ -150,7 +150,8 @@ const generic_fuzz_config predefined_configs[] = {
> > > "-chardev null,id=cd0 -chardev null,id=cd1 "
> > > "-device usb-braille,chardev=cd0 -device usb-ccid -device
> > > usb-ccid "
> > > "-device usb-kbd -device usb-mouse -device
> > > usb-serial,chardev=cd1 "
> > > -"-device usb-tablet -device usb-wacom-tablet -device usb-audio",
> > > +"-device usb-tablet -device usb-wacom-tablet "
> > > +"-device usb-audio,audiodev=snd0 -audiodev none,id=snd0",
> > > .objects = "*usb* *uhci* *xhci*",
> > > }
> >
> > Reviewed-by: Thomas Huth
> >
> > The patch makes sense and I think we should include it, thanks! .. but I
> > still don't understand why the behavior of the fuzzing job was different
> > between Fedora 38 and 40, do you? Why does it complain about "no default
> > audio driver available" on F40 but works fine on F38, though both build jobs
> > include the Alsa, pulseaudio and OSS backends?
> >
>
> Yes that's strange, if the behavior is different.
> The full config here is:
>
> .args = "-machine q35 -nodefaults "
> "-drive file=null-co://,if=none,format=raw,id=disk0 "
> "-device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 "
> "-device usb-bot -device usb-storage,drive=disk0 "
> "-chardev null,id=cd0 -chardev null,id=cd1 "
> "-device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid "
> "-device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1 "
> "-device usb-tablet -device usb-wacom-tablet "
> "-device usb-audio"
>
> This contains "nodefaults", which should always be causing the error,
> since this change:
> c753bf479a ("audio: disable default backends if -audio/-audiodev is used")
>
> This adds audio to qemu_disable_default_devices, which should be called
> by -nodefaults. I wonder if for some reason the order in which the
> audiodev is configured and the default audiodev is disabled is different
> between builds.
>
It might be even simpler. The recent jobs on Fedora 38 don't seem to
ever even make it to the xhci test, because of a detected leak:
https://gitlab.com/qemu-project/qemu/-/jobs/6949417820#L4375
Re: [PATCH] fuzz: specify audiodev for usb-audio
On 240527 0734, Thomas Huth wrote:
> On 27/05/2024 06.07, Alexander Bulekov wrote:
> > Fixes test-failure on Fedora 40 CI.
> >
> > Reported-by: Thomas Huth
> > Signed-off-by: Alexander Bulekov
> > ---
> > tests/qtest/fuzz/generic_fuzz_configs.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
> > b/tests/qtest/fuzz/generic_fuzz_configs.h
> > index 4d7c8ca4ec..ef0ad95712 100644
> > --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> > +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> > @@ -150,7 +150,8 @@ const generic_fuzz_config predefined_configs[] = {
> > "-chardev null,id=cd0 -chardev null,id=cd1 "
> > "-device usb-braille,chardev=cd0 -device usb-ccid -device
> > usb-ccid "
> > "-device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
> > "
> > -"-device usb-tablet -device usb-wacom-tablet -device usb-audio",
> > +"-device usb-tablet -device usb-wacom-tablet "
> > +"-device usb-audio,audiodev=snd0 -audiodev none,id=snd0",
> > .objects = "*usb* *uhci* *xhci*",
> > }
>
> Reviewed-by: Thomas Huth
>
> The patch makes sense and I think we should include it, thanks! .. but I
> still don't understand why the behavior of the fuzzing job was different
> between Fedora 38 and 40, do you? Why does it complain about "no default
> audio driver available" on F40 but works fine on F38, though both build jobs
> include the Alsa, pulseaudio and OSS backends?
>
Yes that's strange, if the behavior is different.
The full config here is:
.args = "-machine q35 -nodefaults "
"-drive file=null-co://,if=none,format=raw,id=disk0 "
"-device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 "
"-device usb-bot -device usb-storage,drive=disk0 "
"-chardev null,id=cd0 -chardev null,id=cd1 "
"-device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid "
"-device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1 "
"-device usb-tablet -device usb-wacom-tablet "
"-device usb-audio"
This contains "nodefaults", which should always be causing the error,
since this change:
c753bf479a ("audio: disable default backends if -audio/-audiodev is used")
This adds audio to qemu_disable_default_devices, which should be called
by -nodefaults. I wonder if for some reason the order in which the
audiodev is configured and the default audiodev is disabled is different
between builds.
[PATCH] fuzz: specify audiodev for usb-audio
Fixes test-failure on Fedora 40 CI.
Reported-by: Thomas Huth
Signed-off-by: Alexander Bulekov
---
tests/qtest/fuzz/generic_fuzz_configs.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h
b/tests/qtest/fuzz/generic_fuzz_configs.h
index 4d7c8ca4ec..ef0ad95712 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -150,7 +150,8 @@ const generic_fuzz_config predefined_configs[] = {
"-chardev null,id=cd0 -chardev null,id=cd1 "
"-device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid "
"-device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1 "
-"-device usb-tablet -device usb-wacom-tablet -device usb-audio",
+"-device usb-tablet -device usb-wacom-tablet "
+"-device usb-audio,audiodev=snd0 -audiodev none,id=snd0",
.objects = "*usb* *uhci* *xhci*",
},{
.name = "pc-i440fx",
--
2.43.0
Re: qemu fuzz crash in virtio_net_queue_reset()
On 240321 2208, Vladimir Sementsov-Ogievskiy wrote: > On 21.03.24 18:01, Alexander Bulekov wrote: > > On 240320 0024, Vladimir Sementsov-Ogievskiy wrote: > > > Hi all! > > > > > > From fuzzing I've got a fuzz-data, which produces the following crash: > > > > > > qemu-fuzz-x86_64: ../hw/net/virtio-net.c:134: void > > > flush_or_purge_queued_packets(NetClientState *): Assertion > > > `!virtio_net_get_subqueue(nc)->async_tx.elem' failed. > > > ==2172308== ERROR: libFuzzer: deadly signal > > > #0 0x5bd8c748b5a1 in __sanitizer_print_stack_trace > > > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26f05a1) > > > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > #1 0x5bd8c73fde38 in fuzzer::PrintStackTrace() > > > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2662e38) > > > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > #2 0x5bd8c73e38b3 in fuzzer::Fuzzer::CrashCallback() > > > (/home/settlements/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26488b3) > > > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > #3 0x739eec84251f (/lib/x86_64-linux-gnu/libc.so.6+0x4251f) > > > (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e) > > > #4 0x739eec8969fb in __pthread_kill_implementation > > > nptl/./nptl/pthread_kill.c:43:17 > > > #5 0x739eec8969fb in __pthread_kill_internal > > > nptl/./nptl/pthread_kill.c:78:10 > > > #6 0x739eec8969fb in pthread_kill nptl/./nptl/pthread_kill.c:89:10 > > > #7 0x739eec842475 in gsignal signal/../sysdeps/posix/raise.c:26:13 > > > #8 0x739eec8287f2 in abort stdlib/./stdlib/abort.c:79:7 > > > #9 0x739eec82871a in __assert_fail_base assert/./assert/assert.c:92:3 > > > #10 0x739eec839e95 in __assert_fail assert/./assert/assert.c:101:3 > > > #11 0x5bd8c995d9e2 in flush_or_purge_queued_packets > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:134:5 > > > #12 0x5bd8c9918a5f in virtio_net_queue_reset > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:563:5 > > > #13 0x5bd8c9b724e5 in virtio_queue_reset > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio.c:2492:9 > > > #14 0x5bd8c8bcfb7c in virtio_pci_common_write > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio-pci.c:1372:13 > > > #15 0x5bd8c9e19cf3 in memory_region_write_accessor > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:492:5 > > > #16 0x5bd8c9e19631 in access_with_adjusted_size > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:554:18 > > > #17 0x5bd8c9e17f3c in memory_region_dispatch_write > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:1514:16 > > > #18 0x5bd8c9ea3bbe in flatview_write_continue > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2825:23 > > > #19 0x5bd8c9e91aab in flatview_write > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2867:12 > > > #20 0x5bd8c9e91568 in address_space_write > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2963:18 > > > #21 0x5bd8c74c8a90 in __wrap_qtest_writeq > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/qtest_wrappers.c:187:9 > > > #22 0x5bd8c74dc4da in op_write > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:487:13 > > > #23 0x5bd8c74d942e in generic_fuzz > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:714:17 > > > #24 0x5bd8c74c016e in LLVMFuzzerTestOneInput > > > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/fuzz.c:152:5 > > > #25 0x5bd8c73e4e43 in fuzzer::Fuzzer::ExecuteCallback(unsigned char > > > const*, unsigned long) > > > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2649e43) > > > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > #26 0x5bd8c73cebbf in fuzzer::RunOneTest(fuzzer::Fuzzer*, char > > > const*, unsigned long) > > > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2633bbf) > > > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > #27 0x5bd8c73d4916 in fuzzer::FuzzerDriver(int*, char***, int > > > (*)(unsigned char const*, unsigned lo
Re: qemu fuzz crash in virtio_net_queue_reset()
On 240320 0024, Vladimir Sementsov-Ogievskiy wrote: > Hi all! > > From fuzzing I've got a fuzz-data, which produces the following crash: > > qemu-fuzz-x86_64: ../hw/net/virtio-net.c:134: void > flush_or_purge_queued_packets(NetClientState *): Assertion > `!virtio_net_get_subqueue(nc)->async_tx.elem' failed. > ==2172308== ERROR: libFuzzer: deadly signal > #0 0x5bd8c748b5a1 in __sanitizer_print_stack_trace > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26f05a1) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #1 0x5bd8c73fde38 in fuzzer::PrintStackTrace() > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2662e38) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #2 0x5bd8c73e38b3 in fuzzer::Fuzzer::CrashCallback() > (/home/settlements/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x26488b3) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #3 0x739eec84251f (/lib/x86_64-linux-gnu/libc.so.6+0x4251f) (BuildId: > c289da5071a3399de893d2af81d6a30c62646e1e) > #4 0x739eec8969fb in __pthread_kill_implementation > nptl/./nptl/pthread_kill.c:43:17 > #5 0x739eec8969fb in __pthread_kill_internal > nptl/./nptl/pthread_kill.c:78:10 > #6 0x739eec8969fb in pthread_kill nptl/./nptl/pthread_kill.c:89:10 > #7 0x739eec842475 in gsignal signal/../sysdeps/posix/raise.c:26:13 > #8 0x739eec8287f2 in abort stdlib/./stdlib/abort.c:79:7 > #9 0x739eec82871a in __assert_fail_base assert/./assert/assert.c:92:3 > #10 0x739eec839e95 in __assert_fail assert/./assert/assert.c:101:3 > #11 0x5bd8c995d9e2 in flush_or_purge_queued_packets > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:134:5 > #12 0x5bd8c9918a5f in virtio_net_queue_reset > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/net/virtio-net.c:563:5 > #13 0x5bd8c9b724e5 in virtio_queue_reset > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio.c:2492:9 > #14 0x5bd8c8bcfb7c in virtio_pci_common_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../hw/virtio/virtio-pci.c:1372:13 > #15 0x5bd8c9e19cf3 in memory_region_write_accessor > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:492:5 > #16 0x5bd8c9e19631 in access_with_adjusted_size > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:554:18 > #17 0x5bd8c9e17f3c in memory_region_dispatch_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/memory.c:1514:16 > #18 0x5bd8c9ea3bbe in flatview_write_continue > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2825:23 > #19 0x5bd8c9e91aab in flatview_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2867:12 > #20 0x5bd8c9e91568 in address_space_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../softmmu/physmem.c:2963:18 > #21 0x5bd8c74c8a90 in __wrap_qtest_writeq > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/qtest_wrappers.c:187:9 > #22 0x5bd8c74dc4da in op_write > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:487:13 > #23 0x5bd8c74d942e in generic_fuzz > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/generic_fuzz.c:714:17 > #24 0x5bd8c74c016e in LLVMFuzzerTestOneInput > /home/vsementsov/work/src/qemu/yc7-fuzz/build/../tests/qtest/fuzz/fuzz.c:152:5 > #25 0x5bd8c73e4e43 in fuzzer::Fuzzer::ExecuteCallback(unsigned char > const*, unsigned long) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2649e43) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #26 0x5bd8c73cebbf in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, > unsigned long) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2633bbf) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #27 0x5bd8c73d4916 in fuzzer::FuzzerDriver(int*, char***, int > (*)(unsigned char const*, unsigned long)) > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2639916) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #28 0x5bd8c73fe732 in main > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x2663732) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > #29 0x739eec829d8f in __libc_start_call_main > csu/../sysdeps/nptl/libc_start_call_main.h:58:16 > #30 0x739eec829e3f in __libc_start_main csu/../csu/libc-start.c:392:3 > #31 0x5bd8c73c9484 in _start > (/home/vsementsov/work/src/qemu/yc7-fuzz/build/qemu-fuzz-x86_64+0x262e484) > (BuildId: b41827f440fd9feaa98c667dbdcc961abb2799ae) > > > Hello Vladimir, This looks like a similar crash. https://gitlab.com/qemu-project/qemu/-/issues/1451 That issue has a qtest reproducer that does not require a fuzzer to reproduce. The fuzzer should run fine under gdb. e.g. gdb ./qemu-fuzz-i386 r --fuzz-target=generic-fuzz-virtio-net-pci-slirp ~/generic-fuzz-virtio-net-pci-slirp.crash-7707e14adea6
Re: QEMU snapshotting
On 231115 1522, Brian Cain wrote: > Alexander, Bandan, Paolo, Stefan, Manuel, > > Hi, I'm Brian and I maintain the Hexagon arch for QEMU. Elia, a security > researcher at Qualcomm is exploring ways to fuzz some hexagon OS kernel with > QEMU and in particular leveraging snapshotting, inspired by your research and > more. I'm not an expert on the details, but I'd like to make an introduction > and see if there's an opportunity for us to learn from one another. Maybe we > can have a call to kick things off? > Hi Brian, Elia, Sounds interesting! Happy to hop on a call to discuss. Mornings (EST) tend to work best for me. -Alex > -Brian
Re: [PATCH 04/13] fuzz: Correct invalid mentions of 'softmmu' by 'system'
Reviewed-by: Alexander Bulekov
Thank you
On 231004 1106, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> tests/qtest/fuzz/fuzz.h | 4 ++--
> softmmu/memory.c| 2 +-
> tests/qtest/fuzz/fuzz.c | 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
> index 21d1362d65..7da0bc3d7e 100644
> --- a/tests/qtest/fuzz/fuzz.h
> +++ b/tests/qtest/fuzz/fuzz.h
> @@ -49,13 +49,13 @@ typedef struct FuzzTarget {
>
>
> /*
> - * Returns the arguments that are passed to qemu/softmmu init(). Freed by
> + * Returns the arguments that are passed to qemu/system init(). Freed by
> * the caller.
> */
> GString *(*get_init_cmdline)(struct FuzzTarget *);
>
> /*
> - * will run once, prior to running qemu/softmmu init.
> + * will run once, prior to running qemu/system init.
> * eg: set up shared-memory for communication with the child-process
> * Can be NULL
> */
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 234bd7b116..fa1c99f9ba 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3638,7 +3638,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> }
>
> /*
> - * Support softmmu builds with CONFIG_FUZZ using a weak symbol and a stub for
> + * Support system builds with CONFIG_FUZZ using a weak symbol and a stub for
> * the fuzz_dma_read_cb callback
> */
> #ifdef CONFIG_FUZZ
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index 3bedb81b32..9b9c9f9c36 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -207,7 +207,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char
> ***envp)
> fuzz_target->pre_vm_init();
> }
>
> -/* Run QEMU's softmmu main with the fuzz-target dependent arguments */
> +/* Run QEMU's system main with the fuzz-target dependent arguments */
> cmd_line = fuzz_target->get_init_cmdline(fuzz_target);
> g_string_append_printf(cmd_line, " %s -qtest /dev/null ",
> getenv("QTEST_LOG") ? "" : "-qtest-log none");
> --
> 2.41.0
>
Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters
On 230112 0412, Lev Kujawski wrote: > > John Snow writes: > > > On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski wrote: > >> > >> Always use the native CHS device parameters for the ATA commands READ > >> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited > >> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch > >> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS > >> command, 2022-07-07.) > >> > >> As stated by the ATA/ATAPI specification, "[t]he native maximum is the > >> highest address accepted by the device in the factory default > >> condition." Therefore this patch substitutes the native values in > >> drive_heads and drive_sectors before calling ide_set_sector(). > >> > >> One consequence of the prior behavior was that setting zero sectors > >> per track could lead to an FPE within ide_set_sector(). Thanks to > >> Alexander Bulekov for reporting this issue. > >> > >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243 > >> Signed-off-by: Lev Kujawski > > > > Does this need attention? > > > > --js > > > > Hi John, > > This patch needs to be merged to mitigate issue 1243, which is still > present within QEMU master as of aa96ab7c9d. > > Thanks, Lev > Ping. oss-fuzz re-discovered this bug.
Re: [PATCH] virtio-gpu: fix potential divide-by-zero regression
On 230704 1119, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Commit 9462ff4695aa0 ("virtio-gpu/win32: allocate shareable 2d
> resources/images") introduces a division, which can lead to crashes when
> "height" is 0.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1744
> Signed-off-by: Marc-André Lureau
Reviewed-by: Alexander Bulekov
Re: [PULL 22/33] virtio-gpu/win32: allocate shareable 2d resources/images
On 230627 1502, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau
>
> Allocate pixman bits for scanouts with qemu_win32_map_alloc() so we can
> set a shareable handle on the associated display surface.
>
> Note: when bits are provided to pixman_image_create_bits(), you must also give
> the rowstride (the argument is ignored when bits is NULL)
>
> Signed-off-by: Marc-André Lureau
> Message-Id: <20230606115658.677673-11-marcandre.lur...@redhat.com>
> ---
> include/hw/virtio/virtio-gpu.h | 3 +++
> hw/display/virtio-gpu.c| 46 +++---
> 2 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 2e28507efe..7a5f8056ea 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -48,6 +48,9 @@ struct virtio_gpu_simple_resource {
> unsigned int iov_cnt;
> uint32_t scanout_bitmask;
> pixman_image_t *image;
> +#ifdef WIN32
> +HANDLE handle;
> +#endif
> uint64_t hostmem;
>
> uint64_t blob_size;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 1f8a5b16c6..347e17d490 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -258,6 +258,16 @@ static uint32_t calc_image_hostmem(pixman_format_code_t
> pformat,
> return height * stride;
> }
>
> +#ifdef WIN32
> +static void
> +win32_pixman_image_destroy(pixman_image_t *image, void *data)
> +{
> +HANDLE handle = data;
> +
> +qemu_win32_map_free(pixman_image_get_data(image), handle, &error_warn);
> +}
> +#endif
> +
> static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
>struct virtio_gpu_ctrl_command
> *cmd)
> {
> @@ -304,12 +314,27 @@ static void virtio_gpu_resource_create_2d(VirtIOGPU *g,
>
> res->hostmem = calc_image_hostmem(pformat, c2d.width, c2d.height);
> if (res->hostmem + g->hostmem < g->conf_max_hostmem) {
> +void *bits = NULL;
> +#ifdef WIN32
> +bits = qemu_win32_map_alloc(res->hostmem, &res->handle, &error_warn);
> +if (!bits) {
> +goto end;
> +}
> +#endif
> res->image = pixman_image_create_bits(pformat,
>c2d.width,
>c2d.height,
> - NULL, 0);
> + bits, res->hostmem /
> c2d.height);
Hello,
This may lead to FPE when c2d.height is 0.
https://gitlab.com/qemu-project/qemu/-/issues/1744
-Alex
Re: [PATCH v2 09/26] tests/docker: add test-fuzz
On 230626 2259, Alex Bennée wrote:
> Running the fuzzer requires some hoop jumping and some problems only
> show up in containers. This basically replicates the build-oss-fuzz
> job from our CI so we can run in the same containers we use in CI.
>
> Signed-off-by: Alex Bennée
Reviewed-by: Alexander Bulekov
Thanks
> ---
> tests/docker/test-fuzz | 28
> 1 file changed, 28 insertions(+)
> create mode 100755 tests/docker/test-fuzz
>
> diff --git a/tests/docker/test-fuzz b/tests/docker/test-fuzz
> new file mode 100755
> index 00..7e506ae1f6
> --- /dev/null
> +++ b/tests/docker/test-fuzz
> @@ -0,0 +1,28 @@
> +#!/bin/bash -e
> +#
> +# Compile and check with oss-fuzz.
> +#
> +# Copyright (c) 2023 Linaro Ltd.
> +#
> +# Authors:
> +# Alex Bennée
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +. common.rc
> +
> +requires_binary clang
> +
> +# the build script runs out of $src so we need to copy across
> +cd "$BUILD_DIR"
> +cp -a $QEMU_SRC .
> +cd src
> +mkdir build-oss-fuzz
> +export LSAN_OPTIONS=suppressions=scripts/oss-fuzz/lsan_suppressions.txt
> +env CC="clang" CXX="clang++" CFLAGS="-fsanitize=address"
> ./scripts/oss-fuzz/build.sh
> +export ASAN_OPTIONS="fast_unwind_on_malloc=0"
> +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} ... ;
> +"${fuzzer}" -runs=1 -seed=1 || exit 1 ;
> +done
> --
> 2.39.2
>
Re: [PATCH v2 08/26] tests/qtests: clean-up and fix leak in generic_fuzz
On 230626 2259, Alex Bennée wrote:
> An update to the clang tooling detects more issues with the code
> including a memory leak from the g_string_new() allocation. Clean up
> the code with g_autoptr and use ARRAY_SIZE while we are at it.
>
> Signed-off-by: Alex Bennée
Reviewed-by: Alexander Bulekov
Thank you
> ---
> tests/qtest/fuzz/generic_fuzz.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index c525d22951..a4841181cc 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -954,17 +954,14 @@ static void register_generic_fuzz_targets(void)
> .crossover = generic_fuzz_crossover
> });
>
> -GString *name;
> +g_autoptr(GString) name = g_string_new("");
> const generic_fuzz_config *config;
>
> -for (int i = 0;
> - i < sizeof(predefined_configs) / sizeof(generic_fuzz_config);
> - i++) {
> +for (int i = 0; i < ARRAY_SIZE(predefined_configs); i++) {
> config = predefined_configs + i;
> -name = g_string_new("generic-fuzz");
> -g_string_append_printf(name, "-%s", config->name);
> +g_string_printf(name, "generic-fuzz-%s", config->name);
> fuzz_add_target(&(FuzzTarget){
> -.name = name->str,
> +.name = g_strdup(name->str),
> .description = "Predefined generic-fuzz config.",
> .get_init_cmdline = generic_fuzz_predefined_config_cmdline,
> .pre_fuzz = generic_pre_fuzz,
> --
> 2.39.2
>
Re: [PATCH] usb/dev-wacom: fix OOB write in usb_mouse_poll()
On 230329 0542, Alexander Bulekov wrote:
> On 230213 1841, Mauro Matteo Cascella wrote:
> > The guest can control the size of buf; an OOB write occurs when buf is 1 or
> > 2
> > bytes long. Only fill in the buffer as long as there is enough space, throw
> > away any data which doesn't fit.
> >
> > Signed-off-by: Mauro Matteo Cascella
>
> Tested-by: Alexander Bulekov
>
> Thanks
>
Ping. I don't think this made it in yet?
> > ---
> > hw/usb/dev-wacom.c | 20 +---
> > 1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> > index 7177c17f03..ca9e6aa82f 100644
> > --- a/hw/usb/dev-wacom.c
> > +++ b/hw/usb/dev-wacom.c
> > @@ -252,14 +252,20 @@ static int usb_mouse_poll(USBWacomState *s, uint8_t
> > *buf, int len)
> > if (s->buttons_state & MOUSE_EVENT_MBUTTON)
> > b |= 0x04;
> >
> > -buf[0] = b;
> > -buf[1] = dx;
> > -buf[2] = dy;
> > -l = 3;
> > -if (len >= 4) {
> > -buf[3] = dz;
> > -l = 4;
> > +l = 0;
> > +if (len > l) {
> > +buf[l++] = b;
> > }
> > +if (len > l) {
> > +buf[l++] = dx;
> > +}
> > +if (len > l) {
> > +buf[l++] = dy;
> > +}
> > +if (len > l) {
> > +buf[l++] = dz;
> > +}
> > +
> > return l;
> > }
> >
> > --
> > 2.39.1
> >
> >
Re: [PATCH v2 1/2] net: Provide MemReentrancyGuard * to qemu_new_nic()
On 230601 1218, Akihiko Odaki wrote: > Recently MemReentrancyGuard was added to DeviceState to record that the > device is engaging in I/O. The network device backend needs to update it > when delivering a packet to a device. > > In preparation for such a change, add MemReentrancyGuard * as a > parameter of qemu_new_nic(). > > Signed-off-by: Akihiko Odaki Reviewed-by: Alexander Bulekov One minor comment below. > --- > include/net/net.h | 1 + > hw/net/allwinner-sun8i-emac.c | 3 ++- > hw/net/allwinner_emac.c | 3 ++- > hw/net/cadence_gem.c | 3 ++- > hw/net/dp8393x.c | 3 ++- > hw/net/e1000.c| 3 ++- > hw/net/e1000e.c | 2 +- > hw/net/eepro100.c | 4 +++- > hw/net/etraxfs_eth.c | 3 ++- > hw/net/fsl_etsec/etsec.c | 3 ++- > hw/net/ftgmac100.c| 3 ++- > hw/net/i82596.c | 2 +- > hw/net/igb.c | 2 +- > hw/net/imx_fec.c | 2 +- > hw/net/lan9118.c | 3 ++- > hw/net/mcf_fec.c | 3 ++- > hw/net/mipsnet.c | 3 ++- > hw/net/msf2-emac.c| 3 ++- > hw/net/mv88w8618_eth.c| 3 ++- > hw/net/ne2000-isa.c | 3 ++- > hw/net/ne2000-pci.c | 3 ++- > hw/net/npcm7xx_emc.c | 3 ++- > hw/net/opencores_eth.c| 3 ++- > hw/net/pcnet.c| 3 ++- > hw/net/rocker/rocker_fp.c | 4 ++-- > hw/net/rtl8139.c | 3 ++- > hw/net/smc91c111.c| 3 ++- > hw/net/spapr_llan.c | 3 ++- > hw/net/stellaris_enet.c | 3 ++- > hw/net/sungem.c | 2 +- > hw/net/sunhme.c | 3 ++- > hw/net/tulip.c| 3 ++- > hw/net/virtio-net.c | 6 -- > hw/net/vmxnet3.c | 2 +- > hw/net/xen_nic.c | 4 ++-- > hw/net/xgmac.c| 3 ++- > hw/net/xilinx_axienet.c | 3 ++- > hw/net/xilinx_ethlite.c | 3 ++- > hw/usb/dev-network.c | 3 ++- > net/net.c | 1 + > 40 files changed, 75 insertions(+), 41 deletions(-) > > diff --git a/include/net/net.h b/include/net/net.h > index 1448d00afb..a7d8deaccb 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -157,6 +157,7 @@ NICState *qemu_new_nic(NetClientInfo *info, > NICConf *conf, > const char *model, > const char *name, > + MemReentrancyGuard *reentrancy_guard, > void *opaque); Does it make sense to roll *reentrancy_guard into NICConf here?
Re: [PATCH v2 2/2] net: Update MemReentrancyGuard for NIC
On 230601 1218, Akihiko Odaki wrote:
> Recently MemReentrancyGuard was added to DeviceState to record that the
> device is engaging in I/O. The network device backend needs to update it
> when delivering a packet to a device.
>
> This implementation follows what bottom half does, but it does not add
> a tracepoint for the case that the network device backend started
> delivering a packet to a device which is already engaging in I/O. This
> is because such reentrancy frequently happens for
> qemu_flush_queued_packets() and is insignificant.
>
> Fixes: CVE-2023-3019
> Reported-by: Alexander Bulekov
> Signed-off-by: Akihiko Odaki
Acked-by: Alexander Bulekov
> ---
> include/net/net.h | 1 +
> net/net.c | 14 ++
> 2 files changed, 15 insertions(+)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index a7d8deaccb..685ec58318 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -124,6 +124,7 @@ typedef QTAILQ_HEAD(NetClientStateList, NetClientState)
> NetClientStateList;
> typedef struct NICState {
> NetClientState *ncs;
> NICConf *conf;
> +MemReentrancyGuard *reentrancy_guard;
> void *opaque;
> bool peer_deleted;
> } NICState;
> diff --git a/net/net.c b/net/net.c
> index 982df2479f..3523cceafc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -332,6 +332,7 @@ NICState *qemu_new_nic(NetClientInfo *info,
> nic = g_malloc0(info->size + sizeof(NetClientState) * queues);
> nic->ncs = (void *)nic + info->size;
> nic->conf = conf;
> +nic->reentrancy_guard = reentrancy_guard,
> nic->opaque = opaque;
>
> for (i = 0; i < queues; i++) {
> @@ -805,6 +806,7 @@ static ssize_t qemu_deliver_packet_iov(NetClientState
> *sender,
> int iovcnt,
> void *opaque)
> {
> +MemReentrancyGuard *owned_reentrancy_guard;
> NetClientState *nc = opaque;
> int ret;
>
> @@ -817,12 +819,24 @@ static ssize_t qemu_deliver_packet_iov(NetClientState
> *sender,
> return 0;
> }
>
> +if (nc->info->type != NET_CLIENT_DRIVER_NIC ||
> +qemu_get_nic(nc)->reentrancy_guard->engaged_in_io) {
> +owned_reentrancy_guard = NULL;
> +} else {
> +owned_reentrancy_guard = qemu_get_nic(nc)->reentrancy_guard;
> +owned_reentrancy_guard->engaged_in_io = true;
> +}
> +
> if (nc->info->receive_iov && !(flags & QEMU_NET_PACKET_FLAG_RAW)) {
> ret = nc->info->receive_iov(nc, iov, iovcnt);
> } else {
> ret = nc_sendv_compat(nc, iov, iovcnt, flags);
> }
>
> +if (owned_reentrancy_guard) {
> +owned_reentrancy_guard->engaged_in_io = false;
> +}
> +
> if (ret == 0) {
> nc->receive_disabled = 1;
> }
> --
> 2.40.1
>
Re: [PATCH v2] hw/scsi/lsi53c895a: Fix reentrancy issues in the LSI controller (CVE-2023-0330)
On 230522 1110, Thomas Huth wrote: > We cannot use the generic reentrancy guard in the LSI code, so > we have to manually prevent endless reentrancy here. The problematic > lsi_execute_script() function has already a way to detect whether > too many instructions have been executed - we just have to slightly > change the logic here that it also takes into account if the function > has been called too often in a reentrant way. > > The code in fuzz-lsi53c895a-test.c has been taken from an earlier > patch by Mauro Matteo Cascella. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1563 > Signed-off-by: Thomas Huth Reviewed-by: Alexander Bulekov
Re: [PATCH] lsi53c895a: disable reentrancy detection for MMIO region, too
On 230516 1105, Thomas Huth wrote: > While trying to use a SCSI disk on the LSI controller with an > older version of Fedora (25), I'm getting: > > qemu: warning: Blocked re-entrant IO on MemoryRegion: lsi-mmio at addr: 0x34 Do you have a gdb backtrace for this one or is there some easy way to reproduce with just a LiveCD or something? Marking mmio_io re-entrancy safe would bring back https://gitlab.com/qemu-project/qemu/-/issues/1563 Maybe there is some other workaround here? -Alex > > and the SCSI controller is not usable. Seems like we have to > disable the reentrancy checker for the MMIO region, too, to > get this working again. > > Signed-off-by: Thomas Huth > --- > hw/scsi/lsi53c895a.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c > index db27872963..048436352b 100644 > --- a/hw/scsi/lsi53c895a.c > +++ b/hw/scsi/lsi53c895a.c > @@ -2307,6 +2307,7 @@ static void lsi_scsi_realize(PCIDevice *dev, Error > **errp) > * re-entrancy guard. > */ > s->ram_io.disable_reentrancy_guard = true; > +s->mmio_io.disable_reentrancy_guard = true; > > address_space_init(&s->pci_io_as, pci_address_space_io(dev), > "lsi-pci-io"); > qdev_init_gpio_out(d, &s->ext_irq, 1); > -- > 2.31.1 >
[PATCH] memory: stricter checks prior to unsetting engaged_in_io
engaged_in_io could be unset by an MR with re-entrancy checks disabled.
Ensure that only MRs that can set the engaged_in_io flag can unset it.
Closes: https://gitlab.com/qemu-project/qemu/-/issues/1563
Reported-by: Thomas Huth
Signed-off-by: Alexander Bulekov
---
softmmu/memory.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b7b3386e9d..26424f1d78 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -534,6 +534,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
unsigned access_size;
unsigned i;
MemTxResult r = MEMTX_OK;
+bool reentrancy_guard_applied = false;
if (!access_size_min) {
access_size_min = 1;
@@ -552,6 +553,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
return MEMTX_ACCESS_ERROR;
}
mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+reentrancy_guard_applied = true;
}
/* FIXME: support unaligned access? */
@@ -568,7 +570,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
-if (mr->dev) {
+if (mr->dev && reentrancy_guard_applied) {
mr->dev->mem_reentrancy_guard.engaged_in_io = false;
}
return r;
--
2.39.0
Re: [PATCH] pnv_lpc: disable reentrancy detection for lpc-hc
On 230511 1104, Cédric Le Goater wrote: > Hello Alexander > > On 5/11/23 10:53, Alexander Bulekov wrote: > > As lpc-hc is designed for re-entrant calls from xscom, mark it > > re-entrancy safe. > > > > Reported-by: Thomas Huth > > Signed-off-by: Alexander Bulekov > > --- > > hw/ppc/pnv_lpc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c > > index 01f44c19eb..67fd049a7f 100644 > > --- a/hw/ppc/pnv_lpc.c > > +++ b/hw/ppc/pnv_lpc.c > > @@ -738,6 +738,8 @@ static void pnv_lpc_realize(DeviceState *dev, Error > > **errp) > > &lpc->opb_master_regs); > > memory_region_init_io(&lpc->lpc_hc_regs, OBJECT(dev), &lpc_hc_ops, > > lpc, > > "lpc-hc", LPC_HC_REGS_OPB_SIZE); > > +/* xscom writes to lpc-hc. As such mark lpc-hc re-entrancy safe */ > > +lpc->lpc_hc_regs.disable_reentrancy_guard = true; > > memory_region_add_subregion(&lpc->opb_mr, LPC_HC_REGS_OPB_ADDR, > > &lpc->lpc_hc_regs); > > The warning changed : > > qemu-system-ppc64: warning: Blocked re-entrant IO on MemoryRegion: > lpc-opb-master at addr: 0x8 > > I will take a look unless you know exactly what to do. > That does not show up for me with "./qemu-system-ppc64 -M powernv8" Do I need to boot a kernel to see the message? I was worried that there might be other re-entrant IO in this device. Maybe there should be a way to just mark the whole device re-entrancy safe.
[PATCH] pnv_lpc: disable reentrancy detection for lpc-hc
As lpc-hc is designed for re-entrant calls from xscom, mark it re-entrancy safe. Reported-by: Thomas Huth Signed-off-by: Alexander Bulekov --- hw/ppc/pnv_lpc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c index 01f44c19eb..67fd049a7f 100644 --- a/hw/ppc/pnv_lpc.c +++ b/hw/ppc/pnv_lpc.c @@ -738,6 +738,8 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp) &lpc->opb_master_regs); memory_region_init_io(&lpc->lpc_hc_regs, OBJECT(dev), &lpc_hc_ops, lpc, "lpc-hc", LPC_HC_REGS_OPB_SIZE); +/* xscom writes to lpc-hc. As such mark lpc-hc re-entrancy safe */ +lpc->lpc_hc_regs.disable_reentrancy_guard = true; memory_region_add_subregion(&lpc->opb_mr, LPC_HC_REGS_OPB_ADDR, &lpc->lpc_hc_regs); -- 2.39.0
[PATCH] loongarch: mark loongarch_ipi_iocsr re-entrnacy safe
loongarch_ipi_iocsr MRs rely on re-entrant IO through the ipi_send
function. As such, mark these MRs re-entrancy-safe.
Fixes: a2e1753b80 ("memory: prevent dma-reentracy issues")
Signed-off-by: Alexander Bulekov
---
hw/intc/loongarch_ipi.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
index bdba0f8107..9de7c01e11 100644
--- a/hw/intc/loongarch_ipi.c
+++ b/hw/intc/loongarch_ipi.c
@@ -215,6 +215,10 @@ static void loongarch_ipi_init(Object *obj)
for (cpu = 0; cpu < MAX_IPI_CORE_NUM; cpu++) {
memory_region_init_io(&s->ipi_iocsr_mem[cpu], obj, &loongarch_ipi_ops,
&lams->ipi_core[cpu], "loongarch_ipi_iocsr", 0x48);
+
+/* loongarch_ipi_iocsr performs re-entrant IO through ipi_send */
+s->ipi_iocsr_mem[cpu].disable_reentrancy_guard = true;
+
sysbus_init_mmio(sbd, &s->ipi_iocsr_mem[cpu]);
memory_region_init_io(&s->ipi64_iocsr_mem[cpu], obj,
&loongarch_ipi64_ops,
--
2.39.0
[PATCH] async: avoid use-after-free on re-entrancy guard
A BH callback can free the BH, causing a use-after-free in aio_bh_call.
Fix that by keeping a local copy of the re-entrancy guard pointer.
Buglink: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58513
Fixes: 9c86c97f12 ("async: Add an optional reentrancy guard to the BH API")
Signed-off-by: Alexander Bulekov
---
util/async.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/util/async.c b/util/async.c
index 9df7674b4e..055070ffbd 100644
--- a/util/async.c
+++ b/util/async.c
@@ -156,18 +156,20 @@ void aio_bh_call(QEMUBH *bh)
{
bool last_engaged_in_io = false;
-if (bh->reentrancy_guard) {
-last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
-if (bh->reentrancy_guard->engaged_in_io) {
+/* Make a copy of the guard-pointer as cb may free the bh */
+MemReentrancyGuard *reentrancy_guard = bh->reentrancy_guard;
+if (reentrancy_guard) {
+last_engaged_in_io = reentrancy_guard->engaged_in_io;
+if (reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
}
-bh->reentrancy_guard->engaged_in_io = true;
+reentrancy_guard->engaged_in_io = true;
}
bh->cb(bh->opaque);
-if (bh->reentrancy_guard) {
-bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
+if (reentrancy_guard) {
+reentrancy_guard->engaged_in_io = last_engaged_in_io;
}
}
--
2.39.0
Re: [PULL 07/13] async: Add an optional reentrancy guard to the BH API
On 230428 1143, Thomas Huth wrote:
> From: Alexander Bulekov
>
> Devices can pass their MemoryReentrancyGuard (from their DeviceState),
> when creating new BHes. Then, the async API will toggle the guard
> before/after calling the BH call-back. This prevents bh->mmio reentrancy
> issues.
>
> Signed-off-by: Alexander Bulekov
> Reviewed-by: Darren Kenny
> Message-Id: <20230427211013.2994127-3-alx...@bu.edu>
> [thuth: Fix "line over 90 characters" checkpatch.pl error]
> Signed-off-by: Thomas Huth
> ---
> void aio_bh_call(QEMUBH *bh)
> {
> +bool last_engaged_in_io = false;
> +
> +if (bh->reentrancy_guard) {
> +last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
> +if (bh->reentrancy_guard->engaged_in_io) {
> +trace_reentrant_aio(bh->ctx, bh->name);
> +}
> +bh->reentrancy_guard->engaged_in_io = true;
> +}
> +
> bh->cb(bh->opaque);
> +
> +if (bh->reentrancy_guard) {
> +bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
> +}
This causes a UAF if bh was freed in bh->cb().
OSS-Fuzz reported this as issue 58513.
==3433535==ERROR: AddressSanitizer: heap-use-after-free on address
0x606427d0 at pc 0x565542b09347 bp 0x7fff2a4cf590 sp 0x7fff2a4cf588
READ of size 8 at 0x606427d0 thread T0
#0 0x565542b09346 in aio_bh_call /../util/async.c:169:19
#1 0x565542b0a2cc in aio_bh_poll /../util/async.c:200:13
#2 0x565542a6a818 in aio_dispatch /../util/aio-posix.c:421:5
#3 0x565542b1156e in aio_ctx_dispatch /../util/async.c:342:5
#4 0x7fc66e3657a8 in g_main_context_dispatch
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x547a8) (BuildId:
77a560369e4633278bc6e75ab0587491e11d5aac)
#5 0x565542b153f9 in glib_pollfds_poll /../util/main-loop.c:290:9
#6 0x565542b13cb3 in os_host_main_loop_wait /../util/main-loop.c:313:5
#7 0x565542b1387c in main_loop_wait /../util/main-loop.c:592:11
0x606427d0 is located 48 bytes inside of 56-byte region
[0x606427a0,0x606427d8)
freed by thread T0 here:
#0 0x56553eff2192 in __interceptor_free (Id:
ba9d8c3e3344b6323a2db18d4ab0bb9948201520)
#1 0x565542b0a32f in aio_bh_poll /../util/async.c:203:13
#2 0x565542a6ed7c in aio_poll /../util/aio-posix.c:721:17
#3 0x565542380b4d in bdrv_aio_cancel /../block/io.c:2812:13
#4 0x56554231aeda in blk_aio_cancel /../block/block-backend.c:1702:5
#5 0x56553f8fc242 in ahci_reset_port /../hw/ide/ahci.c:678:13
#6 0x56553f91d073 in handle_reg_h2d_fis /../hw/ide/ahci.c:1218:17
#7 0x56553f91a6c5 in handle_cmd /../hw/ide/ahci.c:1323:13
#8 0x56553f90fb13 in check_cmd /../hw/ide/ahci.c:595:18
#9 0x56553f944b8d in ahci_check_cmd_bh /../hw/ide/ahci.c:609:5
#10 0x565542b0929c in aio_bh_call /../util/async.c:167:5
#11 0x565542b0a2cc in aio_bh_poll /../util/async.c:200:13
#12 0x565542a6a818 in aio_dispatch /../util/aio-posix.c:421:5
#13 0x565542b1156e in aio_ctx_dispatch /../util/async.c:342:5
#14 0x7fc66e3657a8 in g_main_context_dispatch
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x547a8)
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
On 230428 1015, Thomas Huth wrote:
> On 28/04/2023 10.12, Daniel P. Berrangé wrote:
> > On Thu, Apr 27, 2023 at 05:10:06PM -0400, Alexander Bulekov wrote:
> > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > > This flag is set/checked prior to calling a device's MemoryRegion
> > > handlers, and set when device code initiates DMA. The purpose of this
> > > flag is to prevent two types of DMA-based reentrancy issues:
> > >
> > > 1.) mmio -> dma -> mmio case
> > > 2.) bh -> dma write -> mmio case
> > >
> > > These issues have led to problems such as stack-exhaustion and
> > > use-after-frees.
> > >
> > > Summary of the problem from Peter Maydell:
> > > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
> > >
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> > > Resolves: CVE-2023-0330
> > >
> > > Signed-off-by: Alexander Bulekov
> > > Reviewed-by: Thomas Huth
> > > ---
> > > include/exec/memory.h | 5 +
> > > include/hw/qdev-core.h | 7 +++
> > > softmmu/memory.c | 16
> > > 3 files changed, 28 insertions(+)
> > >
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index 15ade918ba..e45ce6061f 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -767,6 +767,8 @@ struct MemoryRegion {
> > > bool is_iommu;
> > > RAMBlock *ram_block;
> > > Object *owner;
> > > +/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access
> > > hotpath */
> > > +DeviceState *dev;
> > > const MemoryRegionOps *ops;
> > > void *opaque;
> > > @@ -791,6 +793,9 @@ struct MemoryRegion {
> > > unsigned ioeventfd_nb;
> > > MemoryRegionIoeventfd *ioeventfds;
> > > RamDiscardManager *rdm; /* Only for RAM */
> > > +
> > > +/* For devices designed to perform re-entrant IO into their own IO
> > > MRs */
> > > +bool disable_reentrancy_guard;
> > > };
> > > struct IOMMUMemoryRegion {
> > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > > index bd50ad5ee1..7623703943 100644
> > > --- a/include/hw/qdev-core.h
> > > +++ b/include/hw/qdev-core.h
> > > @@ -162,6 +162,10 @@ struct NamedClockList {
> > > QLIST_ENTRY(NamedClockList) node;
> > > };
> > > +typedef struct {
> > > +bool engaged_in_io;
> > > +} MemReentrancyGuard;
> > > +
> > > /**
> > >* DeviceState:
> > >* @realized: Indicates whether the device has been fully constructed.
> > > @@ -194,6 +198,9 @@ struct DeviceState {
> > > int alias_required_for_version;
> > > ResettableState reset;
> > > GSList *unplug_blockers;
> > > +
> > > +/* Is the device currently in mmio/pio/dma? Used to prevent
> > > re-entrancy */
> > > +MemReentrancyGuard mem_reentrancy_guard;
> > > };
> > > struct DeviceListener {
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index b1a6cae6f5..fe23f0e5ce 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr
> > > addr,
> > > access_size_max = 4;
> > > }
> > > +/* Do not allow more than one simultaneous access to a device's IO
> > > Regions */
> > > +if (mr->dev && !mr->disable_reentrancy_guard &&
> > > +!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly)
> > > {
> > > +if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
> > > +warn_report("Blocked re-entrant IO on "
> > > +"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
> > > +memory_region_name(mr), addr);
> > > +return MEMTX_ACCESS_ERROR;
> >
> > If we issue this warn_report on every invalid memory access, is this
> > going to become a denial of service by flooding logs, or is the
> > return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
> > *once* in the lifetime of the QEMU process ?
>
> Maybe it's better to use warn_report_once() here instead?
Sounds good - should I respin the series to change this?
-Alex
[PATCH v10 7/8] raven: disable reentrancy detection for iomem
As the code is designed for re-entrant calls from raven_io_ops to pci-conf, mark raven_io_ops as reentrancy-safe. Signed-off-by: Alexander Bulekov --- hw/pci-host/raven.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c index 072ffe3c5e..9a11ac4b2b 100644 --- a/hw/pci-host/raven.c +++ b/hw/pci-host/raven.c @@ -294,6 +294,13 @@ static void raven_pcihost_initfn(Object *obj) memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f00); address_space_init(&s->pci_io_as, &s->pci_io, "raven-io"); +/* + * Raven's raven_io_ops use the address-space API to access pci-conf-idx + * (which is also owned by the raven device). As such, mark the + * pci_io_non_contiguous as re-entrancy safe. + */ +s->pci_io_non_contiguous.disable_reentrancy_guard = true; + /* CPU address space */ memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR, &s->pci_io); -- 2.39.0
[PATCH v10 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks
Advise authors to use the _guarded versions of the APIs, instead.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers;
signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of
qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new()
to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc.
instead of module_init()\n" . $herecurr);
--
2.39.0
[PATCH v10 6/8] bcm2835_property: disable reentrancy detection for iomem
As the code is designed for re-entrant calls from bcm2835_property to bcm2835_mbox and back into bcm2835_property, mark iomem as reentrancy-safe. Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth --- hw/misc/bcm2835_property.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 890ae7bae5..de056ea2df 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -382,6 +382,13 @@ static void bcm2835_property_init(Object *obj) memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_property_ops, s, TYPE_BCM2835_PROPERTY, 0x10); + +/* + * bcm2835_property_ops call into bcm2835_mbox, which in-turn reads from + * iomem. As such, mark iomem as re-entracy safe. + */ +s->iomem.disable_reentrancy_guard = true; + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(s), &s->mbox_irq); } -- 2.39.0
[PATCH v10 0/8] memory: prevent dma-reentracy issues
v8-> v9: - Replace trace-events and attempt at making re-entrancy fatal with a warn_report message. This message should only be printed if a device is broken (and needs to be marked re-entrancy safe), or if something in the guest is attempting to trigger unintentional re-entrancy. - Added APIC change to the series v7 -> v8: - Disable reentrancy checks for bcm2835_property's iomem (Patch 7) - Cache DeviceState* in the MemoryRegion to avoid dynamic cast for each MemoryRegion access. (Patch 1) - Make re-entrancy fatal for debug-builds (Patch 8) v6 -> v7: - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4) - Add an MR-specific flag to disable reentrancy (Patch 5) - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6) Patches 5 and 6 need review. I left the review-tags for Patch 4, however a few of the qemu_bh_new_guarded calls have changed. v5 -> v6: - Only apply checkpatch checks to code in paths containing "/hw/" (/hw/ and include/hw/) - Fix a bug in a _guarded call added to hw/block/virtio-blk.c v4-> v5: - Add corresponding checkpatch checks - Save/restore reentrancy-flag when entering/exiting BHs - Improve documentation - Check object_dynamic_cast return value v3 -> v4: Instead of changing all of the DMA APIs, instead add an optional reentrancy guard to the BH API. v2 -> v3: Bite the bullet and modify the DMA APIs, rather than attempting to guess DeviceStates in BHs. These patches aim to solve two types of DMA-reentrancy issues: 1.) mmio -> dma -> mmio case To solve this, we track whether the device is engaged in io by checking/setting a reentrancy-guard within APIs used for MMIO access. 2.) bh -> dma write -> mmio case This case is trickier, since we dont have a generic way to associate a bh with the underlying Device/DeviceState. Thus, this version allows a device to associate a reentrancy-guard with a bh, when creating it. (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded) I replaced most of the qemu_bh_new invocations with the guarded analog, except for the ones where the DeviceState was not trivially accessible. Alexander Bulekov (8): memory: prevent dma-reentracy issues async: Add an optional reentrancy guard to the BH API checkpatch: add qemu_bh_new/aio_bh_new checks hw: replace most qemu_bh_new calls with qemu_bh_new_guarded lsi53c895a: disable reentrancy detection for script RAM bcm2835_property: disable reentrancy detection for iomem raven: disable reentrancy detection for iomem apic: disable reentrancy detection for apic-msi docs/devel/multiple-iothreads.txt | 7 +++ hw/9pfs/xen-9p-backend.c | 5 - hw/block/dataplane/virtio-blk.c | 3 ++- hw/block/dataplane/xen-block.c| 5 +++-- hw/char/virtio-serial-bus.c | 3 ++- hw/display/qxl.c | 9 ++--- hw/display/virtio-gpu.c | 6 -- hw/ide/ahci.c | 3 ++- hw/ide/ahci_internal.h| 1 + hw/ide/core.c | 4 +++- hw/intc/apic.c| 7 +++ hw/misc/bcm2835_property.c| 7 +++ hw/misc/imx_rngc.c| 6 -- hw/misc/macio/mac_dbdma.c | 2 +- hw/net/virtio-net.c | 3 ++- hw/nvme/ctrl.c| 6 -- hw/pci-host/raven.c | 7 +++ hw/scsi/lsi53c895a.c | 6 ++ hw/scsi/mptsas.c | 3 ++- hw/scsi/scsi-bus.c| 3 ++- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/dev-uas.c | 3 ++- hw/usb/hcd-dwc2.c | 3 ++- hw/usb/hcd-ehci.c | 3 ++- hw/usb/hcd-uhci.c | 2 +- hw/usb/host-libusb.c | 6 -- hw/usb/redirect.c | 6 -- hw/usb/xen-usb.c | 3 ++- hw/virtio/virtio-balloon.c| 5 +++-- hw/virtio/virtio-crypto.c | 3 ++- include/block/aio.h | 18 -- include/exec/memory.h | 5 + include/hw/qdev-core.h| 7 +++ include/qemu/main-loop.h | 7 +-- scripts/checkpatch.pl | 8 softmmu/memory.c | 16 tests/unit/ptimer-test-stubs.c| 3 ++- util/async.c | 18 +- util/main-loop.c | 5 +++-- util/trace-events | 1 + 40 files changed, 180 insertions(+), 41 deletions(-) -- 2.39.0
[PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
[PATCH v10 5/8] lsi53c895a: disable reentrancy detection for script RAM
As the code is designed to use the memory APIs to access the script ram, disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion. In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion. Reported-by: Fiona Ebner Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth Reviewed-by: Darren Kenny --- hw/scsi/lsi53c895a.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index af93557a9a..db27872963 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); +/* + * Since we use the address-space API to interact with ram_io, disable the + * re-entrancy guard. + */ +s->ram_io.disable_reentrancy_guard = true; + address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1); -- 2.39.0
[PATCH v10 8/8] apic: disable reentrancy detection for apic-msi
As the code is designed for re-entrant calls to apic-msi, mark apic-msi as reentrancy-safe. Signed-off-by: Alexander Bulekov Reviewed-by: Darren Kenny --- hw/intc/apic.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 20b5a94073..ac3d47d231 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -885,6 +885,13 @@ static void apic_realize(DeviceState *dev, Error **errp) memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi", APIC_SPACE_SIZE); +/* + * apic-msi's apic_mem_write can call into ioapic_eoi_broadcast, which can + * write back to apic-msi. As such mark the apic-msi region re-entrancy + * safe. + */ +s->io_memory.disable_reentrancy_guard = true; + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s); local_apics[s->id] = s; -- 2.39.0
[PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v10 1/8] memory: prevent dma-reentracy issues
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
include/exec/memory.h | 5 +
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 16
3 files changed, 28 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..fe23f0e5ce 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions
*/
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+warn_report("Blocked re-entrant IO on "
+"MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
+memory_region_name(mr), addr);
+return MEMTX_ACCESS_ERROR;
+}
+mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+}
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+if (mr->dev) {
+mr->dev->mem_reentrancy_guard.engaged_in_io = false;
+}
return r;
}
@@ -1170,6 +1185,7 @@ static void memory_region_do_init(MemoryRegion *mr,
}
mr->name = g_strdup(name);
mr->owner = owner;
+mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
mr->ram_block = NULL;
if (name) {
--
2.39.0
Re: [PATCH v9 7/8] memory: abort on re-entrancy in debug builds
On 230426 1219, Alexander Bulekov wrote: > This is useful for using unit-tests/fuzzing to detect bugs introduced by > the re-entrancy guard mechanism into devices that are intentionally > re-entrant. > > Signed-off-by: Alexander Bulekov > Reviewed-by: Thomas Huth > --- This doesn't actually do anything right now. Doesn't look like DEBUG is defined with --enable-debug Any suggestion for how to make re-entrancy louder/fatal on debug/developer builds? Maybe we can just replace the trace event with an unconditional log-message?
[PATCH] apic: disable reentrancy detection for apic-msi
As the code is designed for re-entrant calls to apic-msi, mark apic-msi as reentrancy-safe. Signed-off-by: Alexander Bulekov --- Based-on: <20230426161951.2948996-1-alx...@bu.edu> hw/intc/apic.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/intc/apic.c b/hw/intc/apic.c index 20b5a94073..ac3d47d231 100644 --- a/hw/intc/apic.c +++ b/hw/intc/apic.c @@ -885,6 +885,13 @@ static void apic_realize(DeviceState *dev, Error **errp) memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi", APIC_SPACE_SIZE); +/* + * apic-msi's apic_mem_write can call into ioapic_eoi_broadcast, which can + * write back to apic-msi. As such mark the apic-msi region re-entrancy + * safe. + */ +s->io_memory.disable_reentrancy_guard = true; + s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s); local_apics[s->id] = s; -- 2.39.0
Re: [PATCH v9 0/8] memory: prevent dma-reentracy issues
On 230427 1504, Thomas Huth wrote: > On 26/04/2023 18.19, Alexander Bulekov wrote: > > v8-> v9: > > - Disable reentrancy checks for raven's iomem (Patch 8) > > - Fix non-bisectable disable_reentrancy_guard patch by squashing it > >into Patch 1. > > - Fix trailing whitespace > > Sorry for not noticing earlier (I think the test is not run on gitlab-CI), > but I just noticed another failing avocado test: > > tests/venv/bin/avocado --show console run \ > tests/avocado/tuxrun_baselines.py:TuxRunBaselineTest.test_x86_64 > > ... seems to hang now (and finally gets "INTERRUPTED"). > > If I got that right, the test is basically more or less doing: > > wget https://storage.tuxboot.com/x86_64/bzImage > wget https://storage.tuxboot.com/x86_64/rootfs.ext4.zst > unzstd rootfs.ext4.zst > qemu-system-x86_64 -cpu Nehalem -M q35 -kernel bzImage \ > -hda rootfs.ext4 -m 2G -append "root=/dev/sda console=ttyS0" \ > -serial stdio > > Then log in as "root" and shut down with "halt". > > The "halt" works fine with git master, but it fails for me when I have your > patches applied. Could you please have a look? #0 trace_memory_region_reentrant_io at trace/trace-softmmu.h:335 #1 0x55fad303 in access_with_adjusted_size at ../softmmu/memory.c:549 #2 0x55fad0eb in memory_region_dispatch_write at ../softmmu/memory.c:1531 #3 0x55fbb543 in address_space_stl_internal at ../memory_ldst.c.inc:319 #4 0x55f29149 in stl_le_phys at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:121 #5 ioapic_service at ../hw/intc/ioapic.c:138 #6 0x55f28bd1 in ioapic_eoi_broadcast at ../hw/intc/ioapic.c:286 #7 0x55f26e4a in apic_eoi at ../hw/intc/apic.c:432 #8 0x55f2687c in apic_mem_write at ../hw/intc/apic.c:788 #9 0x55fad3f9 in memory_region_write_accessor at ../softmmu/memory.c:493 gef➤ p ((MemoryRegion*)mr)->name $2 = 0x578cbbe0 "apic-msi" I'll send a patch to mark this re-entrancy safe based-on this series. -Alex
[PATCH v9 7/8] memory: abort on re-entrancy in debug builds
This is useful for using unit-tests/fuzzing to detect bugs introduced by
the re-entrancy guard mechanism into devices that are intentionally
re-entrant.
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
softmmu/memory.c | 3 +++
util/async.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af9365bb81..d038633a6c 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -547,6 +547,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+#ifdef DEBUG
+abort();
+#endif
return MEMTX_ACCESS_ERROR;
}
mr->dev->mem_reentrancy_guard.engaged_in_io = true;
diff --git a/util/async.c b/util/async.c
index a9b528c370..2dc9389e0d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -160,6 +160,9 @@ void aio_bh_call(QEMUBH *bh)
last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
if (bh->reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
+#ifdef DEBUG
+abort();
+#endif
}
bh->reentrancy_guard->engaged_in_io = true;
}
--
2.39.0
[PATCH v9 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
[PATCH v9 6/8] bcm2835_property: disable reentrancy detection for iomem
As the code is designed for re-entrant calls from bcm2835_property to bcm2835_mbox and back into bcm2835_property, mark iomem as reentrancy-safe. Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth --- hw/misc/bcm2835_property.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 890ae7bae5..de056ea2df 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -382,6 +382,13 @@ static void bcm2835_property_init(Object *obj) memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_property_ops, s, TYPE_BCM2835_PROPERTY, 0x10); + +/* + * bcm2835_property_ops call into bcm2835_mbox, which in-turn reads from + * iomem. As such, mark iomem as re-entracy safe. + */ +s->iomem.disable_reentrancy_guard = true; + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(s), &s->mbox_irq); } -- 2.39.0
[PATCH v9 1/8] memory: prevent dma-reentracy issues
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
include/exec/memory.h | 5 +
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 14 ++
softmmu/trace-events | 1 +
4 files changed, 27 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 15ade918ba..e45ce6061f 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
@@ -791,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b1a6cae6f5..af9365bb81 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,16 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions
*/
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+return MEMTX_ACCESS_ERROR;
+}
+mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+}
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +566,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+if (mr->dev) {
+mr->dev->mem_reentrancy_guard.engaged_in_io = false;
+}
return r;
}
@@ -1170,6 +1183,7 @@ static void memory_region_do_init(MemoryRegion *mr,
}
mr->name = g_strdup(name);
mr->owner = owner;
+mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
mr->ram_block = NULL;
if (name) {
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 22606dc27b..62d04ea9a7 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t
addr, uint64_t value, u
memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
0x%"PRIx64" size %u name '%s'"
memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) &
[PATCH v9 5/8] lsi53c895a: disable reentrancy detection for script RAM
As the code is designed to use the memory APIs to access the script ram, disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion. In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion. Reported-by: Fiona Ebner Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth Reviewed-by: Darren Kenny --- hw/scsi/lsi53c895a.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index af93557a9a..db27872963 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); +/* + * Since we use the address-space API to interact with ram_io, disable the + * re-entrancy guard. + */ +s->ram_io.disable_reentrancy_guard = true; + address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1); -- 2.39.0
[PATCH v9 8/8] raven: disable reentrancy detection for iomem
As the code is designed for re-entrant calls from raven_io_ops to pci-conf, mark raven_io_ops as reentrancy-safe. Signed-off-by: Alexander Bulekov --- hw/pci-host/raven.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c index 072ffe3c5e..9a11ac4b2b 100644 --- a/hw/pci-host/raven.c +++ b/hw/pci-host/raven.c @@ -294,6 +294,13 @@ static void raven_pcihost_initfn(Object *obj) memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f00); address_space_init(&s->pci_io_as, &s->pci_io, "raven-io"); +/* + * Raven's raven_io_ops use the address-space API to access pci-conf-idx + * (which is also owned by the raven device). As such, mark the + * pci_io_non_contiguous as re-entrancy safe. + */ +s->pci_io_non_contiguous.disable_reentrancy_guard = true; + /* CPU address space */ memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR, &s->pci_io); -- 2.39.0
[PATCH v9 0/8] memory: prevent dma-reentracy issues
v8-> v9: - Disable reentrancy checks for raven's iomem (Patch 8) - Fix non-bisectable disable_reentrancy_guard patch by squashing it into Patch 1. - Fix trailing whitespace v7 -> v8: - Disable reentrancy checks for bcm2835_property's iomem (Patch 7) - Cache DeviceState* in the MemoryRegion to avoid dynamic cast for each MemoryRegion access. (Patch 1) - Make re-entrancy fatal for debug-builds (Patch 8) v6 -> v7: - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4) - Add an MR-specific flag to disable reentrancy (Patch 5) - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6) Patches 5 and 6 need review. I left the review-tags for Patch 4, however a few of the qemu_bh_new_guarded calls have changed. v5 -> v6: - Only apply checkpatch checks to code in paths containing "/hw/" (/hw/ and include/hw/) - Fix a bug in a _guarded call added to hw/block/virtio-blk.c v4-> v5: - Add corresponding checkpatch checks - Save/restore reentrancy-flag when entering/exiting BHs - Improve documentation - Check object_dynamic_cast return value v3 -> v4: Instead of changing all of the DMA APIs, instead add an optional reentrancy guard to the BH API. v2 -> v3: Bite the bullet and modify the DMA APIs, rather than attempting to guess DeviceStates in BHs. These patches aim to solve two types of DMA-reentrancy issues: 1.) mmio -> dma -> mmio case To solve this, we track whether the device is engaged in io by checking/setting a reentrancy-guard within APIs used for MMIO access. 2.) bh -> dma write -> mmio case This case is trickier, since we dont have a generic way to associate a bh with the underlying Device/DeviceState. Thus, this version allows a device to associate a reentrancy-guard with a bh, when creating it. (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded) I replaced most of the qemu_bh_new invocations with the guarded analog, except for the ones where the DeviceState was not trivially accessible. Alexander Bulekov (8): memory: prevent dma-reentracy issues async: Add an optional reentrancy guard to the BH API checkpatch: add qemu_bh_new/aio_bh_new checks hw: replace most qemu_bh_new calls with qemu_bh_new_guarded lsi53c895a: disable reentrancy detection for script RAM bcm2835_property: disable reentrancy detection for iomem memory: abort on re-entrancy in debug builds raven: disable reentrancy detection for iomem docs/devel/multiple-iothreads.txt | 7 +++ hw/9pfs/xen-9p-backend.c | 5 - hw/block/dataplane/virtio-blk.c | 3 ++- hw/block/dataplane/xen-block.c| 5 +++-- hw/char/virtio-serial-bus.c | 3 ++- hw/display/qxl.c | 9 ++--- hw/display/virtio-gpu.c | 6 -- hw/ide/ahci.c | 3 ++- hw/ide/ahci_internal.h| 1 + hw/ide/core.c | 4 +++- hw/misc/bcm2835_property.c| 7 +++ hw/misc/imx_rngc.c| 6 -- hw/misc/macio/mac_dbdma.c | 2 +- hw/net/virtio-net.c | 3 ++- hw/nvme/ctrl.c| 6 -- hw/pci-host/raven.c | 7 +++ hw/scsi/lsi53c895a.c | 6 ++ hw/scsi/mptsas.c | 3 ++- hw/scsi/scsi-bus.c| 3 ++- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/dev-uas.c | 3 ++- hw/usb/hcd-dwc2.c | 3 ++- hw/usb/hcd-ehci.c | 3 ++- hw/usb/hcd-uhci.c | 2 +- hw/usb/host-libusb.c | 6 -- hw/usb/redirect.c | 6 -- hw/usb/xen-usb.c | 3 ++- hw/virtio/virtio-balloon.c| 5 +++-- hw/virtio/virtio-crypto.c | 3 ++- include/block/aio.h | 18 -- include/exec/memory.h | 5 + include/hw/qdev-core.h| 7 +++ include/qemu/main-loop.h | 7 +-- scripts/checkpatch.pl | 8 softmmu/memory.c | 17 + softmmu/trace-events | 1 + tests/unit/ptimer-test-stubs.c| 3 ++- util/async.c | 21 - util/main-loop.c | 5 +++-- util/trace-events | 1 + 40 files changed, 178 insertions(+), 41 deletions(-) -- 2.39.0
[PATCH v9 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v9 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks
Advise authors to use the _guarded versions of the APIs, instead.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers;
signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of
qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new()
to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc.
instead of module_init()\n" . $herecurr);
--
2.39.0
Re: [PATCH v8 0/8] memory: prevent dma-reentracy issues
On 230425 1146, Thomas Huth wrote: > On 21/04/2023 16.27, Alexander Bulekov wrote: > > v7 -> v8: > > - Disable reentrancy checks for bcm2835_property's iomem (Patch 7) > > - Cache DeviceState* in the MemoryRegion to avoid dynamic cast for > >each MemoryRegion access. (Patch 1) > > - Make re-entrancy fatal for debug-builds (Patch 8) > > Hi Alexander, > > I just put your series into a run with the gitlab-CI and it seems this now > introduced another failure in one of the avocado tests: > > https://gitlab.com/thuth/qemu/-/jobs/4171448248#L318 > > The "IbmPrep40pMachine.test_openbios_and_netbsd" test is failing now. > > You can reproduce it manually quite easily: > > wget > https://archive.netbsd.org/pub/NetBSD-archive/NetBSD-7.1.2/iso/NetBSD-7.1.2-prep.iso > > ./qemu-system-ppc -nographic -M 40p -boot d -cdrom NetBSD-7.1.2-prep.iso > > Without your patches, this prints out "NetBSD/prep BOOT, Revision 1.9" in > the console, but with your patches, the message does not appear anymore. #0 trace_memory_region_reentrant_io at trace/trace-softmmu.h:335 #1 0x55f3f315 in access_with_adjusted_size at ../softmmu/memory.c:549 #2 0x55f3f0f6 in memory_region_dispatch_write at ../softmmu/memory.c:1531 #3 0x55f4e655 in flatview_write_continue at ../softmmu/physmem.c:2641 #4 0x55f4b30e in flatview_write at ../softmmu/physmem.c:2683 #5 0x55f4b119 in address_space_write at ../softmmu/physmem.c:2779 #6 0x55c5a233 in raven_io_write at ../hw/pci-host/raven.c:194 #7 0x55f3f409 in memory_region_write_accessor at ../softmmu/memory.c:493 #8 0x55f3f22b in access_with_adjusted_size at ../softmmu/memory.c:568 #9 0x55f3f0f6 in memory_region_dispatch_write at ../softmmu/memory.c:1531 #10 0x55fcf74b in io_writex at ../accel/tcg/cputlb.c:1430 #11 0x55fc33dd in store_helper at ../accel/tcg/cputlb.c:2454 #12 full_le_stl_mmu at ../accel/tcg/cputlb.c:2542 #13 0x7fffa404b78c in code_gen_buffer #14 0x55fa510d in cpu_tb_exec at ../accel/tcg/cpu-exec.c:460 #15 0x55fa5ec0 in cpu_loop_exec_tb at ../accel/tcg/cpu-exec.c:893 #16 cpu_exec_loop at ../accel/tcg/cpu-exec.c:1013 #17 0x55fa5697 in cpu_exec_setjmp at ../accel/tcg/cpu-exec.c:1043 #18 0x55fa558e in cpu_exec at ../accel/tcg/cpu-exec.c:1069 #19 0x55fd1f8f in tcg_cpus_exec at ../accel/tcg/tcg-accel-ops.c:81 #20 0x55fd2c16 in rr_cpu_thread_fn at ../accel/tcg/tcg-accel-ops-rr.c:223 #21 0x56156465 in qemu_thread_start at ../util/qemu-thread-posix.c:541 #22 0x76960fd4 in start_thread at ./nptl/pthread_create.c:442 #23 0x769e166c in clone3 at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 gef➤ p ->name $4 = 0x57615bd0 "pci-conf-idx" Raven owns both the ravio_io MR and the pci-conf MRs. raven_io needs to be marked re-entrancy safe.
Re: [PATCH v8 5/8] memory: Allow disabling re-entrancy checking per-MR
On 230425 0941, Thomas Huth wrote:
> On 21/04/2023 16.27, Alexander Bulekov wrote:
> > Signed-off-by: Alexander Bulekov
> > Reviewed-by: Thomas Huth
> > Reviewed-by: Darren Kenny
> > ---
> > include/exec/memory.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 6c0a5e68d3..4e9531bd8a 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -793,6 +793,9 @@ struct MemoryRegion {
> > unsigned ioeventfd_nb;
> > MemoryRegionIoeventfd *ioeventfds;
> > RamDiscardManager *rdm; /* Only for RAM */
> > +
> > +/* For devices designed to perform re-entrant IO into their own IO MRs
> > */
> > +bool disable_reentrancy_guard;
> > };
> > struct IOMMUMemoryRegion {
>
> Oh, wait, that check for "!mr->disable_reentrancy_guard" has been squashed
> into the first patch now ... that's bad, I think you should squash this into
> the first patch now, too!
>
Oh.. Thanks for catching that.
[PATCH v8 8/8] memory: abort on re-entrancy in debug builds
This is useful for using unit-tests/fuzzing to detect bugs introduced by
the re-entrancy guard mechanism into devices that are intentionally
re-entrant.
Signed-off-by: Alexander Bulekov
---
softmmu/memory.c | 3 +++
util/async.c | 3 +++
2 files changed, 6 insertions(+)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index a11ee3e30d..5390f91db6 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -547,6 +547,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+#ifdef DEBUG
+abort();
+#endif
return MEMTX_ACCESS_ERROR;
}
mr->dev->mem_reentrancy_guard.engaged_in_io = true;
diff --git a/util/async.c b/util/async.c
index a9b528c370..2dc9389e0d 100644
--- a/util/async.c
+++ b/util/async.c
@@ -160,6 +160,9 @@ void aio_bh_call(QEMUBH *bh)
last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
if (bh->reentrancy_guard->engaged_in_io) {
trace_reentrant_aio(bh->ctx, bh->name);
+#ifdef DEBUG
+abort();
+#endif
}
bh->reentrancy_guard->engaged_in_io = true;
}
--
2.39.0
[PATCH v8 7/8] bcm2835_property: disable reentrancy detection for iomem
As the code is designed for re-entrant calls from bcm2835_property to bcm2835_mbox and back into bcm2835_property, mark iomem as reentrancy-safe. Signed-off-by: Alexander Bulekov --- hw/misc/bcm2835_property.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c index 890ae7bae5..de056ea2df 100644 --- a/hw/misc/bcm2835_property.c +++ b/hw/misc/bcm2835_property.c @@ -382,6 +382,13 @@ static void bcm2835_property_init(Object *obj) memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_property_ops, s, TYPE_BCM2835_PROPERTY, 0x10); + +/* + * bcm2835_property_ops call into bcm2835_mbox, which in-turn reads from + * iomem. As such, mark iomem as re-entracy safe. + */ +s->iomem.disable_reentrancy_guard = true; + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); sysbus_init_irq(SYS_BUS_DEVICE(s), &s->mbox_irq); } -- 2.39.0
[PATCH v8 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render
[PATCH v8 0/8] memory: prevent dma-reentracy issues
v7 -> v8: - Disable reentrancy checks for bcm2835_property's iomem (Patch 7) - Cache DeviceState* in the MemoryRegion to avoid dynamic cast for each MemoryRegion access. (Patch 1) - Make re-entrancy fatal for debug-builds (Patch 8) v6 -> v7: - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4) - Add an MR-specific flag to disable reentrancy (Patch 5) - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6) Patches 5 and 6 need review. I left the review-tags for Patch 4, however a few of the qemu_bh_new_guarded calls have changed. v5 -> v6: - Only apply checkpatch checks to code in paths containing "/hw/" (/hw/ and include/hw/) - Fix a bug in a _guarded call added to hw/block/virtio-blk.c v4-> v5: - Add corresponding checkpatch checks - Save/restore reentrancy-flag when entering/exiting BHs - Improve documentation - Check object_dynamic_cast return value v3 -> v4: Instead of changing all of the DMA APIs, instead add an optional reentrancy guard to the BH API. v2 -> v3: Bite the bullet and modify the DMA APIs, rather than attempting to guess DeviceStates in BHs. These patches aim to solve two types of DMA-reentrancy issues: 1.) mmio -> dma -> mmio case To solve this, we track whether the device is engaged in io by checking/setting a reentrancy-guard within APIs used for MMIO access. 2.) bh -> dma write -> mmio case This case is trickier, since we dont have a generic way to associate a bh with the underlying Device/DeviceState. Thus, this version allows a device to associate a reentrancy-guard with a bh, when creating it. (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded) I replaced most of the qemu_bh_new invocations with the guarded analog, except for the ones where the DeviceState was not trivially accessible. Alexander Bulekov (8): memory: prevent dma-reentracy issues async: Add an optional reentrancy guard to the BH API checkpatch: add qemu_bh_new/aio_bh_new checks hw: replace most qemu_bh_new calls with qemu_bh_new_guarded memory: Allow disabling re-entrancy checking per-MR lsi53c895a: disable reentrancy detection for script RAM bcm2835_property: disable reentrancy detection for iomem memory: abort on re-entrancy in debug builds docs/devel/multiple-iothreads.txt | 7 +++ hw/9pfs/xen-9p-backend.c | 5 - hw/block/dataplane/virtio-blk.c | 3 ++- hw/block/dataplane/xen-block.c| 5 +++-- hw/char/virtio-serial-bus.c | 3 ++- hw/display/qxl.c | 9 ++--- hw/display/virtio-gpu.c | 6 -- hw/ide/ahci.c | 3 ++- hw/ide/ahci_internal.h| 1 + hw/ide/core.c | 4 +++- hw/misc/bcm2835_property.c| 7 +++ hw/misc/imx_rngc.c| 6 -- hw/misc/macio/mac_dbdma.c | 2 +- hw/net/virtio-net.c | 3 ++- hw/nvme/ctrl.c| 6 -- hw/scsi/lsi53c895a.c | 6 ++ hw/scsi/mptsas.c | 3 ++- hw/scsi/scsi-bus.c| 3 ++- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/dev-uas.c | 3 ++- hw/usb/hcd-dwc2.c | 3 ++- hw/usb/hcd-ehci.c | 3 ++- hw/usb/hcd-uhci.c | 2 +- hw/usb/host-libusb.c | 6 -- hw/usb/redirect.c | 6 -- hw/usb/xen-usb.c | 3 ++- hw/virtio/virtio-balloon.c| 5 +++-- hw/virtio/virtio-crypto.c | 3 ++- include/block/aio.h | 18 -- include/exec/memory.h | 5 + include/hw/qdev-core.h| 7 +++ include/qemu/main-loop.h | 7 +-- scripts/checkpatch.pl | 8 softmmu/memory.c | 17 + softmmu/trace-events | 1 + tests/unit/ptimer-test-stubs.c| 3 ++- util/async.c | 21 - util/main-loop.c | 5 +++-- util/trace-events | 1 + 39 files changed, 171 insertions(+), 41 deletions(-) -- 2.39.0
[PATCH v8 2/8] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 543717f294..db6f23c619 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, c
[PATCH v8 5/8] memory: Allow disabling re-entrancy checking per-MR
Signed-off-by: Alexander Bulekov
Reviewed-by: Thomas Huth
Reviewed-by: Darren Kenny
---
include/exec/memory.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6c0a5e68d3..4e9531bd8a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -793,6 +793,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
--
2.39.0
[PATCH v8 6/8] lsi53c895a: disable reentrancy detection for script RAM
As the code is designed to use the memory APIs to access the script ram, disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion. In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion. Reported-by: Fiona Ebner Signed-off-by: Alexander Bulekov Reviewed-by: Thomas Huth Reviewed-by: Darren Kenny --- hw/scsi/lsi53c895a.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index af93557a9a..db27872963 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); +/* + * Since we use the address-space API to interact with ram_io, disable the + * re-entrancy guard. + */ +s->ram_io.disable_reentrancy_guard = true; + address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1); -- 2.39.0
[PATCH v8 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks
Advise authors to use the _guarded versions of the APIs, instead.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers;
signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of
qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new()
to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc.
instead of module_init()\n" . $herecurr);
--
2.39.0
[PATCH v8 1/8] memory: prevent dma-reentracy issues
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Resolves: CVE-2023-0330
Signed-off-by: Alexander Bulekov
---
include/exec/memory.h | 2 ++
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 14 ++
softmmu/trace-events | 1 +
4 files changed, 24 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6fa0b071f0..6c0a5e68d3 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -767,6 +767,8 @@ struct MemoryRegion {
bool is_iommu;
RAMBlock *ram_block;
Object *owner;
+/* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath
*/
+DeviceState *dev;
const MemoryRegionOps *ops;
void *opaque;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 4699ba55ec..a11ee3e30d 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -542,6 +542,16 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultaneous access to a device's IO Regions
*/
+if (mr->dev && !mr->disable_reentrancy_guard &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
+trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, size);
+return MEMTX_ACCESS_ERROR;
+}
+mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+}
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +566,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+if (mr->dev) {
+mr->dev->mem_reentrancy_guard.engaged_in_io = false;
+}
return r;
}
@@ -1170,6 +1183,7 @@ static void memory_region_do_init(MemoryRegion *mr,
}
mr->name = g_strdup(name);
mr->owner = owner;
+mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
mr->ram_block = NULL;
if (name) {
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 22606dc27b..62d04ea9a7 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t
addr, uint64_t value, u
memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
0x%"PRIx64" size %u name '%s'"
memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
+memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned
size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size) "cpu %d mr %p ad
Re: [PATCH] rtl8139: fix large_send_mss divide-by-zero
On 230413 1319, Stefan Hajnoczi wrote:
> If the driver sets large_send_mss to 0 then a divide-by-zero occurs.
> Even if the division wasn't a problem, the for loop that emits MSS-sized
> packets would never terminate.
>
> Solve these issues by skipping offloading when large_send_mss=0.
>
> This issue was found by OSS-Fuzz as part of Alexander Bulekov's device
> fuzzing work. The reproducer is:
>
> $ cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
> 512M,slots=1,maxmem=0x -machine q35 -nodefaults -device \
> rtl8139,netdev=net0 -netdev user,id=net0 -device \
> pc-dimm,id=nv1,memdev=mem1,addr=0xb800a6460280 -object \
> memory-backend-ram,id=mem1,size=2M -qtest stdio
> outl 0xcf8 0x8814
> outl 0xcfc 0xe000
> outl 0xcf8 0x8804
> outw 0xcfc 0x06
> write 0xe037 0x1 0x04
> write 0xe0e0 0x2 0x01
> write 0x1 0x1 0x04
> write 0x3 0x1 0x98
> write 0xa 0x1 0x8c
> write 0xb 0x1 0x02
> write 0xc 0x1 0x46
> write 0xd 0x1 0xa6
> write 0xf 0x1 0xb8
> write 0xb800a646028c000c 0x1 0x08
> write 0xb800a646028c000e 0x1 0x47
> write 0xb800a646028c0010 0x1 0x02
> write 0xb800a646028c0017 0x1 0x06
> write 0xb800a646028c0036 0x1 0x80
> write 0xe0d9 0x1 0x40
> EOF
>
> Buglink: https://gitlab.com/qemu-project/qemu/-/issues/1582
Maybe instead:
Closes: https://gitlab.com/qemu-project/qemu/-/issues/1582
so that gitlab will auto-close the issue.
Tested-by: Alexander Bulekov
Thank you
-Alex
> Fixes: 6d71357a3b65 ("rtl8139: honor large send MSS value")
> Reported-by: Alexander Bulekov
> Cc: Peter Maydell
> Signed-off-by: Stefan Hajnoczi
> ---
> hw/net/rtl8139.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 5a5aaf868d..5f1a4d359b 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -2154,6 +2154,9 @@ static int rtl8139_cplus_transmit_one(RTL8139State *s)
>
> int large_send_mss = (txdw0 >> CP_TC_LGSEN_MSS_SHIFT) &
> CP_TC_LGSEN_MSS_MASK;
> +if (large_send_mss == 0) {
> +goto skip_offload;
> +}
>
> DPRINTF("+++ C+ mode offloaded task TSO IP data %d "
> "frame data %d specified MSS=%d\n",
> --
> 2.39.2
>
Re: [PATCH v3] hw/arm: do not free machine->fdt in arm_load_dtb()
On 230328 1859, Markus Armbruster wrote:
> At this moment, arm_load_dtb() can free machine->fdt when
> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be
> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is
> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to
> machine->fdt. And, in that case, the existing g_free(fdt) at the end of
> arm_load_dtb() will make machine->fdt point to an invalid memory region.
>
> Since monitor command 'dumpdtb' was introduced a couple of releases
> ago, running it with any ARM machine that uses arm_load_dtb() will
> crash QEMU.
>
> Let's enable all arm_load_dtb() callers to use dumpdtb properly. Instead
> of freeing 'fdt', assign it back to ms->fdt.
>
> Cc: Peter Maydell
> Cc: qemu-...@nongnu.org
> Fixes: bf353ad55590f ("qmp/hmp, device_tree.c: introduce dumpdtb")
> Reported-by: Markus Armbruster i
^ Seems like a typo
-Alex
Re: [PATCH] usb/dev-wacom: fix OOB write in usb_mouse_poll()
On 230213 1841, Mauro Matteo Cascella wrote:
> The guest can control the size of buf; an OOB write occurs when buf is 1 or 2
> bytes long. Only fill in the buffer as long as there is enough space, throw
> away any data which doesn't fit.
>
> Signed-off-by: Mauro Matteo Cascella
Tested-by: Alexander Bulekov
Thanks
> ---
> hw/usb/dev-wacom.c | 20 +---
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
> index 7177c17f03..ca9e6aa82f 100644
> --- a/hw/usb/dev-wacom.c
> +++ b/hw/usb/dev-wacom.c
> @@ -252,14 +252,20 @@ static int usb_mouse_poll(USBWacomState *s, uint8_t
> *buf, int len)
> if (s->buttons_state & MOUSE_EVENT_MBUTTON)
> b |= 0x04;
>
> -buf[0] = b;
> -buf[1] = dx;
> -buf[2] = dy;
> -l = 3;
> -if (len >= 4) {
> -buf[3] = dz;
> -l = 4;
> +l = 0;
> +if (len > l) {
> +buf[l++] = b;
> }
> +if (len > l) {
> +buf[l++] = dx;
> +}
> +if (len > l) {
> +buf[l++] = dy;
> +}
> +if (len > l) {
> +buf[l++] = dz;
> +}
> +
> return l;
> }
>
> --
> 2.39.1
>
>
Re: [PATCH] usb/dev-wacom: fix OOB write in usb_mouse_poll()
On 230214 1148, Mauro Matteo Cascella wrote: > Hi Philippe, > > On Mon, Feb 13, 2023 at 7:26 PM Philippe Mathieu-Daudé > wrote: > > > > Hi Mauro, > > > > On 13/2/23 18:41, Mauro Matteo Cascella wrote: > > > The guest can control the size of buf; an OOB write occurs when buf is 1 > > > or 2 > > > bytes long. Only fill in the buffer as long as there is enough space, > > > throw > > > away any data which doesn't fit. > > > > Any reproducer? > > No qtest reproducer, we do have a PoC module to compile & load from > within the guest. This is ASAN output: OSS-Fuzz reported something that looks similar. Issue 57504. Stack-Trace: ==1038012==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x (pc 0x55dc41bec541 bp 0x7ffdd50a33b0 sp 0x7ffdd50a3350 T1038012) ==1038012==The signal is caused by a WRITE memory access. ==1038012==Hint: address points to the zero page. #0 0x55dc41bec541 in usb_mouse_poll /home/alxndr/Development/qemu/build/../hw/usb/dev-wacom.c:255:12 #1 0x55dc41bec013 in usb_wacom_handle_data /home/alxndr/Development/qemu/build/../hw/usb/dev-wacom.c:386:23 #2 0x55dc41b93ae9 in usb_device_handle_data /home/alxndr/Development/qemu/build/../hw/usb/bus.c:180:9 #3 0x55dc41b9a004 in usb_process_one /home/alxndr/Development/qemu/build/../hw/usb/core.c:406:9 #4 0x55dc41b999cf in usb_handle_packet /home/alxndr/Development/qemu/build/../hw/usb/core.c:438:9 #5 0x55dc41bd6645 in xhci_submit /home/alxndr/Development/qemu/build/../hw/usb/hcd-xhci.c:1831:5 #6 0x55dc41bd325b in xhci_fire_transfer /home/alxndr/Development/qemu/build/../hw/usb/hcd-xhci.c:1840:12 #7 0x55dc41bd0ef3 in xhci_kick_epctx /home/alxndr/Development/qemu/build/../hw/usb/hcd-xhci.c:1999:13 #8 0x55dc41bd8635 in xhci_kick_ep /home/alxndr/Development/qemu/build/../hw/usb/hcd-xhci.c:1865:5 #9 0x55dc41bdca04 in xhci_doorbell_write /home/alxndr/Development/qemu/build/../hw/usb/hcd-xhci.c:3171:13 #10 0x55dc41f4c75a in memory_region_write_accessor /home/alxndr/Development/qemu/build/../softmmu/memory.c:493:5 #11 0x55dc41f4c60f in access_with_adjusted_size /home/alxndr/Development/qemu/build/../softmmu/memory.c:555:18 #12 0x55dc41f4c1f8 in memory_region_dispatch_write /home/alxndr/Development/qemu/build/../softmmu/memory.c:1515:16 #13 0x55dc41f63932 in flatview_write_continue /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2826:23 #14 0x55dc41f60577 in flatview_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2868:12 #15 0x55dc41f60497 in address_space_write /home/alxndr/Development/qemu/build/../softmmu/physmem.c:2964:18 Here's the QTest Reproducer: cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \ 512M -machine q35 -nodefaults -drive \ file=null-co://,if=none,format=raw,id=disk0 -device qemu-xhci,id=xhci \ -device usb-tablet,bus=xhci.0 -device usb-bot -device \ usb-storage,drive=disk0 -chardev null,id=cd0 -chardev null,id=cd1 \ -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid \ -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1 -device\ usb-tablet -device usb-wacom-tablet -device usb-audio -qtest stdio outl 0xcf8 0x8810 outl 0xcfc 0xe000 outl 0xcf8 0x8804 outw 0xcfc 0x06 writel 0xe040 0x1 write 0x3d 0x1 0x04 write 0x5d 0x1 0x04 write 0x6d 0x1 0x04 write 0x7d 0x1 0x04 write 0x8d 0x1 0x04 write 0x9d 0x1 0x04 write 0xad 0x1 0x04 write 0xbd 0x1 0x04 write 0xcd 0x1 0x04 write 0xdd 0x1 0x04 write 0xed 0x1 0x04 write 0xfd 0x1 0x04 write 0x10d 0x1 0x04 write 0x11d 0x1 0x04 write 0x12d 0x1 0x04 write 0x13d 0x1 0x04 write 0x14d 0x1 0x04 write 0x15d 0x1 0x04 write 0x16d 0x1 0x04 write 0x17d 0x1 0x04 write 0x18d 0x1 0x04 write 0x19d 0x1 0x04 write 0x1ad 0x1 0x04 write 0x1bd 0x1 0x04 write 0x1cd 0x1 0x04 write 0x1dd 0x1 0x04 write 0x1ed 0x1 0x04 write 0x1fd 0x1 0x04 write 0x20d 0x1 0x04 write 0x21d 0x1 0x04 write 0x22d 0x1 0x04 write 0x23d 0x1 0x04 write 0x24d 0x1 0x04 write 0x25d 0x1 0x04 write 0x26d 0x1 0x04 write 0x27d 0x1 0x04 write 0x28d 0x1 0x04 write 0x29d 0x1 0x04 write 0x2ad 0x1 0x04 write 0x2bd 0x1 0x04 write 0x2cd 0x1 0x04 write 0x2dd 0x1 0x04 write 0x2ed 0x1 0x04 write 0x2fd 0x1 0x04 write 0x30d 0x1 0x04 write 0x31d 0x1 0x04 write 0x32d 0x1 0x04 write 0x33d 0x1 0x04 write 0x34d 0x1 0x04 write 0x35d 0x1 0x04 write 0x36d 0x1 0x04 write 0x37d 0x1 0x04 write 0x38d 0x1 0x04 write 0x39d 0x1 0x04 write 0x3ad 0x1 0x04 write 0x3bd 0x1 0x04 write 0x3cd 0x1 0x04 write 0x3dd 0x1 0x04 write 0x3ed 0x1 0x04 write 0x3fd 0x1 0x04 write 0x40d 0x1 0x04 write 0x41d 0x1 0x04 write 0x42d 0x1 0x04 write 0x43d 0x1 0x04 write 0x44d 0x1 0x04 write 0x45d 0x1 0x04 write 0x46d 0x1 0x04 write 0x47d 0x1 0x04 write 0x48d 0x1 0x04 write 0x49d 0x1 0x04 write 0x4ad 0x1 0x04 write 0x4bd 0x1 0x04 write 0x4cd 0x1 0x04 write 0x4dd 0x1 0x04 write 0x4ed 0x1 0x04 write 0x4fd 0x1 0x04 write 0x50d 0x1 0x04 write 0x51d 0x1 0x04 write 0x52d 0x1 0x04 write 0x53d 0x1 0x04 write 0x54d 0x1 0x04 write 0x55d 0x1 0x04 write 0x56d 0x1 0x04 write 0x57d 0x1 0x04 write 0x58d
Re: [PATCH] scsi/lsi53c895a: restrict DMA engine to memory regions (CVE-2023-0330)
On 230324 1200, Mauro Matteo Cascella wrote:
> On Fri, Mar 17, 2023 at 10:59 PM Philippe Mathieu-Daudé
> wrote:
> >
> > On 17/3/23 19:18, Karl Heubaum wrote:
> > > Did this CVE fix fall in the cracks during the QEMU 8.0 merge window?
> >
> > The patch isn't reviewed, and apparently almost no active contributor
> > understand this device enough to be sure this security patch doesn't
> > break normal use. Fuzzed bugs are rarely trivial.
>
> I guess Alexander's new patchset [1] does not help fix this one?
>
> [1]
> https://patchew.org/QEMU/20230313082417.827484-1-alx...@bu.edu/20230313082417.827484-7-alx...@bu.edu/
>
It should cover it. At least the reproducer in this email no longer
works.
-Alex
>
> > >> On Jan 16, 2023, at 2:42 PM, Mauro Matteo Cascella
> > >> wrote:
> > >>
> > >> This prevents the well known DMA-MMIO reentrancy problem (upstream issue
> > >> #556)
> > >> leading to memory corruption bugs like stack overflow or use-after-free.
> > >>
> > >> Fixes: CVE-2023-0330
> > >> Signed-off-by: Mauro Matteo Cascella
> > >> Reported-by: Zheyu Ma
> > >> ---
> > >> hw/scsi/lsi53c895a.c | 14 +
> > >> tests/qtest/fuzz-lsi53c895a-test.c | 32 ++
> > >> 2 files changed, 42 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
> > >> index af93557a9a..89c52594eb 100644
> > >> --- a/hw/scsi/lsi53c895a.c
> > >> +++ b/hw/scsi/lsi53c895a.c
> > >> @@ -446,22 +446,28 @@ static void lsi_reselect(LSIState *s, lsi_request
> > >> *p);
> > >> static inline void lsi_mem_read(LSIState *s, dma_addr_t addr,
> > >> void *buf, dma_addr_t len)
> > >> {
> > >> +const MemTxAttrs attrs = { .memory = true };
> > >> +
> > >> if (s->dmode & LSI_DMODE_SIOM) {
> > >> -address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED,
> > >> +address_space_read(&s->pci_io_as, addr, attrs,
> > >> buf, len);
> > >> } else {
> > >> -pci_dma_read(PCI_DEVICE(s), addr, buf, len);
> > >> +pci_dma_rw(PCI_DEVICE(s), addr, buf, len,
> > >> + DMA_DIRECTION_TO_DEVICE, attrs);
> > >> }
> > >> }
> > >>
> > >> static inline void lsi_mem_write(LSIState *s, dma_addr_t addr,
> > >> const void *buf, dma_addr_t len)
> > >> {
> > >> +const MemTxAttrs attrs = { .memory = true };
> > >> +
> > >> if (s->dmode & LSI_DMODE_DIOM) {
> > >> -address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED,
> > >> +address_space_write(&s->pci_io_as, addr, attrs,
> > >> buf, len);
> > >> } else {
> > >> -pci_dma_write(PCI_DEVICE(s), addr, buf, len);
> > >> +pci_dma_rw(PCI_DEVICE(s), addr, (void *) buf, len,
> > >> + DMA_DIRECTION_FROM_DEVICE, attrs);
> > >> }
> > >> }
> > >>
> > >> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c
> > >> b/tests/qtest/fuzz-lsi53c895a-test.c
> > >> index 392a7ae7ed..35c02e89f3 100644
> > >> --- a/tests/qtest/fuzz-lsi53c895a-test.c
> > >> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
> > >> @@ -8,6 +8,35 @@
> > >> #include "qemu/osdep.h"
> > >> #include "libqtest.h"
> > >>
> > >> +/*
> > >> + * This used to trigger a DMA reentrancy issue
> > >> + * leading to memory corruption bugs like stack
> > >> + * overflow or use-after-free
> > >> + */
> > >> +static void test_lsi_dma_reentrancy(void)
> > >> +{
> > >> +QTestState *s;
> > >> +
> > >> +s = qtest_init("-M q35 -m 512M -nodefaults "
> > >> + "-blockdev driver=null-co,node-name=null0 "
> > >> + "-device lsi53c810 -device scsi-cd,drive=null0");
> > >> +
> > >> +qtest_outl(s, 0xcf8, 0x8804); /* PCI Command Register */
> > >> +qtest_outw(s, 0xcfc, 0x7);/* Enables accesses */
> > >> +qtest_outl(s, 0xcf8, 0x8814); /* Memory Bar 1 */
> > >> +qtest_outl(s, 0xcfc, 0xff10); /* Set MMIO Address*/
> > >> +qtest_outl(s, 0xcf8, 0x8818); /* Memory Bar 2 */
> > >> +qtest_outl(s, 0xcfc, 0xff00); /* Set RAM Address*/
> > >> +qtest_writel(s, 0xff00, 0xc024);
> > >> +qtest_writel(s, 0xff000114, 0x0080);
> > >> +qtest_writel(s, 0xff00012c, 0xff00);
> > >> +qtest_writel(s, 0xff04, 0xff000114);
> > >> +qtest_writel(s, 0xff08, 0xff100014);
> > >> +qtest_writel(s, 0xff10002f, 0x00ff);
> > >> +
> > >> +qtest_quit(s);
> > >> +}
> > >> +
> > >> /*
> > >> * This used to trigger a UAF in lsi_do_msgout()
> > >> * https://gitlab.com/qemu-project/qemu/-/issues/972
> > >> @@ -120,5 +149,8 @@ int main(int argc, char **argv)
> > >> qtest_add_func("fuzz/lsi53c895a/lsi_do_msgout_cancel_req",
> > >> test_lsi_do_msgout_cancel_req);
> > >>
> > >> +qtest_add_func("fuzz/lsi53c895a/lsi_dma_reentrancy",
> > >> + test_lsi_dma_reentrancy);
> > >> +
> > >> return g
Re: [PATCH for 8.0 v2] memory: Prevent recursive memory access
On 230316 2124, Akihiko Odaki wrote:
> A guest may request ask a memory-mapped device to perform DMA. If the
> address specified for DMA is the device performing DMA, it will create
> recursion. It is very unlikely that device implementations are prepared
> for such an abnormal access, which can result in unpredictable behavior.
>
> In particular, such a recursion breaks e1000e, a network device. If
> the device is configured to write the received packet to the register
> to trigger receiving, it triggers re-entry to the Rx logic of e1000e.
> This causes use-after-free since the Rx logic is not re-entrant.
>
> As there should be no valid reason to perform recursive memory access,
> check for recursion before accessing memory-mapped device.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1543
> Signed-off-by: Akihiko Odaki
Hi Akihiko,
I think the spirit of this is similar to the fix I proposed here:
https://lore.kernel.org/qemu-devel/20230313082417.827484-1-alx...@bu.edu/
My version also addresses the following case, which we have found
instances of:
Device Foo Bottom Half -> DMA write to Device Foo Memory Region
That said, the patch is held up on some corner cases and it seems it
will not make it into 8.0. I guess we can add #1543 to the list of
issues in https://gitlab.com/qemu-project/qemu/-/issues/556
Thanks
-Alex
> ---
> softmmu/memory.c | 79 +---
> 1 file changed, 62 insertions(+), 17 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 4699ba55ec..19c60ee1f0 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -50,6 +50,10 @@ static QTAILQ_HEAD(, AddressSpace) address_spaces
>
> static GHashTable *flat_views;
>
> +static const Object **accessed_region_owners;
> +static size_t accessed_region_owners_capacity;
> +static size_t accessed_region_owners_num;
> +
> typedef struct AddrRange AddrRange;
>
> /*
> @@ -1394,6 +1398,16 @@ bool memory_region_access_valid(MemoryRegion *mr,
> return false;
> }
>
> +for (size_t i = 0; i < accessed_region_owners_num; i++) {
> +if (accessed_region_owners[i] == mr->owner) {
> +qemu_log_mask(LOG_GUEST_ERROR, "Invalid %s at addr 0x%"
> HWADDR_PRIX
> + ", size %u, region '%s', reason: recursive
> access\n",
> + is_write ? "write" : "read",
> + addr, size, memory_region_name(mr));
> +return false;
> +}
> +}
> +
> /* Treat zero as compatibility all valid */
> if (!mr->ops->valid.max_access_size) {
> return true;
> @@ -1413,6 +1427,34 @@ bool memory_region_access_valid(MemoryRegion *mr,
> return true;
> }
>
> +static bool memory_region_access_start(MemoryRegion *mr,
> + hwaddr addr,
> + unsigned size,
> + bool is_write,
> + MemTxAttrs attrs)
> +{
> +if (!memory_region_access_valid(mr, addr, size, is_write, attrs)) {
> +return false;
> +}
> +
> +accessed_region_owners_num++;
> +if (accessed_region_owners_num > accessed_region_owners_capacity) {
> +accessed_region_owners_capacity = accessed_region_owners_num;
> +accessed_region_owners = g_realloc_n(accessed_region_owners,
> + accessed_region_owners_capacity,
> +
> sizeof(*accessed_region_owners));
> +}
> +
> +accessed_region_owners[accessed_region_owners_num - 1] = mr->owner;
> +
> +return true;
> +}
> +
> +static void memory_region_access_end(void)
> +{
> +accessed_region_owners_num--;
> +}
> +
> static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
> hwaddr addr,
> uint64_t *pval,
> @@ -1450,12 +1492,13 @@ MemTxResult memory_region_dispatch_read(MemoryRegion
> *mr,
> mr->alias_offset + addr,
> pval, op, attrs);
> }
> -if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
> +if (!memory_region_access_start(mr, addr, size, false, attrs)) {
> *pval = unassigned_mem_read(mr, addr, size);
> return MEMTX_DECODE_ERROR;
> }
>
> r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
> +memory_region_access_end();
> adjust_endianness(mr, pval, op);
> return r;
> }
> @@ -1493,13 +1536,14 @@ MemTxResult memory_region_dispatch_write(MemoryRegion
> *mr,
> MemTxAttrs attrs)
> {
> unsigned size = memop_size(op);
> +MemTxResult result;
>
> if (mr->alias) {
> return memory_region_dispatch_write(mr->alias,
>
Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues
On 230313 1608, Peter Maydell wrote: > On Mon, 13 Mar 2023 at 15:41, Philippe Mathieu-Daudé > wrote: > > Now I wonder again if this is a good time to merge this change set. > > No, I don't think it is at this point in the release > cycle. I would vote for merging it when we reopen for 8.1, > so that we'll have a full cycle to find all the weird corner > cases that it breaks. Ok. I'll fix the rasbpi issue and look into adding some compile-time option to make the re-entrancy check fatal. That way we might catch additional edge-cases with fuzzing (though non-x86 coverage is poor) and unit-tests. -Alex
Re: [PATCH v7 0/6] memory: prevent dma-reentracy issues
On 230313 1502, Thomas Huth wrote: > On 13/03/2023 09.24, Alexander Bulekov wrote: > > v6 -> v7: > > - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4) > > - Add an MR-specific flag to disable reentrancy (Patch 5) > > - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6) > > Patches 5 and 6 need review. I left the review-tags for Patch 4, > > however a few of the qemu_bh_new_guarded calls have changed. > > Hi Alexander, > > there seems to be another issue with one of the avocado tests: > > make -j8 qemu-system-aarch64 > make check-venv > ./tests/venv/bin/avocado run \ > > tests/avocado/boot_linux_console.py:BootLinuxConsole.test_aarch64_raspi3_atf > > ... works fine for me with the master branch, but it fails > for me after applying your patch series. > Can you reproduce that failure? #0 __GI_exit (status=0x1) at ./stdlib/exit.c:143 #1 0x55f05819 in access_with_adjusted_size (addr=0x0, addr@entry=0x73b609d0, value=0x73b609d0, size=size@entry=0x4, access_size_min=0x1, access_size_max=0x4, access_fn=0x55f0b4b0 , mr=0x7 #2 0x55f05380 in memory_region_dispatch_read1 (mr=0x73e34990, addr=0x1, pval=, size=0x4, attrs=...) at ../softmmu/memory.c:1442 #3 memory_region_dispatch_read (mr=, mr@entry=0x73e34990, addr=0x1, pval=, pval@entry=0x73b609d0, op=, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1476 #4 0x55f1278f in address_space_ldl_internal (as=, addr=, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:41 #5 0x559ebb5d in ldl_le_phys (as=0x73e35258, addr=0x80) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:79 #6 bcm2835_mbox_update (s=0x73e34f20) at ../hw/misc/bcm2835_mbox.c:109 #7 0x559ecd5d in bcm2835_property_write (opaque=0x73e34600, offset=, value=, size=) at ../hw/misc/bcm2835_property.c:349 #8 0x55f05903 in memory_region_write_accessor (mr=0x73e34990, addr=0x0, value=, size=0x4, shift=, mask=, attrs=...) at ../softmmu/memory.c:493 #9 0x55f0576b in access_with_adjusted_size (addr=addr@entry=0x0, value=0x73b60c38, value@entry=0x73b60c28, size=size@entry=0x4, access_size_min=, access_size_max=, access_fn=0x55f05820 < attrs=...) at ../softmmu/memory.c:570 #10 0x55f055c6 in memory_region_dispatch_write (mr=, mr@entry=0x73e34990, addr=0x0, data=, data@entry=0x2f2228, op=, attrs=..., attrs@entry=...) at ../softmmu/memory.c:1532 #11 0x55f132ec in address_space_stl_internal (as=, addr=, val=0x2f2228, attrs=..., result=0x0, endian=DEVICE_LITTLE_ENDIAN) at ../memory_ldst.c.inc:319 #12 0x559eb9a4 in stl_le_phys (as=, addr=0x80, val=0x2f2228) at /home/alxndr/Development/qemu-demo/qemu/include/exec/memory_ldst_phys.h.inc:121 #13 bcm2835_mbox_write (opaque=0x73e34f20, offset=, value=0x2f2228, size=) at ../hw/misc/bcm2835_mbox.c:227 #14 0x55f05903 in memory_region_write_accessor (mr=0x73e352b0, addr=0xa0, value=, size=0x4, shift=, mask=, attrs=...) at ../softmmu/memory.c:493 #15 0x55f0576b in access_with_adjusted_size (addr=addr@entry=0xa0, value=0x73b60e48, value@entry=0x73b60e38, size=size@entry=0x4, access_size_min=, access_size_max=, access_fn=0x55f05820 attrs=...) at ../softmmu/memory.c:570 #16 0x55f055c6 in memory_region_dispatch_write (mr=, mr@entry=0x2, addr=addr@entry=0xa0, data=, data@entry=0x2f2228, op=, op@entry=MO_32, attrs=...) at ../softmmu/memory.c:1532 #17 0x55f9b3ae in io_writex (env=0x73dd60e0, full=0x5790c710, mmu_idx=0x7, val=0x4, val@entry=0x2f2228, addr=0x3f00b8a0, retaddr=retaddr@entry=0x7fffac01f9dd, op=MO_32) at ../accel/tcg/cputlb.c:1430 #18 0x55f90062 in store_helper (env=, addr=, val=0x2f2228, oi=, retaddr=0x73b609d0, op=MO_32) at ../accel/tcg/cputlb.c:2454 #19 full_le_stl_mmu (env=, addr=, val=0x2f2228, oi=, retaddr=0x73b609d0) at ../accel/tcg/cputlb.c:2542 #20 0x7fffac01f9dd in code_gen_buffer () #21 0x55f7367e in cpu_tb_exec (cpu=cpu@entry=0x73dd4210, itb=itb@entry=0x7fffac01f8c0 , tb_exit=tb_exit@entry=0x73b6148c) at ../accel/tcg/cpu-exec.c:460 #22 0x55f744f9 in cpu_loop_exec_tb (cpu=0x73dd4210, tb=, pc=, tb_exit=0x73b6148c, last_tb=) at ../accel/tcg/cpu-exec.c:894 #23 cpu_exec_loop (cpu=cpu@entry=0x73dd4210, sc=sc@entry=0x73b61510) at ../accel/tcg/cpu-exec.c:1005 #24 0x55f73c27 in cpu_exec_setjmp (cpu=cpu@entry=0x73dd4210, sc=sc@entry=0x73b61510) at ../accel/tcg/cpu-exec.c:1037 #25 0x55f73aee in cpu_exec (cpu=cpu@entry=0x73dd4210) at ../accel/tcg/cpu-exec.c:1063 #26 0x55f9da4f in tcg_cpus_exec (cpu=cpu@entry=0x73dd4210) at ../accel/tcg/tcg-accel-ops.c:81 #27 0x55f9e019 in mttcg_cpu_thread_fn (arg=arg@entry=0x73dd4210)
Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues
On 230313 0515, Alexander Bulekov wrote: > > > > At this point I'm not sure anymore this is a device or MR property. > > It's designed to be an MR property. If it were MR specific, it wouldn't Should be "It's designed to be a Device property."
Re: [PATCH v7 1/6] memory: prevent dma-reentracy issues
On 230313 0945, Philippe Mathieu-Daudé wrote:
> Hi Alex,
>
> Sorry for the late review, *sigh*.
>
> On 13/3/23 09:24, Alexander Bulekov wrote:
> > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> > This flag is set/checked prior to calling a device's MemoryRegion
> > handlers, and set when device code initiates DMA. The purpose of this
> > flag is to prevent two types of DMA-based reentrancy issues:
> >
> > 1.) mmio -> dma -> mmio case
> > 2.) bh -> dma write -> mmio case
> >
> > These issues have led to problems such as stack-exhaustion and
> > use-after-frees.
> >
> > Summary of the problem from Peter Maydell:
> > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> >
> > Reviewed-by: Darren Kenny
> > Reviewed-by: Stefan Hajnoczi
> > Signed-off-by: Alexander Bulekov
> > Acked-by: Peter Xu
> > ---
> > include/hw/qdev-core.h | 7 +++
> > softmmu/memory.c | 17 +
> > softmmu/trace-events | 1 +
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index bd50ad5ee1..7623703943 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -162,6 +162,10 @@ struct NamedClockList {
> > QLIST_ENTRY(NamedClockList) node;
> > };
> > +typedef struct {
> > +bool engaged_in_io;
>
> Do you plan to add more fields?
Not right now, but maybe some need will come up.
> > +} MemReentrancyGuard;
> > +
> > /**
> >* DeviceState:
> >* @realized: Indicates whether the device has been fully constructed.
> > @@ -194,6 +198,9 @@ struct DeviceState {
> > int alias_required_for_version;
> > ResettableState reset;
> > GSList *unplug_blockers;
> > +
> > +/* Is the device currently in mmio/pio/dma? Used to prevent
> > re-entrancy */
> > +MemReentrancyGuard mem_reentrancy_guard;
>
> At this point I'm not sure anymore this is a device or MR property.
It's designed to be an MR property. If it were MR specific, it wouldn't
handle the BH -> DMA case, or this one, where there are two MRs (doorbell
and oper) involed.
https://gitlab.com/qemu-project/qemu/-/issues/540
>
> > };
> > struct DeviceListener {
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 4699ba55ec..57bf18a257 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -533,6 +533,7 @@ static MemTxResult access_with_adjusted_size(hwaddr
> > addr,
> > uint64_t access_mask;
> > unsigned access_size;
> > unsigned i;
> > +DeviceState *dev = NULL;
> > MemTxResult r = MEMTX_OK;
> > if (!access_size_min) {
> > @@ -542,6 +543,19 @@ static MemTxResult access_with_adjusted_size(hwaddr
> > addr,
> > access_size_max = 4;
> > }
> > +/* Do not allow more than one simultanous access to a device's IO
> > Regions */
>
> Typo "simultaneous".
>
> 1/ access_with_adjusted_size() is complex enough and we are having hard
>time getting it right. I'd prefer we don't intermix size adjustment
>and re-entrancy check in the same function. This check could belong
>to the callers.
>
Would moving the code within this function to keep it separate from the
size adjustment be good enough? Otherwise we would end up with duplicate
code in the read/write callers.
The size-adjustment seems to be orthogonal (the MR won't change)?
> 2/ I'm not keen on calling QOM object_dynamic_cast() in this hot path;
>and mixing QDev API within MR one. At least, can we cache this value
>once in memory_region_do_init() since we have access to @owner?
>
Sounds like a good idea. Is it ever possible for the owner/owner's
address to change?
Thanks
-Alex
> > +if (mr->owner &&
> > +!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
> > +dev = (DeviceSt
[PATCH v7 1/6] memory: prevent dma-reentracy issues
Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
This flag is set/checked prior to calling a device's MemoryRegion
handlers, and set when device code initiates DMA. The purpose of this
flag is to prevent two types of DMA-based reentrancy issues:
1.) mmio -> dma -> mmio case
2.) bh -> dma write -> mmio case
These issues have led to problems such as stack-exhaustion and
use-after-frees.
Summary of the problem from Peter Maydell:
https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Signed-off-by: Alexander Bulekov
Acked-by: Peter Xu
---
include/hw/qdev-core.h | 7 +++
softmmu/memory.c | 17 +
softmmu/trace-events | 1 +
3 files changed, 25 insertions(+)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..7623703943 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -162,6 +162,10 @@ struct NamedClockList {
QLIST_ENTRY(NamedClockList) node;
};
+typedef struct {
+bool engaged_in_io;
+} MemReentrancyGuard;
+
/**
* DeviceState:
* @realized: Indicates whether the device has been fully constructed.
@@ -194,6 +198,9 @@ struct DeviceState {
int alias_required_for_version;
ResettableState reset;
GSList *unplug_blockers;
+
+/* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+MemReentrancyGuard mem_reentrancy_guard;
};
struct DeviceListener {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 4699ba55ec..57bf18a257 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -533,6 +533,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
uint64_t access_mask;
unsigned access_size;
unsigned i;
+DeviceState *dev = NULL;
MemTxResult r = MEMTX_OK;
if (!access_size_min) {
@@ -542,6 +543,19 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_size_max = 4;
}
+/* Do not allow more than one simultanous access to a device's IO Regions
*/
+if (mr->owner &&
+!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
+dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
+if (dev) {
+if (dev->mem_reentrancy_guard.engaged_in_io) {
+trace_memory_region_reentrant_io(get_cpu_index(), mr, addr,
size);
+return MEMTX_ERROR;
+}
+dev->mem_reentrancy_guard.engaged_in_io = true;
+}
+}
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +570,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+if (dev) {
+dev->mem_reentrancy_guard.engaged_in_io = false;
+}
return r;
}
diff --git a/softmmu/trace-events b/softmmu/trace-events
index 22606dc27b..62d04ea9a7 100644
--- a/softmmu/trace-events
+++ b/softmmu/trace-events
@@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t
addr, uint64_t value, u
memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value
0x%"PRIx64" size %u name '%s'"
memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t
value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size
%u"
+memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, unsigned
size) "cpu %d mr %p offset 0x%"PRIx64" size %u"
memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t
value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr,
uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64"
size %u"
memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr
'%s' listener '%s' synced (global=%d)"
--
2.39.0
[PATCH v7 3/6] checkpatch: add qemu_bh_new/aio_bh_new checks
Advise authors to use the _guarded versions of the APIs, instead.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
scripts/checkpatch.pl | 8
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers;
signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of
qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new()
to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc.
instead of module_init()\n" . $herecurr);
--
2.39.0
[PATCH v7 5/6] memory: Allow disabling re-entrancy checking per-MR
Signed-off-by: Alexander Bulekov
---
include/exec/memory.h | 3 +++
softmmu/memory.c | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6fa0b071f0..5154b123d8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -791,6 +791,9 @@ struct MemoryRegion {
unsigned ioeventfd_nb;
MemoryRegionIoeventfd *ioeventfds;
RamDiscardManager *rdm; /* Only for RAM */
+
+/* For devices designed to perform re-entrant IO into their own IO MRs */
+bool disable_reentrancy_guard;
};
struct IOMMUMemoryRegion {
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 57bf18a257..3018fa2edb 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
}
/* Do not allow more than one simultanous access to a device's IO Regions
*/
-if (mr->owner &&
+if (mr->owner && !mr->disable_reentrancy_guard &&
!mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
if (dev) {
--
2.39.0
[PATCH v7 6/6] lsi53c895a: disable reentrancy detection for script RAM
As the code is designed to use the memory APIs to access the script ram, disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion. In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion. Reported-by: Fiona Ebner Signed-off-by: Alexander Bulekov --- hw/scsi/lsi53c895a.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c index af93557a9a..db27872963 100644 --- a/hw/scsi/lsi53c895a.c +++ b/hw/scsi/lsi53c895a.c @@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp) memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s, "lsi-io", 256); +/* + * Since we use the address-space API to interact with ram_io, disable the + * re-entrancy guard. + */ +s->ram_io.disable_reentrancy_guard = true; + address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io"); qdev_init_gpio_out(d, &s->ext_irq, 1); -- 2.39.0
[PATCH v7 4/6] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth for diagnosing OS X test failure.
Reviewed-by: Darren Kenny
Reviewed-by: Stefan Hajnoczi
Reviewed-by: Michael S. Tsirkin
Reviewed-by: Paul Durrant
Signed-off-by: Alexander Bulekov
---
hw/9pfs/xen-9p-backend.c| 5 -
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c| 9 ++---
hw/display/virtio-gpu.c | 6 --
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 --
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 --
hw/scsi/mptsas.c| 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c| 3 ++-
hw/usb/dev-uas.c| 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c| 6 --
hw/usb/redirect.c | 6 --
hw/usb/xen-usb.c| 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
-xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh,
&xen_9pdev->rings[i]);
+xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+
&xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev,
VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
-s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice
*xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
-dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
+port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index ec712d3ca2..c0460c4ef1 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
-qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
-qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
[PATCH v7 2/6] async: Add an optional reentrancy guard to the BH API
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny
Signed-off-by: Alexander Bulekov
---
docs/devel/multiple-iothreads.txt | 7 +++
include/block/aio.h | 18 --
include/qemu/main-loop.h | 7 +--
tests/unit/ptimer-test-stubs.c| 3 ++-
util/async.c | 18 +-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt
b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see
include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index 8fba6a3584..3e3bdb9352 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -331,9 +333,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx,
QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
-const char *name);
+const char *name, MemReentrancyGuard
*reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -342,7 +346,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb,
void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
-aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index c25f390696..84d1ce57f0 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -389,9 +389,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
void qemu_fd_register(int fd);
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
-qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int
attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opa
[PATCH v7 0/6] memory: prevent dma-reentracy issues
v6 -> v7: - Fix bad qemu_bh_new_guarded calls found by Thomas (Patch 4) - Add an MR-specific flag to disable reentrancy (Patch 5) - Disable reentrancy checks for lsi53c895a's RAM-like MR (Patch 6) Patches 5 and 6 need review. I left the review-tags for Patch 4, however a few of the qemu_bh_new_guarded calls have changed. v5 -> v6: - Only apply checkpatch checks to code in paths containing "/hw/" (/hw/ and include/hw/) - Fix a bug in a _guarded call added to hw/block/virtio-blk.c v4-> v5: - Add corresponding checkpatch checks - Save/restore reentrancy-flag when entering/exiting BHs - Improve documentation - Check object_dynamic_cast return value v3 -> v4: Instead of changing all of the DMA APIs, instead add an optional reentrancy guard to the BH API. v2 -> v3: Bite the bullet and modify the DMA APIs, rather than attempting to guess DeviceStates in BHs. These patches aim to solve two types of DMA-reentrancy issues: 1.) mmio -> dma -> mmio case To solve this, we track whether the device is engaged in io by checking/setting a reentrancy-guard within APIs used for MMIO access. 2.) bh -> dma write -> mmio case This case is trickier, since we dont have a generic way to associate a bh with the underlying Device/DeviceState. Thus, this version allows a device to associate a reentrancy-guard with a bh, when creating it. (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded) I replaced most of the qemu_bh_new invocations with the guarded analog, except for the ones where the DeviceState was not trivially accessible. Alexander Bulekov (6): memory: prevent dma-reentracy issues async: Add an optional reentrancy guard to the BH API checkpatch: add qemu_bh_new/aio_bh_new checks hw: replace most qemu_bh_new calls with qemu_bh_new_guarded memory: Allow disabling re-entrancy checking per-MR lsi53c895a: disable reentrancy detection for script RAM docs/devel/multiple-iothreads.txt | 7 +++ hw/9pfs/xen-9p-backend.c | 5 - hw/block/dataplane/virtio-blk.c | 3 ++- hw/block/dataplane/xen-block.c| 5 +++-- hw/char/virtio-serial-bus.c | 3 ++- hw/display/qxl.c | 9 ++--- hw/display/virtio-gpu.c | 6 -- hw/ide/ahci.c | 3 ++- hw/ide/ahci_internal.h| 1 + hw/ide/core.c | 4 +++- hw/misc/imx_rngc.c| 6 -- hw/misc/macio/mac_dbdma.c | 2 +- hw/net/virtio-net.c | 3 ++- hw/nvme/ctrl.c| 6 -- hw/scsi/lsi53c895a.c | 6 ++ hw/scsi/mptsas.c | 3 ++- hw/scsi/scsi-bus.c| 3 ++- hw/scsi/vmw_pvscsi.c | 3 ++- hw/usb/dev-uas.c | 3 ++- hw/usb/hcd-dwc2.c | 3 ++- hw/usb/hcd-ehci.c | 3 ++- hw/usb/hcd-uhci.c | 2 +- hw/usb/host-libusb.c | 6 -- hw/usb/redirect.c | 6 -- hw/usb/xen-usb.c | 3 ++- hw/virtio/virtio-balloon.c| 5 +++-- hw/virtio/virtio-crypto.c | 3 ++- include/block/aio.h | 18 -- include/exec/memory.h | 3 +++ include/hw/qdev-core.h| 7 +++ include/qemu/main-loop.h | 7 +-- scripts/checkpatch.pl | 8 softmmu/memory.c | 17 + softmmu/trace-events | 1 + tests/unit/ptimer-test-stubs.c| 3 ++- util/async.c | 18 +- util/main-loop.c | 5 +++-- util/trace-events | 1 + 38 files changed, 159 insertions(+), 41 deletions(-) -- 2.39.0
Re: [PATCH v2] virtio: fix reachable assertion due to stale value of cached region sizey
On 230302 1103, Carlos López wrote:
> In virtqueue_{split,packed}_get_avail_bytes() descriptors are read
> in a loop via MemoryRegionCache regions and calls to
> vring_{split,packed}_desc_read() - these take a region cache and the
> index of the descriptor to be read.
>
> For direct descriptors we use a cache provided by the caller, whose
> size matches that of the virtqueue vring. We limit the number of
> descriptors we can read by the size of that vring:
>
> max = vq->vring.num;
> ...
> MemoryRegionCache *desc_cache = &caches->desc;
>
> For indirect descriptors, we initialize a new cache and limit the
> number of descriptors by the size of the intermediate descriptor:
>
> len = address_space_cache_init(&indirect_desc_cache,
>vdev->dma_as,
>desc.addr, desc.len, false);
> desc_cache = &indirect_desc_cache;
> ...
> max = desc.len / sizeof(VRingDesc);
>
> However, the first initialization of `max` is done outside the loop
> where we process guest descriptors, while the second one is done
> inside. This means that a sequence of an indirect descriptor followed
> by a direct one will leave a stale value in `max`. If the second
> descriptor's `next` field is smaller than the stale value, but
> greater than the size of the virtqueue ring (and thus the cached
> region), a failed assertion will be triggered in
> address_space_read_cached() down the call chain.
>
> Fix this by initializing `max` inside the loop in both functions.
>
> Fixes: 9796d0ac8fb0 ("virtio: use address_space_map/unmap to access
> descriptors")
> Signed-off-by: Carlos López
OSS-Fuzz confirmed this fixed an issue:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=52152
Never made a gitlab report for it because, it seemed very similar to
some other issues
https://gitlab.com/qemu-project/qemu/-/issues/301
https://gitlab.com/qemu-project/qemu/-/issues/781
https://gitlab.com/qemu-project/qemu/-/issues/300
But apparently it was a different one.
Thank you
-Alex
Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues
On 230310 0802, Alexander Bulekov wrote: > On 230310 1245, Peter Maydell wrote: > > On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov wrote: > > > This MR seems to be "lsi-ram". > > > > > > From hw/scsi/lsi53c895a.c: > > > > > > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, > > > "lsi-ram", 0x2000); > > > > > > So the LSI device is reading an LSI "Script" from its own IO region.. In > > > this particular case, I think there was no reason to use > > > memory_region_init_io rather than memory_region_init_ram, but this makes > > > me worried that there are other devices that use something like this. > > > > This particular device predates the entire MemoryRegion set of > > abstractions, so it might have seemed easier at the time. > > The endianness handling of the current code is also a bit > > confusing and might make it tricky to convert to a RAM MR. > > With my trivial mr_io - > mr_ram conversion, I no longer hit the > re-entrancy tracepoint, and my livecd boots, but it's probably not > exhaustively using the script functionality.. > > Does the endianness actually cause a problem? As long as all accesses > (CPU -> LSI_RAM and LSI -> LSI_RAM) occur through the address_space API, > are we safe? > -Alex Or maybe a rom_device with memory_region_rom_device_set_romd(romd_mode = false) is better here?
Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues
On 230310 1245, Peter Maydell wrote: > On Fri, 10 Mar 2023 at 12:32, Alexander Bulekov wrote: > > This MR seems to be "lsi-ram". > > > > From hw/scsi/lsi53c895a.c: > > > > memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, > > "lsi-ram", 0x2000); > > > > So the LSI device is reading an LSI "Script" from its own IO region.. In > > this particular case, I think there was no reason to use > > memory_region_init_io rather than memory_region_init_ram, but this makes > > me worried that there are other devices that use something like this. > > This particular device predates the entire MemoryRegion set of > abstractions, so it might have seemed easier at the time. > The endianness handling of the current code is also a bit > confusing and might make it tricky to convert to a RAM MR. With my trivial mr_io - > mr_ram conversion, I no longer hit the re-entrancy tracepoint, and my livecd boots, but it's probably not exhaustively using the script functionality.. Does the endianness actually cause a problem? As long as all accesses (CPU -> LSI_RAM and LSI -> LSI_RAM) occur through the address_space API, are we safe? -Alex
Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues
On 230310 0723, Alexander Bulekov wrote: > On 230310 1214, Fiona Ebner wrote: > > Am 05.02.23 um 05:07 schrieb Alexander Bulekov: > > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > > This flag is set/checked prior to calling a device's MemoryRegion > > > handlers, and set when device code initiates DMA. The purpose of this > > > flag is to prevent two types of DMA-based reentrancy issues: > > > > > > 1.) mmio -> dma -> mmio case > > > 2.) bh -> dma write -> mmio case > > > > > > These issues have led to problems such as stack-exhaustion and > > > use-after-frees. > > > > > > Summary of the problem from Peter Maydell: > > > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com > > > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827 > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282 > > > > > > Reviewed-by: Darren Kenny > > > Reviewed-by: Stefan Hajnoczi > > > Signed-off-by: Alexander Bulekov > > > Acked-by: Peter Xu > > > --- > > > include/hw/qdev-core.h | 7 +++ > > > softmmu/memory.c | 17 + > > > softmmu/trace-events | 1 + > > > 3 files changed, 25 insertions(+) > > > > > Hi, > > there seems to be an issue with this patch or existing issue exposed by > > this patch in combination with the LSI SCSI controller. > > > > After applying this patch on current master (i.e. > > ee59483267de29056b5b2ee2421ef3844e5c9932), a Debian 11 with the LSI > > controller would not boot properly anymore: > > > [7.540907] sym0: <895a> rev 0x0 at pci :00:05.0 irq 10 > > > [7.546028] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking > > > [7.559724] sym0: SCSI BUS has been reset. > > > [7.560820] sym0: interrupted SCRIPT address not found. > > > [7.563802] scsi host2: sym-2.2.3 > > > [7.88] e1000 :00:03.0 eth0: (PCI:33MHz:32-bit) > > > 52:54:00:12:34:56 > > > [7.881998] e1000 :00:03.0 eth0: Intel(R) PRO/1000 Network > > > Connection > > > [7.925902] e1000 :00:03.0 ens3: renamed from eth0 > > > [ 32.654811] scsi 2:0:0:0: tag#192 ABORT operation started > > > [ 37.764283] scsi 2:0:0:0: ABORT operation timed-out. > > > [ 37.774974] scsi 2:0:0:0: tag#192 DEVICE RESET operation started > > > [ 42.882488] scsi 2:0:0:0: DEVICE RESET operation timed-out. > > > [ 42.883606] scsi 2:0:0:0: tag#192 BUS RESET operation started > > > [ 48.002437] scsi 2:0:0:0: BUS RESET operation timed-out. > > > [ 48.003030] scsi 2:0:0:0: tag#192 HOST RESET operation started > > > [ 48.010226] sym0: SCSI BUS has been reset. > > > [ 53.122472] scsi 2:0:0:0: HOST RESET operation timed-out. > > > [ 53.123030] scsi 2:0:0:0: Device offlined - not ready after error > > > recovery > > > > The commandline I used is: > > ./qemu-system-x86_64 \ > >-cpu 'kvm64' \ > >-m 4096 \ > >-serial 'stdio' \ > >-device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \ > >-drive > > 'file=/dev/zvol/myzpool/vm-9006-disk-0,if=none,id=drive-scsi0,format=raw' \ > >-device > > 'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0,bootindex=100' \ > >-machine 'pc' > > > > Happy to provide any more information if necessary! > > > > CC-ing Fam Zheng (reviewer:SCSI) > > > > Originally reported by one of our community members: > > https://forum.proxmox.com/threads/123843/ > > > > Best Regards, > > Fiona > > > > Thanks, I confirmed this by booting up a livecd iso with an lsi device > attached. I will do some digging > > Stack-trace: > > #0 trace_memory_region_reentrant_io (cpu_index=, > mr=, offset=, size=) at > trace/trace-softmmu.h:337 > #1 0x5815ce67 in access_with_adjusted_size (addr=addr@entry=0x1000, > value=0x7ffef01fb980, size=size@entry=0x4, access_size_min=0x1, > access_size_min@entry=0x0, access_size_max=0x4, access_fn=0x58181370 > , mr=0x62700c50, attrs=... > ) at ../softmmu/memory.c:552 > #2 0x5815aec7 in memory_region_dispatch_read1 (mr=0x62700c50, > addr=0x1000, pval=, size=0x4, attrs=...) at > ../softmmu/memory.c:1448 This MR seems to be "lsi-ram". >From hw/scsi/lsi53c895a.c: memory_region_init_io(&s->ram_io, OBJECT(s), &lsi_ram_ops, s, "lsi-ram", 0x2000); So the LSI device is reading an LSI "Script" from its own IO region.. In this particular case, I think there was no reason to use memory_region_init_io rather than memory_region_init_ram, but this makes me worried that there are other devices that use something like this. -Alex
Re: [PATCH v6 1/4] memory: prevent dma-reentracy issues
On 230310 1214, Fiona Ebner wrote: > Am 05.02.23 um 05:07 schrieb Alexander Bulekov: > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > This flag is set/checked prior to calling a device's MemoryRegion > > handlers, and set when device code initiates DMA. The purpose of this > > flag is to prevent two types of DMA-based reentrancy issues: > > > > 1.) mmio -> dma -> mmio case > > 2.) bh -> dma write -> mmio case > > > > These issues have led to problems such as stack-exhaustion and > > use-after-frees. > > > > Summary of the problem from Peter Maydell: > > https://lore.kernel.org/qemu-devel/cafeaca_23vc7he3iam-jva6w38lk4hjowae5kcknhprd5fp...@mail.gmail.com > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827 > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282 > > > > Reviewed-by: Darren Kenny > > Reviewed-by: Stefan Hajnoczi > > Signed-off-by: Alexander Bulekov > > Acked-by: Peter Xu > > --- > > include/hw/qdev-core.h | 7 +++ > > softmmu/memory.c | 17 + > > softmmu/trace-events | 1 + > > 3 files changed, 25 insertions(+) > > > Hi, > there seems to be an issue with this patch or existing issue exposed by > this patch in combination with the LSI SCSI controller. > > After applying this patch on current master (i.e. > ee59483267de29056b5b2ee2421ef3844e5c9932), a Debian 11 with the LSI > controller would not boot properly anymore: > > [7.540907] sym0: <895a> rev 0x0 at pci :00:05.0 irq 10 > > [7.546028] sym0: No NVRAM, ID 7, Fast-40, LVD, parity checking > > [7.559724] sym0: SCSI BUS has been reset. > > [7.560820] sym0: interrupted SCRIPT address not found. > > [7.563802] scsi host2: sym-2.2.3 > > [7.88] e1000 :00:03.0 eth0: (PCI:33MHz:32-bit) 52:54:00:12:34:56 > > [7.881998] e1000 :00:03.0 eth0: Intel(R) PRO/1000 Network Connection > > [7.925902] e1000 :00:03.0 ens3: renamed from eth0 > > [ 32.654811] scsi 2:0:0:0: tag#192 ABORT operation started > > [ 37.764283] scsi 2:0:0:0: ABORT operation timed-out. > > [ 37.774974] scsi 2:0:0:0: tag#192 DEVICE RESET operation started > > [ 42.882488] scsi 2:0:0:0: DEVICE RESET operation timed-out. > > [ 42.883606] scsi 2:0:0:0: tag#192 BUS RESET operation started > > [ 48.002437] scsi 2:0:0:0: BUS RESET operation timed-out. > > [ 48.003030] scsi 2:0:0:0: tag#192 HOST RESET operation started > > [ 48.010226] sym0: SCSI BUS has been reset. > > [ 53.122472] scsi 2:0:0:0: HOST RESET operation timed-out. > > [ 53.123030] scsi 2:0:0:0: Device offlined - not ready after error > > recovery > > The commandline I used is: > ./qemu-system-x86_64 \ >-cpu 'kvm64' \ >-m 4096 \ >-serial 'stdio' \ >-device 'lsi,id=scsihw0,bus=pci.0,addr=0x5' \ >-drive > 'file=/dev/zvol/myzpool/vm-9006-disk-0,if=none,id=drive-scsi0,format=raw' \ >-device > 'scsi-hd,bus=scsihw0.0,scsi-id=0,drive=drive-scsi0,id=scsi0,bootindex=100' \ >-machine 'pc' > > Happy to provide any more information if necessary! > > CC-ing Fam Zheng (reviewer:SCSI) > > Originally reported by one of our community members: > https://forum.proxmox.com/threads/123843/ > > Best Regards, > Fiona > Thanks, I confirmed this by booting up a livecd iso with an lsi device attached. I will do some digging Stack-trace: #0 trace_memory_region_reentrant_io (cpu_index=, mr=, offset=, size=) at trace/trace-softmmu.h:337 #1 0x5815ce67 in access_with_adjusted_size (addr=addr@entry=0x1000, value=0x7ffef01fb980, size=size@entry=0x4, access_size_min=0x1, access_size_min@entry=0x0, access_size_max=0x4, access_fn=0x58181370 , mr=0x62700c50, attrs=... ) at ../softmmu/memory.c:552 #2 0x5815aec7 in memory_region_dispatch_read1 (mr=0x62700c50, addr=0x1000, pval=, size=0x4, attrs=...) at ../softmmu/memory.c:1448 #3 memory_region_dispatch_read (mr=0x62700c50, addr=0x1000, pval=, op=, attrs=...) at ../softmmu/memory.c:1475 #4 0x5819eb77 in flatview_read_continue (fv=, addr=0xfebf1000, attrs=..., ptr=, len=0x4, addr1=0x62700c50, l=, mr=) at ../softmmu/physmem.c:2893 #5 0x5819f844 in flatview_
Re: [PATCH v6 4/4] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
[[ CCing qemu-devel in case someone can spot something wrong faster than me]] On 230308 1042, Thomas Huth wrote: [snip] > > > I'd really love to see this series included in QEMU 8.0, so to help with > > > testing a little bit, I've put it in my gitlab-CI for testing. However, it > > > hit a segfault in the macOS runner: > > > > > > https://gitlab.com/thuth/qemu/-/jobs/3889796931#L4757 > > > > > > Do you have an idea what might be going wrong here? > > > > > > > Unfortunately I wasn't able to reproduce this on x86_64 linux and I > > don't have a mac to test on. I will try to comb through the code in > > patch 4/4 as that seams like the most likely culprit. > > Yes, you're right, it's the final patch that is causing the problem. I've > pushed the branch without the final patch, and here it was still working > fine: > > https://gitlab.com/thuth/qemu/-/jobs/3893883020 > > I also don't have a mac for testing, but cirrus-ci recently started to offer > terminal access to the jobs ... so if you've got a github account, you could > fork the qemu repo there and enable the cirrus-ci for it in the marketplace. > See .gitlab-ci.d/cirrus/README.rst for how to trigger it via the gitlab-CI. > Or if that's too complicated right now, let me know if I can test something > for you in my setup. The detailed cirrus logs indicate that the last test in bios-tables-test was /x86_64/acpi/q35/multif-bridge and it started qemu with these devices: -display none -machine q35 -accel kvm -accel tcg -net none -S -device virtio-balloon,id=balloon0,addr=0x4.0x2 -device pcie-root-port,id=rp0,multifunction=on,port=0x0,chassis =1,addr=0x2 -device pcie-root-port,id=rp1,port=0x1,chassis=2,addr=0x3.0x1 -device pcie-root-port,id=rp2,port=0x0,chassis=3,bus=rp1,addr=0.0 -device pci-bridge,bus=rp2,chassis_nr=4,id=br1 -device pcie-root-port,id=rphptgt1, port=0x0,chassis=5,addr=2.1 -device pcie-root-port,id=rphptgt2,port=0x0,chassis=6,addr=2.2 -device pcie-root-port,id=rphptgt3,port=0x0,chassis=7,addr=2.3 -device pci-testdev,bus=pcie.0,addr=2.4 -device pci-testdev,bus=pcie .0,addr=5.0 -device pci-testdev,bus=rp0,addr=0.0 -device pci-testdev,bus=br1 -drive id=hd0,if=none,file=tests/acpi-test-disk-0q5LlN,format=raw -device ide-hd,drive=hd0 using these identical arguments I still could not reproduce the issue (replacing the test disk). The test attaches a few devices but it doesn't seem like they would be related. Out of the listed devices, the ones that were touched in patch 4 seem to be virtio-balloon and ide. I took at quick look at the corresponding changes and didn't immediately notice something wrong, but I'll keep digging. Still have no clue why it would only show up on mac (I tried different compilers and asan). -Alex
Re: Is the fix for "DMA MMIO reentrancy" in qemu stable now?
On 230302 1627, byzero wrote: > Hi, > The bug class of MMIO reentrancy is fixed by adding a member "memory" in > the struct "MemTxAttrs", but the patch only exists in 7.x version, which is As a side-note, that patch doesn't fix the entire class of DMA-reentrnacy bugs. There are still active DMA reentrancy issues. I believe this is the closest we have to a "global" fix is which will hopefully make it into mainline qemu soon: https://lore.kernel.org/all/20230205040737.3567731-1-alx...@bu.edu/ > only release version, but not stable version. The latest stable version is > 6.1, and there is no stable version being released for nearly a year. > According to the docs(https://www.qemu.org/download/): "The stable trees > are located in branches named stable-X.YY branch, where X.YY is the release > version.". > So I want to know that if the patch is stable enough? Will the community > be possible to change the way for fixing the bug class? > thanks for reading this email.
Re: [PATCH v6 0/4] memory: prevent dma-reentracy issues
On 230216 1214, Thomas Huth wrote: > On 13/02/2023 03.11, Alexander Bulekov wrote: > > ping > > I think it would be really good to finally get these dma-reentrancy issues > fixed! Who's supposed to pick up these patches? Paolo? David? Peter? Ping > > Thomas
Re: [PATCH] tests: Ensure TAP version is printed before other messages
On 230227 1740, Richard W.M. Jones wrote: > These two tests were failing with this error: > > stderr: > TAP parsing error: version number must be on the first line > [...] > Unknown TAP version. The first line MUST be `TAP version `. Assuming > version 12. > > This can be fixed by ensuring we always call g_test_init first in the > body of main. > > Thanks: Daniel Berrange, for diagnosing the problem > Signed-off-by: Richard W.M. Jones Reviewed-by: Alexander Bulekov
Re: [PULL 02/10] fuzz: add fuzz_reset API
On 230217 1048, Darren Kenny wrote: > I know this is a pull request, but if you prefer to have all the patches > with and R-b, you can add mine here too, but I'll leave it up to you. > > Reviewed-by: Darren Kenny > Thank you Darren - I missed that this one wasn't reviewed..
[PULL 10/10] docs/fuzz: remove mentions of fork-based fuzzing
Signed-off-by: Alexander Bulekov Reviewed-by: Darren Kenny --- docs/devel/fuzzing.rst | 22 ++ 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/docs/devel/fuzzing.rst b/docs/devel/fuzzing.rst index 715330c856..3bfcb33fc4 100644 --- a/docs/devel/fuzzing.rst +++ b/docs/devel/fuzzing.rst @@ -19,11 +19,6 @@ responsibility to ensure that state is reset between fuzzing-runs. Building the fuzzers -*NOTE*: If possible, build a 32-bit binary. When forking, the 32-bit fuzzer is -much faster, since the page-map has a smaller size. This is due to the fact that -AddressSanitizer maps ~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). Here, enable-sanitizers, is optional but it allows us to reliably detect bugs @@ -296,10 +291,9 @@ input. It is also responsible for manually calling ``main_loop_wait`` to ensure that bottom halves are executed and any cleanup required before the next input. Since the same process is reused for many fuzzing runs, QEMU state needs to -be reset at the end of each run. There are currently two implemented -options for resetting state: +be reset at the end of each run. For example, this can be done by rebooting the +VM, after each run. -- Reboot the guest between runs. - *Pros*: Straightforward and fast for simple fuzz targets. - *Cons*: Depending on the device, does not reset all device state. If the @@ -308,15 +302,3 @@ options for resetting state: reboot. - *Example target*: ``i440fx-qtest-reboot-fuzz`` - -- Run each test case in a separate forked process and copy the coverage - information back to the parent. This is fairly similar to AFL's "deferred" - fork-server mode [3] - - - *Pros*: Relatively fast. Devices only need to be initialized once. No need to -do slow reboots or vmloads. - - - *Cons*: Not officially supported by libfuzzer. Does not work well for - devices that rely on dedicated threads. - - - *Example target*: ``virtio-net-fork-fuzz`` -- 2.39.0
[PULL 07/10] fuzz/virtio-blk: remove fork-based fuzzer
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/virtio_blk_fuzz.c | 51 --
1 file changed, 7 insertions(+), 44 deletions(-)
diff --git a/tests/qtest/fuzz/virtio_blk_fuzz.c
b/tests/qtest/fuzz/virtio_blk_fuzz.c
index a9fb9ecf6c..651fd4f043 100644
--- a/tests/qtest/fuzz/virtio_blk_fuzz.c
+++ b/tests/qtest/fuzz/virtio_blk_fuzz.c
@@ -19,7 +19,6 @@
#include "standard-headers/linux/virtio_pci.h"
#include "standard-headers/linux/virtio_blk.h"
#include "fuzz.h"
-#include "fork_fuzz.h"
#include "qos_fuzz.h"
#define TEST_IMAGE_SIZE (64 * 1024 * 1024)
@@ -128,48 +127,24 @@ static void virtio_blk_fuzz(QTestState *s,
QVirtioBlkQueues* queues,
}
}
-static void virtio_blk_fork_fuzz(QTestState *s,
-const unsigned char *Data, size_t Size)
-{
-QVirtioBlk *blk = fuzz_qos_obj;
-static QVirtioBlkQueues *queues;
-if (!queues) {
-queues = qvirtio_blk_init(blk->vdev, 0);
-}
-if (fork() == 0) {
-virtio_blk_fuzz(s, queues, Data, Size);
-flush_events(s);
-_Exit(0);
-} else {
-flush_events(s);
-wait(NULL);
-}
-}
-
static void virtio_blk_with_flag_fuzz(QTestState *s,
const unsigned char *Data, size_t Size)
{
QVirtioBlk *blk = fuzz_qos_obj;
static QVirtioBlkQueues *queues;
-if (fork() == 0) {
-if (Size >= sizeof(uint64_t)) {
-queues = qvirtio_blk_init(blk->vdev, *(uint64_t *)Data);
-virtio_blk_fuzz(s, queues,
- Data + sizeof(uint64_t), Size - sizeof(uint64_t));
-flush_events(s);
-}
-_Exit(0);
-} else {
+if (Size >= sizeof(uint64_t)) {
+queues = qvirtio_blk_init(blk->vdev, *(uint64_t *)Data);
+virtio_blk_fuzz(s, queues,
+Data + sizeof(uint64_t), Size - sizeof(uint64_t));
flush_events(s);
-wait(NULL);
}
+fuzz_reset(s);
}
static void virtio_blk_pre_fuzz(QTestState *s)
{
qos_init_path(s);
-counter_shm_init();
}
static void drive_destroy(void *path)
@@ -208,22 +183,10 @@ static void *virtio_blk_test_setup(GString *cmd_line,
void *arg)
static void register_virtio_blk_fuzz_targets(void)
{
-fuzz_add_qos_target(&(FuzzTarget){
-.name = "virtio-blk-fuzz",
-.description = "Fuzz the virtio-blk virtual queues, forking "
-"for each fuzz run",
-.pre_vm_init = &counter_shm_init,
-.pre_fuzz = &virtio_blk_pre_fuzz,
-.fuzz = virtio_blk_fork_fuzz,},
-"virtio-blk",
-&(QOSGraphTestOptions){.before = virtio_blk_test_setup}
-);
-
fuzz_add_qos_target(&(FuzzTarget){
.name = "virtio-blk-flags-fuzz",
-.description = "Fuzz the virtio-blk virtual queues, forking "
-"for each fuzz run (also fuzzes the virtio flags)",
-.pre_vm_init = &counter_shm_init,
+.description = "Fuzz the virtio-blk virtual queues. "
+"Also fuzzes the virtio flags)",
.pre_fuzz = &virtio_blk_pre_fuzz,
.fuzz = virtio_blk_with_flag_fuzz,},
"virtio-blk",
--
2.39.0
[PULL 03/10] fuzz/generic-fuzz: use reboots instead of forks to reset state
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/generic_fuzz.c | 114 ++--
1 file changed, 22 insertions(+), 92 deletions(-)
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index 7326f6840b..f4acfa45cc 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -18,7 +18,6 @@
#include "tests/qtest/libqtest.h"
#include "tests/qtest/libqos/pci-pc.h"
#include "fuzz.h"
-#include "fork_fuzz.h"
#include "string.h"
#include "exec/memory.h"
#include "exec/ramblock.h"
@@ -29,6 +28,8 @@
#include "generic_fuzz_configs.h"
#include "hw/mem/sparse-mem.h"
+static void pci_enum(gpointer pcidev, gpointer bus);
+
/*
* SEPARATOR is used to separate "operations" in the fuzz input
*/
@@ -47,7 +48,6 @@ enum cmds {
OP_CLOCK_STEP,
};
-#define DEFAULT_TIMEOUT_US 10
#define USEC_IN_SEC 10
#define MAX_DMA_FILL_SIZE 0x1
@@ -60,8 +60,6 @@ typedef struct {
ram_addr_t size; /* The number of bytes until the end of the I/O region */
} address_range;
-static useconds_t timeout = DEFAULT_TIMEOUT_US;
-
static bool qtest_log_enabled;
MemoryRegion *sparse_mem_mr;
@@ -589,30 +587,6 @@ static void op_disable_pci(QTestState *s, const unsigned
char *data, size_t len)
pci_disabled = true;
}
-static void handle_timeout(int sig)
-{
-if (qtest_log_enabled) {
-fprintf(stderr, "[Timeout]\n");
-fflush(stderr);
-}
-
-/*
- * If there is a crash, libfuzzer/ASAN forks a child to run an
- * "llvm-symbolizer" process for printing out a pretty stacktrace. It
- * communicates with this child using a pipe. If we timeout+Exit, while
- * libfuzzer is still communicating with the llvm-symbolizer child, we will
- * be left with an orphan llvm-symbolizer process. Sometimes, this appears
- * to lead to a deadlock in the forkserver. Use waitpid to check if there
- * are any waitable children. If so, exit out of the signal-handler, and
- * let libfuzzer finish communicating with the child, and exit, on its own.
- */
-if (waitpid(-1, NULL, WNOHANG) == 0) {
-return;
-}
-
-_Exit(0);
-}
-
/*
* Here, we interpret random bytes from the fuzzer, as a sequence of commands.
* Some commands can be variable-width, so we use a separator, SEPARATOR, to
@@ -669,64 +643,32 @@ static void generic_fuzz(QTestState *s, const unsigned
char *Data, size_t Size)
size_t cmd_len;
uint8_t op;
-if (fork() == 0) {
-struct sigaction sact;
-struct itimerval timer;
-sigset_t set;
-/*
- * Sometimes the fuzzer will find inputs that take quite a long time to
- * process. Often times, these inputs do not result in new coverage.
- * Even if these inputs might be interesting, they can slow down the
- * fuzzer, overall. Set a timeout for each command to avoid hurting
- * performance, too much
- */
-if (timeout) {
-
-sigemptyset(&sact.sa_mask);
-sact.sa_flags = SA_NODEFER;
-sact.sa_handler = handle_timeout;
-sigaction(SIGALRM, &sact, NULL);
+op_clear_dma_patterns(s, NULL, 0);
+pci_disabled = false;
-sigemptyset(&set);
-sigaddset(&set, SIGALRM);
-pthread_sigmask(SIG_UNBLOCK, &set, NULL);
-
-memset(&timer, 0, sizeof(timer));
-timer.it_value.tv_sec = timeout / USEC_IN_SEC;
-timer.it_value.tv_usec = timeout % USEC_IN_SEC;
-}
-
-op_clear_dma_patterns(s, NULL, 0);
-pci_disabled = false;
-
-while (cmd && Size) {
-/* Reset the timeout, each time we run a new command */
-if (timeout) {
-setitimer(ITIMER_REAL, &timer, NULL);
-}
+QPCIBus *pcibus = qpci_new_pc(s, NULL);
+g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus);
+qpci_free_pc(pcibus);
-/* Get the length until the next command or end of input */
-nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
-cmd_len = nextcmd ? nextcmd - cmd : Size;
+while (cmd && Size) {
+/* Get the length until the next command or end of input */
+nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
+cmd_len = nextcmd ? nextcmd - cmd : Size;
-if (cmd_len > 0) {
-/* Interpret the first byte of the command as an opcode */
-op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
-ops[op](s, cmd + 1, cmd_len - 1);
+if (cmd_len > 0) {
+/* Interpret the first byte of the command as an opcode */
+op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
+
[PULL 08/10] fuzz/i440fx: remove fork-based fuzzer
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/i440fx_fuzz.c | 27 +--
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/tests/qtest/fuzz/i440fx_fuzz.c b/tests/qtest/fuzz/i440fx_fuzz.c
index b17fc725df..155fe018f8 100644
--- a/tests/qtest/fuzz/i440fx_fuzz.c
+++ b/tests/qtest/fuzz/i440fx_fuzz.c
@@ -18,7 +18,6 @@
#include "tests/qtest/libqos/pci-pc.h"
#include "fuzz.h"
#include "qos_fuzz.h"
-#include "fork_fuzz.h"
#define I440FX_PCI_HOST_BRIDGE_CFG 0xcf8
@@ -89,6 +88,7 @@ static void i440fx_fuzz_qtest(QTestState *s,
size_t Size)
{
ioport_fuzz_qtest(s, Data, Size);
+fuzz_reset(s);
}
static void pciconfig_fuzz_qos(QTestState *s, QPCIBus *bus,
@@ -145,17 +145,6 @@ static void i440fx_fuzz_qos(QTestState *s,
pciconfig_fuzz_qos(s, bus, Data, Size);
}
-static void i440fx_fuzz_qos_fork(QTestState *s,
-const unsigned char *Data, size_t Size) {
-if (fork() == 0) {
-i440fx_fuzz_qos(s, Data, Size);
-_Exit(0);
-} else {
-flush_events(s);
-wait(NULL);
-}
-}
-
static const char *i440fx_qtest_argv = TARGET_NAME " -machine accel=qtest"
" -m 0 -display none";
static GString *i440fx_argv(FuzzTarget *t)
@@ -163,10 +152,6 @@ static GString *i440fx_argv(FuzzTarget *t)
return g_string_new(i440fx_qtest_argv);
}
-static void fork_init(void)
-{
-counter_shm_init();
-}
static void register_pci_fuzz_targets(void)
{
@@ -178,16 +163,6 @@ static void register_pci_fuzz_targets(void)
.get_init_cmdline = i440fx_argv,
.fuzz = i440fx_fuzz_qtest});
-/* Uses libqos and forks to prevent state leakage */
-fuzz_add_qos_target(&(FuzzTarget){
-.name = "i440fx-qos-fork-fuzz",
-.description = "Fuzz the i440fx using raw qtest commands and "
- "rebooting after each run",
-.pre_vm_init = &fork_init,
-.fuzz = i440fx_fuzz_qos_fork,},
-"i440FX-pcihost",
-&(QOSGraphTestOptions){}
-);
/*
* Uses libqos. Doesn't do anything to reset state. Note that if we were to
--
2.39.0
[PULL 05/10] fuzz/virtio-scsi: remove fork-based fuzzer
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/virtio_scsi_fuzz.c | 51 -
1 file changed, 7 insertions(+), 44 deletions(-)
diff --git a/tests/qtest/fuzz/virtio_scsi_fuzz.c
b/tests/qtest/fuzz/virtio_scsi_fuzz.c
index b3220ef6cb..b6268efd59 100644
--- a/tests/qtest/fuzz/virtio_scsi_fuzz.c
+++ b/tests/qtest/fuzz/virtio_scsi_fuzz.c
@@ -20,7 +20,6 @@
#include "standard-headers/linux/virtio_pci.h"
#include "standard-headers/linux/virtio_scsi.h"
#include "fuzz.h"
-#include "fork_fuzz.h"
#include "qos_fuzz.h"
#define PCI_SLOT0x02
@@ -132,48 +131,24 @@ static void virtio_scsi_fuzz(QTestState *s,
QVirtioSCSIQueues* queues,
}
}
-static void virtio_scsi_fork_fuzz(QTestState *s,
-const unsigned char *Data, size_t Size)
-{
-QVirtioSCSI *scsi = fuzz_qos_obj;
-static QVirtioSCSIQueues *queues;
-if (!queues) {
-queues = qvirtio_scsi_init(scsi->vdev, 0);
-}
-if (fork() == 0) {
-virtio_scsi_fuzz(s, queues, Data, Size);
-flush_events(s);
-_Exit(0);
-} else {
-flush_events(s);
-wait(NULL);
-}
-}
-
static void virtio_scsi_with_flag_fuzz(QTestState *s,
const unsigned char *Data, size_t Size)
{
QVirtioSCSI *scsi = fuzz_qos_obj;
static QVirtioSCSIQueues *queues;
-if (fork() == 0) {
-if (Size >= sizeof(uint64_t)) {
-queues = qvirtio_scsi_init(scsi->vdev, *(uint64_t *)Data);
-virtio_scsi_fuzz(s, queues,
- Data + sizeof(uint64_t), Size - sizeof(uint64_t));
-flush_events(s);
-}
-_Exit(0);
-} else {
+if (Size >= sizeof(uint64_t)) {
+queues = qvirtio_scsi_init(scsi->vdev, *(uint64_t *)Data);
+virtio_scsi_fuzz(s, queues,
+Data + sizeof(uint64_t), Size - sizeof(uint64_t));
flush_events(s);
-wait(NULL);
}
+fuzz_reset(s);
}
static void virtio_scsi_pre_fuzz(QTestState *s)
{
qos_init_path(s);
-counter_shm_init();
}
static void *virtio_scsi_test_setup(GString *cmd_line, void *arg)
@@ -189,22 +164,10 @@ static void *virtio_scsi_test_setup(GString *cmd_line,
void *arg)
static void register_virtio_scsi_fuzz_targets(void)
{
-fuzz_add_qos_target(&(FuzzTarget){
-.name = "virtio-scsi-fuzz",
-.description = "Fuzz the virtio-scsi virtual queues, forking "
-"for each fuzz run",
-.pre_vm_init = &counter_shm_init,
-.pre_fuzz = &virtio_scsi_pre_fuzz,
-.fuzz = virtio_scsi_fork_fuzz,},
-"virtio-scsi",
-&(QOSGraphTestOptions){.before = virtio_scsi_test_setup}
-);
-
fuzz_add_qos_target(&(FuzzTarget){
.name = "virtio-scsi-flags-fuzz",
-.description = "Fuzz the virtio-scsi virtual queues, forking "
-"for each fuzz run (also fuzzes the virtio flags)",
-.pre_vm_init = &counter_shm_init,
+.description = "Fuzz the virtio-scsi virtual queues. "
+"Also fuzzes the virtio flags",
.pre_fuzz = &virtio_scsi_pre_fuzz,
.fuzz = virtio_scsi_with_flag_fuzz,},
"virtio-scsi",
--
2.39.0
[PULL 09/10] fuzz: remove fork-fuzzing scaffolding
Fork-fuzzing provides a few pros, but our implementation prevents us
from using fuzzers other than libFuzzer, and may be causing issues such
as coverage-failure builds on OSS-Fuzz. It is not a great long-term
solution as it depends on internal implementation details of libFuzzer
(which is no longer in active development). Remove it in favor of other
methods of resetting state between inputs.
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
meson.build | 4 ---
tests/qtest/fuzz/fork_fuzz.c | 41 -
tests/qtest/fuzz/fork_fuzz.h | 23 --
tests/qtest/fuzz/fork_fuzz.ld | 56 ---
tests/qtest/fuzz/meson.build | 6 ++--
5 files changed, 3 insertions(+), 127 deletions(-)
delete mode 100644 tests/qtest/fuzz/fork_fuzz.c
delete mode 100644 tests/qtest/fuzz/fork_fuzz.h
delete mode 100644 tests/qtest/fuzz/fork_fuzz.ld
diff --git a/meson.build b/meson.build
index a76c855312..b6f92bba35 100644
--- a/meson.build
+++ b/meson.build
@@ -215,10 +215,6 @@ endif
# Specify linker-script with add_project_link_arguments so that it is not
placed
# within a linker --start-group/--end-group pair
if get_option('fuzzing')
- add_project_link_arguments(['-Wl,-T,',
- (meson.current_source_dir() /
'tests/qtest/fuzz/fork_fuzz.ld')],
- native: false, language: all_languages)
-
# Specify a filter to only instrument code that is directly related to
# virtual-devices.
configure_file(output: 'instrumentation-filter',
diff --git a/tests/qtest/fuzz/fork_fuzz.c b/tests/qtest/fuzz/fork_fuzz.c
deleted file mode 100644
index 6ffb2a7937..00
--- a/tests/qtest/fuzz/fork_fuzz.c
+++ /dev/null
@@ -1,41 +0,0 @@
-/*
- * Fork-based fuzzing helpers
- *
- * Copyright Red Hat Inc., 2019
- *
- * Authors:
- * Alexander Bulekov
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#include "qemu/osdep.h"
-#include "fork_fuzz.h"
-
-
-void counter_shm_init(void)
-{
-/* Copy what's in the counter region to a temporary buffer.. */
-void *copy = malloc(&__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
-memcpy(copy,
- &__FUZZ_COUNTERS_START,
- &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
-
-/* Map a shared region over the counter region */
-if (mmap(&__FUZZ_COUNTERS_START,
- &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
- PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED | MAP_ANONYMOUS,
- 0, 0) == MAP_FAILED) {
-perror("Error: ");
-exit(1);
-}
-
-/* Copy the original data back to the counter-region */
-memcpy(&__FUZZ_COUNTERS_START, copy,
- &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
-free(copy);
-}
-
-
diff --git a/tests/qtest/fuzz/fork_fuzz.h b/tests/qtest/fuzz/fork_fuzz.h
deleted file mode 100644
index 9ecb8b58ef..00
--- a/tests/qtest/fuzz/fork_fuzz.h
+++ /dev/null
@@ -1,23 +0,0 @@
-/*
- * Fork-based fuzzing helpers
- *
- * Copyright Red Hat Inc., 2019
- *
- * Authors:
- * Alexander Bulekov
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- *
- */
-
-#ifndef FORK_FUZZ_H
-#define FORK_FUZZ_H
-
-extern uint8_t __FUZZ_COUNTERS_START;
-extern uint8_t __FUZZ_COUNTERS_END;
-
-void counter_shm_init(void);
-
-#endif
-
diff --git a/tests/qtest/fuzz/fork_fuzz.ld b/tests/qtest/fuzz/fork_fuzz.ld
deleted file mode 100644
index cfb88b7fdb..00
--- a/tests/qtest/fuzz/fork_fuzz.ld
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * We adjust linker script modification to place all of the stuff that needs to
- * persist across fuzzing runs into a contiguous section of memory. Then, it is
- * easy to re-map the counter-related memory as shared.
- */
-
-SECTIONS
-{
- .data.fuzz_start : ALIGN(4K)
- {
- __FUZZ_COUNTERS_START = .;
- __start___sancov_cntrs = .;
- *(_*sancov_cntrs);
- __stop___sancov_cntrs = .;
-
- /* Lowest stack counter */
- *(__sancov_lowest_stack);
- }
-}
-INSERT AFTER .data;
-
-SECTIONS
-{
- .data.fuzz_ordered :
- {
- /*
- * Coverage counters. They're not necessary for fuzzing, but are useful
- * for analyzing the fuzzing performance
- */
- __start___llvm_prf_cnts = .;
- *(*llvm_prf_cnts);
- __stop___llvm_prf_cnts = .;
-
- /* Internal Libfuzzer TracePC object which contains the ValueProfileMap
*/
- FuzzerTracePC*(.bss*);
- /*
- * In case the above line fails, explicitly specify the (mangled) name of
- * the object we care about
- */
- *(.bss._ZN6fuzzer3TPCE);
- }
-}
-INSERT AFTER .data.fuzz_start;
-
-SECTIONS
-{
- .data.fuzz_end : ALIGN(4K)
-
[PULL 06/10] fuzz/virtio-net: remove fork-based fuzzer
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/virtio_net_fuzz.c | 54 +++---
1 file changed, 5 insertions(+), 49 deletions(-)
diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c
b/tests/qtest/fuzz/virtio_net_fuzz.c
index c2c15f07f0..e239875e3b 100644
--- a/tests/qtest/fuzz/virtio_net_fuzz.c
+++ b/tests/qtest/fuzz/virtio_net_fuzz.c
@@ -16,7 +16,6 @@
#include "tests/qtest/libqtest.h"
#include "tests/qtest/libqos/virtio-net.h"
#include "fuzz.h"
-#include "fork_fuzz.h"
#include "qos_fuzz.h"
@@ -115,36 +114,18 @@ static void virtio_net_fuzz_multi(QTestState *s,
}
}
-static void virtio_net_fork_fuzz(QTestState *s,
-const unsigned char *Data, size_t Size)
-{
-if (fork() == 0) {
-virtio_net_fuzz_multi(s, Data, Size, false);
-flush_events(s);
-_Exit(0);
-} else {
-flush_events(s);
-wait(NULL);
-}
-}
-static void virtio_net_fork_fuzz_check_used(QTestState *s,
+static void virtio_net_fuzz_check_used(QTestState *s,
const unsigned char *Data, size_t Size)
{
-if (fork() == 0) {
-virtio_net_fuzz_multi(s, Data, Size, true);
-flush_events(s);
-_Exit(0);
-} else {
-flush_events(s);
-wait(NULL);
-}
+virtio_net_fuzz_multi(s, Data, Size, true);
+flush_events(s);
+fuzz_reset(s);
}
static void virtio_net_pre_fuzz(QTestState *s)
{
qos_init_path(s);
-counter_shm_init();
}
static void *virtio_net_test_setup_socket(GString *cmd_line, void *arg)
@@ -158,23 +139,8 @@ static void *virtio_net_test_setup_socket(GString
*cmd_line, void *arg)
return arg;
}
-static void *virtio_net_test_setup_user(GString *cmd_line, void *arg)
-{
-g_string_append_printf(cmd_line, " -netdev user,id=hs0 ");
-return arg;
-}
-
static void register_virtio_net_fuzz_targets(void)
{
-fuzz_add_qos_target(&(FuzzTarget){
-.name = "virtio-net-socket",
-.description = "Fuzz the virtio-net virtual queues. Fuzz incoming "
-"traffic using the socket backend",
-.pre_fuzz = &virtio_net_pre_fuzz,
-.fuzz = virtio_net_fork_fuzz,},
-"virtio-net",
-&(QOSGraphTestOptions){.before = virtio_net_test_setup_socket}
-);
fuzz_add_qos_target(&(FuzzTarget){
.name = "virtio-net-socket-check-used",
@@ -182,20 +148,10 @@ static void register_virtio_net_fuzz_targets(void)
"descriptors to be used. Timeout may indicate improperly handled "
"input",
.pre_fuzz = &virtio_net_pre_fuzz,
-.fuzz = virtio_net_fork_fuzz_check_used,},
+.fuzz = virtio_net_fuzz_check_used,},
"virtio-net",
&(QOSGraphTestOptions){.before = virtio_net_test_setup_socket}
);
-fuzz_add_qos_target(&(FuzzTarget){
-.name = "virtio-net-slirp",
-.description = "Fuzz the virtio-net virtual queues with the slirp "
-" backend. Warning: May result in network traffic emitted from the
"
-" process. Run in an isolated network environment.",
-.pre_fuzz = &virtio_net_pre_fuzz,
-.fuzz = virtio_net_fork_fuzz,},
-"virtio-net",
-&(QOSGraphTestOptions){.before = virtio_net_test_setup_user}
-);
}
fuzz_target_init(register_virtio_net_fuzz_targets);
--
2.39.0
[PULL 02/10] fuzz: add fuzz_reset API
As we are converting most fuzzers to rely on reboots to reset state,
introduce an API to make sure reboots are invoked in a consistent
manner.
Signed-off-by: Alexander Bulekov
---
tests/qtest/fuzz/fuzz.c | 6 ++
tests/qtest/fuzz/fuzz.h | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index eb7520544b..3bedb81b32 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -51,6 +51,12 @@ void flush_events(QTestState *s)
}
}
+void fuzz_reset(QTestState *s)
+{
+qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
+main_loop_wait(true);
+}
+
static QTestState *qtest_setup(void)
{
qtest_server_set_send_handler(&qtest_client_inproc_recv, &fuzz_qts);
diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
index 327c1c5a55..21d1362d65 100644
--- a/tests/qtest/fuzz/fuzz.h
+++ b/tests/qtest/fuzz/fuzz.h
@@ -103,7 +103,7 @@ typedef struct FuzzTarget {
} FuzzTarget;
void flush_events(QTestState *);
-void reboot(QTestState *);
+void fuzz_reset(QTestState *);
/* Use the QTest ASCII protocol or call address_space API directly?*/
void fuzz_qtest_set_serialize(bool option);
--
2.39.0
[PULL 01/10] hw/sparse-mem: clear memory on reset
We use sparse-mem for fuzzing. For long-running fuzzing processes, we
eventually end up with many allocated sparse-mem pages. To avoid this,
clear the allocated pages on system-reset.
Signed-off-by: Alexander Bulekov
Reviewed-by: Darren Kenny
Reviewed-by: Philippe Mathieu-Daudé
---
hw/mem/sparse-mem.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/mem/sparse-mem.c b/hw/mem/sparse-mem.c
index e6640eb8e7..72f038d47d 100644
--- a/hw/mem/sparse-mem.c
+++ b/hw/mem/sparse-mem.c
@@ -77,6 +77,13 @@ static void sparse_mem_write(void *opaque, hwaddr addr,
uint64_t v,
}
+static void sparse_mem_enter_reset(Object *obj, ResetType type)
+{
+SparseMemState *s = SPARSE_MEM(obj);
+g_hash_table_remove_all(s->mapped);
+return;
+}
+
static const MemoryRegionOps sparse_mem_ops = {
.read = sparse_mem_read,
.write = sparse_mem_write,
@@ -123,7 +130,8 @@ static void sparse_mem_realize(DeviceState *dev, Error
**errp)
assert(s->baseaddr + s->length > s->baseaddr);
-s->mapped = g_hash_table_new(NULL, NULL);
+s->mapped = g_hash_table_new_full(NULL, NULL, NULL,
+ (GDestroyNotify)g_free);
memory_region_init_io(&s->mmio, OBJECT(s), &sparse_mem_ops, s,
"sparse-mem", s->length);
sysbus_init_mmio(sbd, &s->mmio);
@@ -131,12 +139,15 @@ static void sparse_mem_realize(DeviceState *dev, Error
**errp)
static void sparse_mem_class_init(ObjectClass *klass, void *data)
{
+ResettableClass *rc = RESETTABLE_CLASS(klass);
DeviceClass *dc = DEVICE_CLASS(klass);
device_class_set_props(dc, sparse_mem_properties);
dc->desc = "Sparse Memory Device";
dc->realize = sparse_mem_realize;
+
+rc->phases.enter = sparse_mem_enter_reset;
}
static const TypeInfo sparse_mem_types[] = {
--
2.39.0
[PULL 04/10] fuzz/generic-fuzz: add a limit on DMA bytes written
As we have repplaced fork-based fuzzing, with reboots - we can no longer
use a timeout+exit() to avoid slow inputs. Libfuzzer has its own timer
that it uses to catch slow inputs, however these timeouts are usually
seconds-minutes long: more than enough to bog-down the fuzzing process.
However, I found that slow inputs often attempt to fill overly large DMA
requests. Thus, we can mitigate most timeouts by setting a cap on the
total number of DMA bytes written by an input.
Signed-off-by: Alexander Bulekov
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Darren Kenny
---
tests/qtest/fuzz/generic_fuzz.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index f4acfa45cc..c525d22951 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -51,6 +51,7 @@ enum cmds {
#define USEC_IN_SEC 10
#define MAX_DMA_FILL_SIZE 0x1
+#define MAX_TOTAL_DMA_SIZE 0x1000
#define PCI_HOST_BRIDGE_CFG 0xcf8
#define PCI_HOST_BRIDGE_DATA 0xcfc
@@ -61,6 +62,7 @@ typedef struct {
} address_range;
static bool qtest_log_enabled;
+size_t dma_bytes_written;
MemoryRegion *sparse_mem_mr;
@@ -194,6 +196,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion
*mr)
*/
if (dma_patterns->len == 0
|| len == 0
+|| dma_bytes_written + len > MAX_TOTAL_DMA_SIZE
|| (mr != current_machine->ram && mr != sparse_mem_mr)) {
return;
}
@@ -266,6 +269,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion
*mr)
fflush(stderr);
}
qtest_memwrite(qts_global, addr, buf, l);
+dma_bytes_written += l;
}
len -= l;
buf += l;
@@ -645,6 +649,7 @@ static void generic_fuzz(QTestState *s, const unsigned char
*Data, size_t Size)
op_clear_dma_patterns(s, NULL, 0);
pci_disabled = false;
+dma_bytes_written = 0;
QPCIBus *pcibus = qpci_new_pc(s, NULL);
g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus);
--
2.39.0
[PULL 00/10] Replace fork-based fuzzing with reboots
Hi Peter, The following changes since commit 6dffbe36af79e26a4d23f94a9a1c1201de99c261: Merge tag 'migration-20230215-pull-request' of https://gitlab.com/juan.quintela/qemu into staging (2023-02-16 13:09:51 +) are available in the Git repository at: https://gitlab.com/a1xndr/qemu/ tags/pr-2023-02-16 for you to fetch changes up to 7d9e5f18a94792ed875a1caed2bfcd1e68a49481: docs/fuzz: remove mentions of fork-based fuzzing (2023-02-16 23:02:46 -0500) Replace fork-based fuzzing with reboots. Now the fuzzers will reboot the guest between inputs. ---- Alexander Bulekov (10): hw/sparse-mem: clear memory on reset fuzz: add fuzz_reset API fuzz/generic-fuzz: use reboots instead of forks to reset state fuzz/generic-fuzz: add a limit on DMA bytes written fuzz/virtio-scsi: remove fork-based fuzzer fuzz/virtio-net: remove fork-based fuzzer fuzz/virtio-blk: remove fork-based fuzzer fuzz/i440fx: remove fork-based fuzzer fuzz: remove fork-fuzzing scaffolding docs/fuzz: remove mentions of fork-based fuzzing docs/devel/fuzzing.rst | 22 +-- hw/mem/sparse-mem.c | 13 +++- meson.build | 4 -- tests/qtest/fuzz/fork_fuzz.c| 41 - tests/qtest/fuzz/fork_fuzz.h| 23 --- tests/qtest/fuzz/fork_fuzz.ld | 56 - tests/qtest/fuzz/fuzz.c | 6 ++ tests/qtest/fuzz/fuzz.h | 2 +- tests/qtest/fuzz/generic_fuzz.c | 119 tests/qtest/fuzz/i440fx_fuzz.c | 27 +--- tests/qtest/fuzz/meson.build| 6 +- tests/qtest/fuzz/virtio_blk_fuzz.c | 51 +++- tests/qtest/fuzz/virtio_net_fuzz.c | 54 ++-- tests/qtest/fuzz/virtio_scsi_fuzz.c | 51 +++- 14 files changed, 71 insertions(+), 404 deletions(-) delete mode 100644 tests/qtest/fuzz/fork_fuzz.c delete mode 100644 tests/qtest/fuzz/fork_fuzz.h delete mode 100644 tests/qtest/fuzz/fork_fuzz.ld
Re: [PATCH 03/10] fuzz/generic-fuzz: use reboots instead of forks to reset state
On 230213 1426, Darren Kenny wrote:
> Hi Alex,
>
> On Saturday, 2023-02-04 at 23:29:44 -05, Alexander Bulekov wrote:
> > Signed-off-by: Alexander Bulekov
> > ---
> > tests/qtest/fuzz/generic_fuzz.c | 106 +++-
> > 1 file changed, 23 insertions(+), 83 deletions(-)
> >
> > diff --git a/tests/qtest/fuzz/generic_fuzz.c
> > b/tests/qtest/fuzz/generic_fuzz.c
> > index 7326f6840b..c2e5642150 100644
> > --- a/tests/qtest/fuzz/generic_fuzz.c
> > +++ b/tests/qtest/fuzz/generic_fuzz.c
> > @@ -18,7 +18,6 @@
> > #include "tests/qtest/libqtest.h"
> > #include "tests/qtest/libqos/pci-pc.h"
> > #include "fuzz.h"
> > -#include "fork_fuzz.h"
> > #include "string.h"
> > #include "exec/memory.h"
> > #include "exec/ramblock.h"
> > @@ -29,6 +28,8 @@
> > #include "generic_fuzz_configs.h"
> > #include "hw/mem/sparse-mem.h"
> >
> > +static void pci_enum(gpointer pcidev, gpointer bus);
> > +
> > /*
> > * SEPARATOR is used to separate "operations" in the fuzz input
> > */
> > @@ -589,30 +590,6 @@ static void op_disable_pci(QTestState *s, const
> > unsigned char *data, size_t len)
> > pci_disabled = true;
> > }
> >
> > -static void handle_timeout(int sig)
> > -{
> > -if (qtest_log_enabled) {
> > -fprintf(stderr, "[Timeout]\n");
> > -fflush(stderr);
> > -}
> > -
> > -/*
> > - * If there is a crash, libfuzzer/ASAN forks a child to run an
> > - * "llvm-symbolizer" process for printing out a pretty stacktrace. It
> > - * communicates with this child using a pipe. If we timeout+Exit,
> > while
> > - * libfuzzer is still communicating with the llvm-symbolizer child, we
> > will
> > - * be left with an orphan llvm-symbolizer process. Sometimes, this
> > appears
> > - * to lead to a deadlock in the forkserver. Use waitpid to check if
> > there
> > - * are any waitable children. If so, exit out of the signal-handler,
> > and
> > - * let libfuzzer finish communicating with the child, and exit, on its
> > own.
> > - */
> > -if (waitpid(-1, NULL, WNOHANG) == 0) {
> > -return;
> > -}
> > -
> > -_Exit(0);
> > -}
> > -
> > /*
> >
>
> I'm presuming that the timeout is being left to the fuzz orchestrator
> now, rather than us managing it directly in our own way?
Yes. The fuzzer should handle timeouts directly now.
-Alex
Re: [PATCH 04/10] fuzz/generic-fuzz: add a limit on DMA bytes written
On 230213 1438, Darren Kenny wrote:
> Hi Alex,
>
> On Saturday, 2023-02-04 at 23:29:45 -05, Alexander Bulekov wrote:
> > As we have repplaced fork-based fuzzing, with reboots - we can no longer
> > use a timeout+exit() to avoid slow inputs. Libfuzzer has its own timer
> > that it uses to catch slow inputs, however these timeouts are usually
> > seconds-minutes long: more than enough to bog-down the fuzzing process.
> > However, I found that slow inputs often attempt to fill overly large DMA
> > requests. Thus, we can mitigate most timeouts by setting a cap on the
> > total number of DMA bytes written by an input.
> >
> > Signed-off-by: Alexander Bulekov
> > ---
> > tests/qtest/fuzz/generic_fuzz.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/tests/qtest/fuzz/generic_fuzz.c
> > b/tests/qtest/fuzz/generic_fuzz.c
> > index c2e5642150..eab92cbc23 100644
> > --- a/tests/qtest/fuzz/generic_fuzz.c
> > +++ b/tests/qtest/fuzz/generic_fuzz.c
> > @@ -52,6 +52,7 @@ enum cmds {
> > #define USEC_IN_SEC 10
> >
> > #define MAX_DMA_FILL_SIZE 0x1
> > +#define MAX_TOTAL_DMA_SIZE 0x1000
> >
> > #define PCI_HOST_BRIDGE_CFG 0xcf8
> > #define PCI_HOST_BRIDGE_DATA 0xcfc
> > @@ -64,6 +65,7 @@ typedef struct {
> > static useconds_t timeout = DEFAULT_TIMEOUT_US;
> >
> > static bool qtest_log_enabled;
> > +size_t dma_bytes_written;
> >
> > MemoryRegion *sparse_mem_mr;
> >
> > @@ -197,6 +199,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len,
> > MemoryRegion *mr)
> > */
> > if (dma_patterns->len == 0
> > || len == 0
> > +|| dma_bytes_written > MAX_TOTAL_DMA_SIZE
>
> NIT: Just wondering if you should check dma_bytes_written + l as opposed
> to dma_bytes_written? It's probably not important enough given it's
> just an artificial limit, but thought I'd ask.
>
Done :)
> > || (mr != current_machine->ram && mr != sparse_mem_mr)) {
> > return;
> > }
> > @@ -269,6 +272,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len,
> > MemoryRegion *mr)
> > fflush(stderr);
> > }
> > qtest_memwrite(qts_global, addr, buf, l);
> > +dma_bytes_written += l;
> > }
> > len -= l;
> > buf += l;
> > @@ -648,6 +652,7 @@ static void generic_fuzz(QTestState *s, const unsigned
> > char *Data, size_t Size)
> >
> > op_clear_dma_patterns(s, NULL, 0);
> > pci_disabled = false;
> > +dma_bytes_written = 0;
> >
> > QPCIBus *pcibus = qpci_new_pc(s, NULL);
> > g_ptr_array_foreach(fuzzable_pci_devices, pci_enum, pcibus);
> > --
> > 2.39.0
>
> While this will still consume the existing corpus, is it likely to
> cause these existing corpus to be trimmed?
Not sure - It would affect inputs that generate a lot of DMA
activity (though those should have been caught by our previous timeout
mechanism).
>
> Otherwise, the changes look good:
>
> Reviewed-by: Darren Kenny
>
> Thanks,
>
> Darren.
