* [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 @ 2021-11-18 12:06 Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 Philippe Mathieu-Daudé ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-18 12:06 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Philippe Mathieu-Daudé, Markus Armbruster, Alexander Bulekov, Hanna Reitz, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow I'm not sure what happened to v1 from Prasad, so since we are at rc2 I took a simpler approach to fix this CVE: create an empty drive to satisfy the BlockBackend API calls. Added Alexander's reproducer along. Since v2: - Reword comment (Darren) - Add Darren R-b tag v2: https://lore.kernel.org/qemu-devel/20211117232422.1026411-1-philmd@redhat.com/ v1: https://lore.kernel.org/qemu-devel/20210123100345.642933-1-ppandit@redhat.com/ Based-on: <20211118115733.4038610-1-philmd@redhat.com> Alexander Bulekov (1): tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé (1): hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 hw/block/fdc.c | 14 +++++++++++++- tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 2021-11-18 12:06 [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé @ 2021-11-18 12:06 ` Philippe Mathieu-Daudé 2021-11-23 13:33 ` Hanna Reitz 2021-11-18 12:06 ` [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-22 14:55 ` [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé 2 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-18 12:06 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Philippe Mathieu-Daudé, Markus Armbruster, qemu-stable, Alexander Bulekov, Hanna Reitz, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow Guest might select another drive on the bus by setting the DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR). The current controller model doesn't expect a BlockBackend to be NULL. A simple way to fix CVE-2021-20196 is to create an empty BlockBackend when it is missing. All further accesses will be safely handled, and the controller state machines keep behaving correctly. Cc: qemu-stable@nongnu.org Fixes: CVE-2021-20196 Reported-by: Gaoning Pan (Ant Security Light-Year Lab) <pgn@zju.edu.cn> BugLink: https://bugs.launchpad.net/qemu/+bug/1912780 Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338 Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/block/fdc.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index d21b717b7d6..6f94b6a6daa 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit) static FDrive *get_cur_drv(FDCtrl *fdctrl) { - return get_drv(fdctrl, fdctrl->cur_drv); + FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv); + + if (!cur_drv->blk) { + /* + * Kludge: empty drive line selected. Create an anonymous + * BlockBackend to avoid NULL deref with various BlockBackend + * API calls within this model (CVE-2021-20196). + * Due to the controller QOM model limitations, we don't + * attach the created to the controller device. + */ + cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); + } + return cur_drv; } /* Status A register : 0x00 (read-only) */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 2021-11-18 12:06 ` [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 Philippe Mathieu-Daudé @ 2021-11-23 13:33 ` Hanna Reitz 2021-11-23 13:57 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Hanna Reitz @ 2021-11-23 13:33 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, qemu-stable, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: > Guest might select another drive on the bus by setting the > DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR). > The current controller model doesn't expect a BlockBackend > to be NULL. A simple way to fix CVE-2021-20196 is to create > an empty BlockBackend when it is missing. All further > accesses will be safely handled, and the controller state > machines keep behaving correctly. > > Cc: qemu-stable@nongnu.org > Fixes: CVE-2021-20196 > Reported-by: Gaoning Pan (Ant Security Light-Year Lab) <pgn@zju.edu.cn> > BugLink: https://bugs.launchpad.net/qemu/+bug/1912780 > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338 > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/block/fdc.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index d21b717b7d6..6f94b6a6daa 100644 > --- a/hw/block/fdc.c > +++ b/hw/block/fdc.c > @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit) > > static FDrive *get_cur_drv(FDCtrl *fdctrl) > { > - return get_drv(fdctrl, fdctrl->cur_drv); > + FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv); > + > + if (!cur_drv->blk) { > + /* > + * Kludge: empty drive line selected. Create an anonymous > + * BlockBackend to avoid NULL deref with various BlockBackend > + * API calls within this model (CVE-2021-20196). > + * Due to the controller QOM model limitations, we don't > + * attach the created to the controller device. > + */ > + cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); So to me this looks basically like a mini version of floppy_drive_realize(), and I was wondering what else we might want to use from that function. fd_init() and fd_revalidate() look interesting, but it appears that fdctrl_realize_common() already did that for all drives so we should be good. Then again, fd_revalidate() behaves differently for the initial drv->blk == NULL (drv->drive is set to TYPE_NONE, and last_sect and max_track are set to 0) and for then later !blk_is_inserted() (drv->drive not changed (so I guess it stays TYPE_NONE?), but last_sect and max_track are set to 0xff). Not sure if that’s a problem. Probably not, given that I think drv->disk and drv->drive both stay TYPE_NONE. Reviewed-by: Hanna Reitz <hreitz@redhat.com> > + } > + return cur_drv; > } > > /* Status A register : 0x00 (read-only) */ ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 2021-11-23 13:33 ` Hanna Reitz @ 2021-11-23 13:57 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-23 13:57 UTC (permalink / raw) To: Hanna Reitz, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, qemu-stable, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 11/23/21 14:33, Hanna Reitz wrote: > On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >> Guest might select another drive on the bus by setting the >> DRIVE_SEL bit of the DIGITAL OUTPUT REGISTER (DOR). >> The current controller model doesn't expect a BlockBackend >> to be NULL. A simple way to fix CVE-2021-20196 is to create >> an empty BlockBackend when it is missing. All further >> accesses will be safely handled, and the controller state >> machines keep behaving correctly. >> >> Cc: qemu-stable@nongnu.org >> Fixes: CVE-2021-20196 >> Reported-by: Gaoning Pan (Ant Security Light-Year Lab) <pgn@zju.edu.cn> >> BugLink: https://bugs.launchpad.net/qemu/+bug/1912780 >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/338 >> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/block/fdc.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/fdc.c b/hw/block/fdc.c >> index d21b717b7d6..6f94b6a6daa 100644 >> --- a/hw/block/fdc.c >> +++ b/hw/block/fdc.c >> @@ -1161,7 +1161,19 @@ static FDrive *get_drv(FDCtrl *fdctrl, int unit) >> static FDrive *get_cur_drv(FDCtrl *fdctrl) >> { >> - return get_drv(fdctrl, fdctrl->cur_drv); >> + FDrive *cur_drv = get_drv(fdctrl, fdctrl->cur_drv); >> + >> + if (!cur_drv->blk) { >> + /* >> + * Kludge: empty drive line selected. Create an anonymous >> + * BlockBackend to avoid NULL deref with various BlockBackend >> + * API calls within this model (CVE-2021-20196). >> + * Due to the controller QOM model limitations, we don't >> + * attach the created to the controller device. >> + */ >> + cur_drv->blk = blk_new(qemu_get_aio_context(), 0, BLK_PERM_ALL); > > So to me this looks basically like a mini version of > floppy_drive_realize(), and I was wondering what else we might want to > use from that function. fd_init() and fd_revalidate() look interesting, > but it appears that fdctrl_realize_common() already did that for all > drives so we should be good. How the controller / bus / floppy / drive are connected is a bit confusing. Did you ever try to hot-remove the magnetic medium from the floppy plastic enclosure while the controller rotates it? > Then again, fd_revalidate() behaves differently for the initial drv->blk > == NULL (drv->drive is set to TYPE_NONE, and last_sect and max_track are > set to 0) and for then later !blk_is_inserted() (drv->drive not changed > (so I guess it stays TYPE_NONE?), but last_sect and max_track are set to > 0xff). Not sure if that’s a problem. Probably not, given that I think > drv->disk and drv->drive both stay TYPE_NONE. I'm not sure about the future plans for this device model... > Reviewed-by: Hanna Reitz <hreitz@redhat.com> Thanks! ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-18 12:06 [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 Philippe Mathieu-Daudé @ 2021-11-18 12:06 ` Philippe Mathieu-Daudé 2021-11-23 13:42 ` Hanna Reitz 2021-11-22 14:55 ` [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé 2 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-18 12:06 UTC (permalink / raw) To: qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Philippe Mathieu-Daudé, Markus Armbruster, Alexander Bulekov, Hanna Reitz, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow From: Alexander Bulekov <alxndr@bu.edu> Without the previous commit, when running 'make check-qtest-i386' with QEMU configured with '--enable-sanitizers' we get: AddressSanitizer:DEADLYSIGNAL ================================================================= ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000344 ==287878==The signal is caused by a WRITE memory access. ==287878==Hint: address points to the zero page. #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5 #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 Add the reproducer for CVE-2021-20196. Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> [PMD: Rebased, use global test_image] Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/qtest/fdc-test.c b/tests/qtest/fdc-test.c index f164d972d10..0f8b9b753f4 100644 --- a/tests/qtest/fdc-test.c +++ b/tests/qtest/fdc-test.c @@ -565,6 +565,26 @@ static void test_cve_2021_3507(void) qtest_quit(s); } +static void test_cve_2021_20196(void) +{ + QTestState *s; + + s = qtest_initf("-nographic -m 32M -nodefaults " + "-drive file=%s,format=raw,if=floppy", test_image); + qtest_outw(s, 0x3f2, 0x0004); + qtest_outw(s, 0x3f4, 0x0200); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f4, 0x0000); + qtest_outw(s, 0x3f2, 0x0001); + qtest_quit(s); +} + int main(int argc, char **argv) { int fd; @@ -596,6 +616,7 @@ int main(int argc, char **argv) qtest_add_func("/fdc/read_no_dma_19", test_read_no_dma_19); qtest_add_func("/fdc/fuzz-registers", fuzz_registers); qtest_add_func("/fdc/fuzz/cve_2021_3507", test_cve_2021_3507); + qtest_add_func("/fdc/fuzz/cve_2021_20196", test_cve_2021_20196); ret = g_test_run(); -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-18 12:06 ` [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé @ 2021-11-23 13:42 ` Hanna Reitz 2021-11-23 13:49 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Hanna Reitz @ 2021-11-23 13:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: > From: Alexander Bulekov <alxndr@bu.edu> > > Without the previous commit, when running 'make check-qtest-i386' > with QEMU configured with '--enable-sanitizers' we get: > > AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==287878==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000344 > ==287878==The signal is caused by a WRITE memory access. > ==287878==Hint: address points to the zero page. > #0 0x564b2e5bac27 in blk_inc_in_flight block/block-backend.c:1346:5 > #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 > #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 > #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 > #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 > #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 > > Add the reproducer for CVE-2021-20196. > > Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > Message-Id: <20210319050906.14875-2-alxndr@bu.edu> > [PMD: Rebased, use global test_image] > Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) Not sure if I’m doing something wrong, but: Using the global test_image brings a problem, namely that this test fails unconditionally (for me at least...?), with the reason being that the global QEMU instance (launched by qtest_start(), quit by qtest_end()) still has that image open, so by launching a second instance concurrently, I get this: qemu-system-x86_64: Failed to get "write" lock Is another process using the image [/tmp/qtest.xV4IxX]? So either we need to use a different image file, or we need to quit the global instance before using it (e.g. putting a qtest_end() at the beginning of test_cve_*()), although the latter just seems wrong. Second, I can’t make this test fail. When I apply this patch first (to master) and run it, I don’t get a SIGSEGV. Hanna ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-23 13:42 ` Hanna Reitz @ 2021-11-23 13:49 ` Philippe Mathieu-Daudé 2021-11-23 14:14 ` Hanna Reitz 2021-11-23 16:05 ` Alexander Bulekov 0 siblings, 2 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-23 13:49 UTC (permalink / raw) To: Hanna Reitz, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 11/23/21 14:42, Hanna Reitz wrote: > On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >> From: Alexander Bulekov <alxndr@bu.edu> >> >> Without the previous commit, when running 'make check-qtest-i386' >> with QEMU configured with '--enable-sanitizers' we get: >> >> AddressSanitizer:DEADLYSIGNAL >> ================================================================= >> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >> 0x000000000344 >> ==287878==The signal is caused by a WRITE memory access. >> ==287878==Hint: address points to the zero page. >> #0 0x564b2e5bac27 in blk_inc_in_flight >> block/block-backend.c:1346:5 >> #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 >> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 >> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 >> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >> >> Add the reproducer for CVE-2021-20196. >> >> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> >> [PMD: Rebased, use global test_image] >> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) > > Not sure if I’m doing something wrong, but: > > Using the global test_image brings a problem, namely that this test > fails unconditionally (for me at least...?), with the reason being that > the global QEMU instance (launched by qtest_start(), quit by > qtest_end()) still has that image open, so by launching a second > instance concurrently, I get this: > > qemu-system-x86_64: Failed to get "write" lock > Is another process using the image [/tmp/qtest.xV4IxX]? Hmm I had too many odd problems running qtests in parallel so I switched to 'make check-qtest -j1' more than 1 year ago, which is probably why I haven't noticed that issue. Using another 'test_image' seems against code reuse principle, but at this point in release, duplicating it is simpler. Someone will clean that later =) > So either we need to use a different image file, or we need to quit the > global instance before using it (e.g. putting a qtest_end() at the > beginning of test_cve_*()), although the latter just seems wrong. > > Second, I can’t make this test fail. When I apply this patch first (to > master) and run it, I don’t get a SIGSEGV. Is your QEMU built with --enable-sanitizers ? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-23 13:49 ` Philippe Mathieu-Daudé @ 2021-11-23 14:14 ` Hanna Reitz 2021-11-24 12:50 ` Philippe Mathieu-Daudé 2021-11-23 16:05 ` Alexander Bulekov 1 sibling, 1 reply; 13+ messages in thread From: Hanna Reitz @ 2021-11-23 14:14 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: > On 11/23/21 14:42, Hanna Reitz wrote: >> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >>> From: Alexander Bulekov <alxndr@bu.edu> >>> >>> Without the previous commit, when running 'make check-qtest-i386' >>> with QEMU configured with '--enable-sanitizers' we get: >>> >>> AddressSanitizer:DEADLYSIGNAL >>> ================================================================= >>> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >>> 0x000000000344 >>> ==287878==The signal is caused by a WRITE memory access. >>> ==287878==Hint: address points to the zero page. >>> #0 0x564b2e5bac27 in blk_inc_in_flight >>> block/block-backend.c:1346:5 >>> #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 >>> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 >>> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 >>> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >>> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >>> >>> Add the reproducer for CVE-2021-20196. >>> >>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >>> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> >>> [PMD: Rebased, use global test_image] >>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >> Not sure if I’m doing something wrong, but: >> >> Using the global test_image brings a problem, namely that this test >> fails unconditionally (for me at least...?), with the reason being that >> the global QEMU instance (launched by qtest_start(), quit by >> qtest_end()) still has that image open, so by launching a second >> instance concurrently, I get this: >> >> qemu-system-x86_64: Failed to get "write" lock >> Is another process using the image [/tmp/qtest.xV4IxX]? > Hmm I had too many odd problems running qtests in parallel so I > switched to 'make check-qtest -j1' more than 1 year ago, which > is probably why I haven't noticed that issue. I’ve run the test with QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test so there definitely weren’t any other tests running at the same time. I don’t know why you don’t encounter this problem, but it’s caused by the concurrent QEMU instance launched in the very same test (qtest_start() in main(), and cleaned up by qtest_end() after g_test_run()). > Using another 'test_image' seems against code reuse principle, > but at this point in release, duplicating it is simpler. Someone > will clean that later =) > >> So either we need to use a different image file, or we need to quit the >> global instance before using it (e.g. putting a qtest_end() at the >> beginning of test_cve_*()), although the latter just seems wrong. >> >> Second, I can’t make this test fail. When I apply this patch first (to >> master) and run it, I don’t get a SIGSEGV. > Is your QEMU built with --enable-sanitizers ? Uh, no. I had (wrongly) assumed there’d be no need, given that the SIGSEGV access address is below 4 kB... I now did configure and compile it with --enable-sanitizers but still can’t reproduce it. Hanna ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-23 14:14 ` Hanna Reitz @ 2021-11-24 12:50 ` Philippe Mathieu-Daudé 2021-11-24 14:00 ` Hanna Reitz 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-24 12:50 UTC (permalink / raw) To: Hanna Reitz, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Alexander Bulekov, Markus Armbruster, Darren Kenny, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 11/23/21 15:14, Hanna Reitz wrote: > On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: >> On 11/23/21 14:42, Hanna Reitz wrote: >>> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >>>> From: Alexander Bulekov <alxndr@bu.edu> >>>> >>>> Without the previous commit, when running 'make check-qtest-i386' >>>> with QEMU configured with '--enable-sanitizers' we get: >>>> >>>> AddressSanitizer:DEADLYSIGNAL >>>> ================================================================= >>>> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >>>> 0x000000000344 >>>> ==287878==The signal is caused by a WRITE memory access. >>>> ==287878==Hint: address points to the zero page. >>>> #0 0x564b2e5bac27 in blk_inc_in_flight >>>> block/block-backend.c:1346:5 >>>> #1 0x564b2e5bb228 in blk_pwritev_part >>>> block/block-backend.c:1317:5 >>>> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 >>>> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 >>>> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >>>> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >>>> >>>> Add the reproducer for CVE-2021-20196. >>>> >>>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >>>> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> >>>> [PMD: Rebased, use global test_image] >>>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>> Not sure if I’m doing something wrong, but: >>> >>> Using the global test_image brings a problem, namely that this test >>> fails unconditionally (for me at least...?), with the reason being that >>> the global QEMU instance (launched by qtest_start(), quit by >>> qtest_end()) still has that image open, so by launching a second >>> instance concurrently, I get this: >>> >>> qemu-system-x86_64: Failed to get "write" lock >>> Is another process using the image [/tmp/qtest.xV4IxX]? >> Hmm I had too many odd problems running qtests in parallel so I >> switched to 'make check-qtest -j1' more than 1 year ago, which >> is probably why I haven't noticed that issue. > > I’ve run the test with > > QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test > > so there definitely weren’t any other tests running at the same time. I > don’t know why you don’t encounter this problem, but it’s caused by the > concurrent QEMU instance launched in the very same test (qtest_start() > in main(), and cleaned up by qtest_end() after g_test_run()). I run all my qtests on top of this patch, otherwise I can't get any coredump: https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4bug@amsat.org/ But I don't think it mattered here... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-24 12:50 ` Philippe Mathieu-Daudé @ 2021-11-24 14:00 ` Hanna Reitz 2021-11-24 15:11 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Hanna Reitz @ 2021-11-24 14:00 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Alexander Bulekov, Markus Armbruster, Darren Kenny, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 24.11.21 13:50, Philippe Mathieu-Daudé wrote: > On 11/23/21 15:14, Hanna Reitz wrote: >> On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: >>> On 11/23/21 14:42, Hanna Reitz wrote: >>>> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >>>>> From: Alexander Bulekov <alxndr@bu.edu> >>>>> >>>>> Without the previous commit, when running 'make check-qtest-i386' >>>>> with QEMU configured with '--enable-sanitizers' we get: >>>>> >>>>> AddressSanitizer:DEADLYSIGNAL >>>>> ================================================================= >>>>> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >>>>> 0x000000000344 >>>>> ==287878==The signal is caused by a WRITE memory access. >>>>> ==287878==Hint: address points to the zero page. >>>>> #0 0x564b2e5bac27 in blk_inc_in_flight >>>>> block/block-backend.c:1346:5 >>>>> #1 0x564b2e5bb228 in blk_pwritev_part >>>>> block/block-backend.c:1317:5 >>>>> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 >>>>> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 >>>>> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >>>>> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >>>>> >>>>> Add the reproducer for CVE-2021-20196. >>>>> >>>>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >>>>> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> >>>>> [PMD: Rebased, use global test_image] >>>>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>> --- >>>>> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ >>>>> 1 file changed, 21 insertions(+) >>>> Not sure if I’m doing something wrong, but: >>>> >>>> Using the global test_image brings a problem, namely that this test >>>> fails unconditionally (for me at least...?), with the reason being that >>>> the global QEMU instance (launched by qtest_start(), quit by >>>> qtest_end()) still has that image open, so by launching a second >>>> instance concurrently, I get this: >>>> >>>> qemu-system-x86_64: Failed to get "write" lock >>>> Is another process using the image [/tmp/qtest.xV4IxX]? >>> Hmm I had too many odd problems running qtests in parallel so I >>> switched to 'make check-qtest -j1' more than 1 year ago, which >>> is probably why I haven't noticed that issue. >> I’ve run the test with >> >> QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test >> >> so there definitely weren’t any other tests running at the same time. I >> don’t know why you don’t encounter this problem, but it’s caused by the >> concurrent QEMU instance launched in the very same test (qtest_start() >> in main(), and cleaned up by qtest_end() after g_test_run()). > I run all my qtests on top of this patch, otherwise I can't > get any coredump: > https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4bug@amsat.org/ > But I don't think it mattered here... I can give that a try, but since I use coredumpctl, I generally don’t have a problem with one coredump overwriting another (only that I need to give a PID to `coredumpctl gdb` to load not the latest coredump (the qtest) but the one before (qemu)). Hm, perhaps the problem is that I never applied the other series before this one. Also one thing that remains to be tested... Hanna ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-24 14:00 ` Hanna Reitz @ 2021-11-24 15:11 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-24 15:11 UTC (permalink / raw) To: Hanna Reitz, qemu-devel Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Markus Armbruster, Alexander Bulekov, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 11/24/21 15:00, Hanna Reitz wrote: > On 24.11.21 13:50, Philippe Mathieu-Daudé wrote: >> On 11/23/21 15:14, Hanna Reitz wrote: >>> On 23.11.21 14:49, Philippe Mathieu-Daudé wrote: >>>> On 11/23/21 14:42, Hanna Reitz wrote: >>>>> On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: >>>>>> From: Alexander Bulekov <alxndr@bu.edu> >>>>>> >>>>>> Without the previous commit, when running 'make check-qtest-i386' >>>>>> with QEMU configured with '--enable-sanitizers' we get: >>>>>> >>>>>> AddressSanitizer:DEADLYSIGNAL >>>>>> >>>>>> ================================================================= >>>>>> ==287878==ERROR: AddressSanitizer: SEGV on unknown address >>>>>> 0x000000000344 >>>>>> ==287878==The signal is caused by a WRITE memory access. >>>>>> ==287878==Hint: address points to the zero page. >>>>>> #0 0x564b2e5bac27 in blk_inc_in_flight >>>>>> block/block-backend.c:1346:5 >>>>>> #1 0x564b2e5bb228 in blk_pwritev_part >>>>>> block/block-backend.c:1317:5 >>>>>> #2 0x564b2e5bcd57 in blk_pwrite >>>>>> block/block-backend.c:1498:11 >>>>>> #3 0x564b2ca1cdd3 in fdctrl_write_data >>>>>> hw/block/fdc.c:2221:17 >>>>>> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 >>>>>> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 >>>>>> >>>>>> Add the reproducer for CVE-2021-20196. >>>>>> >>>>>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> >>>>>> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> >>>>>> [PMD: Rebased, use global test_image] >>>>>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> >>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>>>> --- >>>>>> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ >>>>>> 1 file changed, 21 insertions(+) >>>>> Not sure if I’m doing something wrong, but: >>>>> >>>>> Using the global test_image brings a problem, namely that this test >>>>> fails unconditionally (for me at least...?), with the reason being >>>>> that >>>>> the global QEMU instance (launched by qtest_start(), quit by >>>>> qtest_end()) still has that image open, so by launching a second >>>>> instance concurrently, I get this: >>>>> >>>>> qemu-system-x86_64: Failed to get "write" lock >>>>> Is another process using the image [/tmp/qtest.xV4IxX]? >>>> Hmm I had too many odd problems running qtests in parallel so I >>>> switched to 'make check-qtest -j1' more than 1 year ago, which >>>> is probably why I haven't noticed that issue. >>> I’ve run the test with >>> >>> QTEST_QEMU_BINARY=$PWD/qemu-system-x86_64 tests/qtest/fdc-test >>> >>> so there definitely weren’t any other tests running at the same time. I >>> don’t know why you don’t encounter this problem, but it’s caused by the >>> concurrent QEMU instance launched in the very same test (qtest_start() >>> in main(), and cleaned up by qtest_end() after g_test_run()). >> I run all my qtests on top of this patch, otherwise I can't >> get any coredump: >> https://lore.kernel.org/qemu-devel/20200707031920.17428-1-f4bug@amsat.org/ >> >> But I don't think it mattered here... > > I can give that a try, but since I use coredumpctl, I generally don’t > have a problem with one coredump overwriting another (only that I need > to give a PID to `coredumpctl gdb` to load not the latest coredump (the > qtest) but the one before (qemu)). I'm not a Fedora expert and use the default, for some reason only the last coredump is available (which in this case is tests/qtest/fdc-test, not useful at all). > Hm, perhaps the problem is that I never applied the other series before > this one. Also one thing that remains to be tested... Don't waste time now, wait for v4 ;) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 2021-11-23 13:49 ` Philippe Mathieu-Daudé 2021-11-23 14:14 ` Hanna Reitz @ 2021-11-23 16:05 ` Alexander Bulekov 1 sibling, 0 replies; 13+ messages in thread From: Alexander Bulekov @ 2021-11-23 16:05 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, qemu-devel, Markus Armbruster, Darren Kenny, Hanna Reitz, Hervé Poussineau, Paolo Bonzini, Gaoning Pan, John Snow On 211123 1449, Philippe Mathieu-Daudé wrote: > On 11/23/21 14:42, Hanna Reitz wrote: > > On 18.11.21 13:06, Philippe Mathieu-Daudé wrote: > >> From: Alexander Bulekov <alxndr@bu.edu> > >> > >> Without the previous commit, when running 'make check-qtest-i386' > >> with QEMU configured with '--enable-sanitizers' we get: > >> > >> AddressSanitizer:DEADLYSIGNAL > >> ================================================================= > >> ==287878==ERROR: AddressSanitizer: SEGV on unknown address > >> 0x000000000344 > >> ==287878==The signal is caused by a WRITE memory access. > >> ==287878==Hint: address points to the zero page. > >> #0 0x564b2e5bac27 in blk_inc_in_flight > >> block/block-backend.c:1346:5 > >> #1 0x564b2e5bb228 in blk_pwritev_part block/block-backend.c:1317:5 > >> #2 0x564b2e5bcd57 in blk_pwrite block/block-backend.c:1498:11 > >> #3 0x564b2ca1cdd3 in fdctrl_write_data hw/block/fdc.c:2221:17 > >> #4 0x564b2ca1b2f7 in fdctrl_write hw/block/fdc.c:829:9 > >> #5 0x564b2dc49503 in portio_write softmmu/ioport.c:201:9 > >> > >> Add the reproducer for CVE-2021-20196. > >> > >> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> > >> Message-Id: <20210319050906.14875-2-alxndr@bu.edu> > >> [PMD: Rebased, use global test_image] > >> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > > > > Not sure if I’m doing something wrong, but: > > > > Using the global test_image brings a problem, namely that this test > > fails unconditionally (for me at least...?), with the reason being that > > the global QEMU instance (launched by qtest_start(), quit by > > qtest_end()) still has that image open, so by launching a second > > instance concurrently, I get this: > > > > qemu-system-x86_64: Failed to get "write" lock > > Is another process using the image [/tmp/qtest.xV4IxX]? > > Hmm I had too many odd problems running qtests in parallel so I > switched to 'make check-qtest -j1' more than 1 year ago, which > is probably why I haven't noticed that issue. > > Using another 'test_image' seems against code reuse principle, > but at this point in release, duplicating it is simpler. Someone > will clean that later =) Maybe it makes sense to run this with -snasphot ? > > > So either we need to use a different image file, or we need to quit the > > global instance before using it (e.g. putting a qtest_end() at the > > beginning of test_cve_*()), although the latter just seems wrong. > > > > Second, I can’t make this test fail. When I apply this patch first (to > > master) and run it, I don’t get a SIGSEGV. > > Is your QEMU built with --enable-sanitizers ? > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 2021-11-18 12:06 [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé @ 2021-11-22 14:55 ` Philippe Mathieu-Daudé 2 siblings, 0 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-22 14:55 UTC (permalink / raw) To: qemu-devel, John Snow, Paolo Bonzini, Markus Armbruster Cc: Laurent Vivier, Kevin Wolf, Thomas Huth, Prasad J Pandit, qemu-block, Darren Kenny, Alexander Bulekov, Hanna Reitz, Hervé Poussineau, Gaoning Pan ping for 6.2? > Alexander Bulekov (1): > tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 > > Philippe Mathieu-Daudé (1): > hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 > > hw/block/fdc.c | 14 +++++++++++++- > tests/qtest/fdc-test.c | 21 +++++++++++++++++++++ > 2 files changed, 34 insertions(+), 1 deletion(-) > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-24 15:13 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-18 12:06 [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 1/2] hw/block/fdc: Kludge missing floppy drive to fix CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-23 13:33 ` Hanna Reitz 2021-11-23 13:57 ` Philippe Mathieu-Daudé 2021-11-18 12:06 ` [PATCH-for-6.2 v3 2/2] tests/qtest/fdc-test: Add a regression test for CVE-2021-20196 Philippe Mathieu-Daudé 2021-11-23 13:42 ` Hanna Reitz 2021-11-23 13:49 ` Philippe Mathieu-Daudé 2021-11-23 14:14 ` Hanna Reitz 2021-11-24 12:50 ` Philippe Mathieu-Daudé 2021-11-24 14:00 ` Hanna Reitz 2021-11-24 15:11 ` Philippe Mathieu-Daudé 2021-11-23 16:05 ` Alexander Bulekov 2021-11-22 14:55 ` [PATCH-for-6.2 v3 0/2] hw/block/fdc: Fix CVE-2021-20196 Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.