* Re: [PATCH] fdc: check null block pointer before blk_pwrite
2020-08-27 11:38 [PATCH] fdc: check null block pointer before blk_pwrite P J P
@ 2020-09-15 12:47 ` P J P
2020-09-18 10:52 ` Li Qiang
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: P J P @ 2020-09-15 12:47 UTC (permalink / raw)
To: John Snow; +Cc: Ruhr-University, QEMU Developers, qemu-block
+-- On Thu, 27 Aug 2020, P J P wrote --+
| While transferring data via fdctrl_write_data(), check that
| current drive does not have a null block pointer. Avoid
| null pointer dereference.
|
| -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
| ==1658854==Hint: address points to the zero page.
| #0 blk_inc_in_flight block/block-backend.c:1327
| #1 blk_prw block/block-backend.c:1299
| #2 blk_pwrite block/block-backend.c:1464
| #3 fdctrl_write_data hw/block/fdc.c:2418
| #4 fdctrl_write hw/block/fdc.c:962
| #5 portio_write ioport.c:205
| #6 memory_region_write_accessor memory.c:483
| #7 access_with_adjusted_size memory.c:544
| #8 memory_region_dispatch_write memory.c:1476
|
| Reported-by: Ruhr-University <bugs-syssec@rub.de>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
| hw/block/fdc.c | 3 ++-
| 1 file changed, 2 insertions(+), 1 deletion(-)
|
| diff --git a/hw/block/fdc.c b/hw/block/fdc.c
| index e9ed3eef45..dedadac68a 100644
| --- a/hw/block/fdc.c
| +++ b/hw/block/fdc.c
| @@ -2419,7 +2419,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
| if (pos == FD_SECTOR_LEN - 1 ||
| fdctrl->data_pos == fdctrl->data_len) {
| cur_drv = get_cur_drv(fdctrl);
| - if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| + if (cur_drv->blk
| + && blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
| BDRV_SECTOR_SIZE, 0) < 0) {
| FLOPPY_DPRINTF("error writing sector %d\n",
| fd_sector(cur_drv));
|
Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] fdc: check null block pointer before blk_pwrite
2020-08-27 11:38 [PATCH] fdc: check null block pointer before blk_pwrite P J P
2020-09-15 12:47 ` P J P
@ 2020-09-18 10:52 ` Li Qiang
2021-03-19 5:09 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Alexander Bulekov
2021-05-18 17:30 ` [PATCH] fdc: check null block pointer before blk_pwrite John Snow
3 siblings, 0 replies; 12+ messages in thread
From: Li Qiang @ 2020-09-18 10:52 UTC (permalink / raw)
To: P J P
Cc: Ruhr-University, John Snow, QEMU Developers, qemu-block, Prasad J Pandit
P J P <ppandit@redhat.com> 于2020年8月27日周四 下午7:41写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While transferring data via fdctrl_write_data(), check that
> current drive does not have a null block pointer. Avoid
> null pointer dereference.
>
> -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
> ==1658854==Hint: address points to the zero page.
> #0 blk_inc_in_flight block/block-backend.c:1327
> #1 blk_prw block/block-backend.c:1299
> #2 blk_pwrite block/block-backend.c:1464
> #3 fdctrl_write_data hw/block/fdc.c:2418
> #4 fdctrl_write hw/block/fdc.c:962
> #5 portio_write ioport.c:205
> #6 memory_region_write_accessor memory.c:483
> #7 access_with_adjusted_size memory.c:544
> #8 memory_region_dispatch_write memory.c:1476
>
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/block/fdc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e9ed3eef45..dedadac68a 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2419,7 +2419,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> if (pos == FD_SECTOR_LEN - 1 ||
> fdctrl->data_pos == fdctrl->data_len) {
> cur_drv = get_cur_drv(fdctrl);
> - if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> + if (cur_drv->blk
> + && blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> BDRV_SECTOR_SIZE, 0) < 0) {
> FLOPPY_DPRINTF("error writing sector %d\n",
> fd_sector(cur_drv));
> --
Hello Prasad,
There are several pattern to address this NULL check in fdc.c.
I think it is better to consider this as an error. Just like the check
in 'fdctrl_format_sector':
if (cur_drv->blk == NULL ||
blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
BDRV_SECTOR_SIZE, 0) < 0) {
FLOPPY_DPRINTF("error formatting sector %d\n", fd_sector(cur_drv));
fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
} else {
Also there seems exists the same issue in 'fdctrl_read_data'
Thanks,
Li Qiang
> 2.26.2
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2020-08-27 11:38 [PATCH] fdc: check null block pointer before blk_pwrite P J P
2020-09-15 12:47 ` P J P
2020-09-18 10:52 ` Li Qiang
@ 2021-03-19 5:09 ` Alexander Bulekov
2021-03-19 5:09 ` [PATCH 2/2] floppy: add a regression test for CVE-2021-20196 Alexander Bulekov
2021-03-19 5:53 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Markus Armbruster
2021-05-18 17:30 ` [PATCH] fdc: check null block pointer before blk_pwrite John Snow
3 siblings, 2 replies; 12+ messages in thread
From: Alexander Bulekov @ 2021-03-19 5:09 UTC (permalink / raw)
To: John Snow
Cc: Li Qiang, Michael Tokarev, QEMU Developers, qemu-block,
Alexander Bulekov
dd if=/dev/zero of=/tmp/fda.img bs=1024 count=1440
cat << EOF | ./qemu-system-i386 -nographic -m 512M -nodefaults \
-accel qtest -fda /tmp/fda.img -qtest stdio
outw 0x3f4 0x0500
outb 0x3f5 0x00
outb 0x3f5 0x00
outw 0x3f4 0x00
outb 0x3f5 0x00
outw 0x3f1 0x0400
outw 0x3f4 0x0
outw 0x3f4 0x00
outb 0x3f5 0x0
outb 0x3f5 0x01
outw 0x3f1 0x0500
outb 0x3f5 0x00
EOF
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
Might be useful for reproducing/regression testing
tests/qtest/fuzz-test.c | 54 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 00149abec7..62ececc66f 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -24,6 +24,58 @@ static void test_lp1878642_pci_bus_get_irq_level_assert(void)
qtest_quit(s);
}
+/*
+ * ERROR: AddressSanitizer: SEGV on unknown address 0x000000000344
+ * The signal is caused by a WRITE memory access.
+ * #0 0x555559d7f519 in blk_inc_in_flight /block/block-backend.c:1356:5
+ * #1 0x555559d7f519 in blk_prw /block/block-backend.c:1328:5
+ * #2 0x555559d81673 in blk_pwrite /block/block-backend.c:1501:15
+ * #3 0x555558fc763f in fdctrl_write_data /hw/block/fdc.c:2414:17
+ * #4 0x555558fc763f in fdctrl_write /hw/block/fdc.c:967:9
+ * #5 0x5555594a622c in memory_region_write_accessor /softmmu/memory.c:491:5
+ * #6 0x5555594a5c93 in access_with_adjusted_size /softmmu/memory.c:552:18
+ * #7 0x5555594a54f0 in memory_region_dispatch_write /softmmu/memory.c
+ * #8 0x55555964a686 in flatview_write_continue /softmmu/physmem.c:2776:23
+ * #9 0x55555963fde8 in flatview_write /softmmu/physmem.c:2816:14
+ * #10 0x55555963fde8 in address_space_write /softmmu/physmem.c:2908:18
+ * #11 0x55555968f8a1 in cpu_outb /softmmu/ioport.c:60:5
+ * #12 0x5555597d5619 in qtest_process_command /softmmu/qtest.c:479:13
+ * #13 0x5555597d34bf in qtest_process_inbuf /softmmu/qtest.c:797:9
+ * #14 0x555559b4539d in fd_chr_read /chardev/char-fd.c:68:9
+ * #15 0x7ffff7b7dc3e in g_main_context_dispatch
+ * #16 0x55555a4facdc in glib_pollfds_poll /util/main-loop.c:231:9
+ * #17 0x55555a4facdc in os_host_main_loop_wait /util/main-loop.c:254:5
+ * #18 0x55555a4facdc in main_loop_wait /util/main-loop.c:530:11
+ * #19 0x555559a6dec9 in qemu_main_loop /softmmu/runstate.c:725:9
+ * #20 0x5555581a3085 in main /softmmu/main.c:50:5
+ */
+static void test_fdc_cve_2020_25741(void)
+{
+ QTestState *s;
+ int fd;
+ char tmpdisk[] = "/tmp/fda.XXXXXX";
+ fd = mkstemp(tmpdisk);
+ assert(fd >= 0);
+ ftruncate(fd, 1440 * 1024);
+ close(fd);
+
+ s = qtest_initf("-nographic -m 512M -nodefaults "
+ "-drive file=%s,format=raw,if=floppy", tmpdisk);
+ qtest_outw(s, 0x3f4, 0x0500);
+ qtest_outb(s, 0x3f5, 0x00);
+ qtest_outb(s, 0x3f5, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outb(s, 0x3f5, 0x00);
+ qtest_outw(s, 0x3f1, 0x0400);
+ qtest_outw(s, 0x3f4, 0x0);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outb(s, 0x3f5, 0x0);
+ qtest_outb(s, 0x3f5, 0x01);
+ qtest_outw(s, 0x3f1, 0x0500);
+ qtest_outb(s, 0x3f5, 0x00);
+ qtest_quit(s);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -33,6 +85,8 @@ int main(int argc, char **argv)
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
test_lp1878642_pci_bus_get_irq_level_assert);
+ qtest_add_func("fuzz/test_fdc_cve_2020_25741",
+ test_fdc_cve_2020_25741);
}
return g_test_run();
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 2/2] floppy: add a regression test for CVE-2021-20196
2021-03-19 5:09 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Alexander Bulekov
@ 2021-03-19 5:09 ` Alexander Bulekov
2021-03-19 5:53 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Markus Armbruster
1 sibling, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2021-03-19 5:09 UTC (permalink / raw)
To: John Snow
Cc: Li Qiang, Michael Tokarev, QEMU Developers, qemu-block,
Alexander Bulekov
dd if=/dev/zero of=/tmp/fda.img bs=1024 count=1440
cat << EOF | ./qemu-system-i386 -nographic -m 512M -nodefaults \
-accel qtest -fda /tmp/fda.img -qtest stdio
outw 0x3f4 0x0500
outb 0x3f5 0x00
outb 0x3f5 0x00
outw 0x3f4 0x00
outb 0x3f5 0x00
outw 0x3f1 0x0400
outw 0x3f4 0x0
outw 0x3f4 0x00
outb 0x3f5 0x0
outb 0x3f5 0x01
outw 0x3f1 0x0500
outb 0x3f5 0x00
EOF
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
Since this looks very similar to CVE-2021-20196 (I believe Li pointed
out that issue in this thread), I'm also posting the reproducer for that
here.
tests/qtest/fuzz-test.c | 57 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
index 62ececc66f..8e4ccdaca8 100644
--- a/tests/qtest/fuzz-test.c
+++ b/tests/qtest/fuzz-test.c
@@ -76,6 +76,61 @@ static void test_fdc_cve_2020_25741(void)
qtest_quit(s);
}
+
+/*
+ * ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000344
+ * The signal is caused by a WRITE memory access.
+ * #0 0x555556494543 in blk_inc_in_flight /block/block-backend.c:1356:5
+ * #1 0x555556494543 in blk_prw /block/block-backend.c:1328:5
+ * #2 0x555556494d03 in blk_pread /block/block-backend.c:1491:15
+ * #3 0x555555ec8986 in fdctrl_read_data /hw/block/fdc.c:1910:17
+ * #4 0x555555ec8986 in fdctrl_read /hw/block/fdc.c:936:18
+ * #5 0x5555563f26b7 in portio_read /softmmu/ioport.c:185:25
+ * #6 0x55555636908a in memory_region_read_accessor /softmmu/memory.c:442:11
+ * #7 0x55555635ec25 in access_with_adjusted_size /softmmu/memory.c:552:18
+ * #8 0x55555635ec25 in memory_region_dispatch_read1 /softmmu/memory.c:1420:16
+ * #9 0x55555635ec25 in memory_region_dispatch_read /softmmu/memory.c:1448:9
+ * #10 0x555556248aa7 in flatview_read_continue /softmmu/physmem.c:2810:23
+ * #11 0x5555563f18f0 in address_space_read /include/exec/memory.h:2494:26
+ * #12 0x5555563f18f0 in cpu_inw /softmmu/ioport.c:99:5
+ * #13 0x55555637619c in qtest_process_command /softmmu/qtest.c:502:21
+ * #14 0x55555637535d in qtest_process_inbuf /softmmu/qtest.c:797:9
+ * #15 0x555556405f9c in tcp_chr_read /chardev/char-socket.c:557:13
+ * #16 0x7ffff6f8ac3e in g_main_context_dispatch
+ * #17 0x5555567479f1 in glib_pollfds_poll /util/main-loop.c:231:9
+ * #18 0x5555567479f1 in os_host_main_loop_wait /util/main-loop.c:254:5
+ * #19 0x5555567479f1 in main_loop_wait /util/main-loop.c:530:11
+ * #20 0x5555562d9ee4 in qemu_main_loop /softmmu/runstate.c:725:9
+ * #21 0x555555d5b615 in main /softmmu/main.c:50:5
+*/
+static void test_fdc_cve_2021_20196(void)
+{
+ QTestState *s;
+ int fd;
+ char tmpdisk[] = "/tmp/fda.XXXXXX";
+ fd = mkstemp(tmpdisk);
+ assert(fd >= 0);
+ ftruncate(fd, 1440 * 1024);
+ close(fd);
+
+ s = qtest_initf("-nographic -m 512M -nodefaults "
+ "-drive file=%s,format=raw,if=floppy", tmpdisk);
+ qtest_outw(s, 0x3f2, 0x04);
+ qtest_outw(s, 0x3f4, 0x0200);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f4, 0x00);
+ qtest_outw(s, 0x3f2, 0x01);
+ qtest_inw(s, 0x3f4);
+ qtest_quit(s);
+ unlink(tmpdisk);
+}
+
int main(int argc, char **argv)
{
const char *arch = qtest_get_arch();
@@ -87,6 +142,8 @@ int main(int argc, char **argv)
test_lp1878642_pci_bus_get_irq_level_assert);
qtest_add_func("fuzz/test_fdc_cve_2020_25741",
test_fdc_cve_2020_25741);
+ qtest_add_func("fuzz/test_fdc_cve_2021_20196",
+ test_fdc_cve_2021_20196);
}
return g_test_run();
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 5:09 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Alexander Bulekov
2021-03-19 5:09 ` [PATCH 2/2] floppy: add a regression test for CVE-2021-20196 Alexander Bulekov
@ 2021-03-19 5:53 ` Markus Armbruster
2021-03-19 9:26 ` Paolo Bonzini
1 sibling, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2021-03-19 5:53 UTC (permalink / raw)
To: Alexander Bulekov
Cc: Li Qiang, Michael Tokarev, John Snow, QEMU Developers, qemu-block
Alexander Bulekov <alxndr@bu.edu> writes:
> dd if=/dev/zero of=/tmp/fda.img bs=1024 count=1440
> cat << EOF | ./qemu-system-i386 -nographic -m 512M -nodefaults \
> -accel qtest -fda /tmp/fda.img -qtest stdio
> outw 0x3f4 0x0500
> outb 0x3f5 0x00
> outb 0x3f5 0x00
> outw 0x3f4 0x00
> outb 0x3f5 0x00
> outw 0x3f1 0x0400
> outw 0x3f4 0x0
> outw 0x3f4 0x00
> outb 0x3f5 0x0
> outb 0x3f5 0x01
> outw 0x3f1 0x0500
> outb 0x3f5 0x00
> EOF
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
I guess this is a reproducer. Please also describe actual and expected
result. Same for PATCH 2.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 5:53 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Markus Armbruster
@ 2021-03-19 9:26 ` Paolo Bonzini
2021-03-19 9:54 ` Markus Armbruster
2021-03-19 14:52 ` Alexander Bulekov
0 siblings, 2 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-03-19 9:26 UTC (permalink / raw)
To: Markus Armbruster, Alexander Bulekov
Cc: John Snow, Michael Tokarev, Li Qiang, QEMU Developers, qemu-block
On 19/03/21 06:53, Markus Armbruster wrote:
> I guess this is a reproducer. Please also describe actual and expected
> result. Same for PATCH 2.
Isn't it in the patch itself?
Alexander, I think these reproducers are self-contained enough (no
writes to PCI configuration space to set up the device) that we can
place them in fdc-test.c.
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 9:26 ` Paolo Bonzini
@ 2021-03-19 9:54 ` Markus Armbruster
2021-03-19 10:17 ` Paolo Bonzini
2021-03-19 14:51 ` Alexander Bulekov
2021-03-19 14:52 ` Alexander Bulekov
1 sibling, 2 replies; 12+ messages in thread
From: Markus Armbruster @ 2021-03-19 9:54 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, Li Qiang, Michael Tokarev, QEMU Developers,
Alexander Bulekov, John Snow
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 19/03/21 06:53, Markus Armbruster wrote:
>> I guess this is a reproducer. Please also describe actual and expected
>> result. Same for PATCH 2.
>
> Isn't it in the patch itself?
A commit message should tell me what the patch is trying to accomplish.
This commit message's title tells me it's a test for a CVE. Okay. The
body additionally gives me the reproducer. To be useful, a reproducer
needs to come with actual and expected result. Yes, I can find those in
the patch. But I could find the reproducer there, too. If you're nice
enough to save me the trouble of digging through the patch for the
reproducer (thanks), please consider saving me the trouble digging for
the information I need to make use of it (thanks again). That's all :)
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 9:54 ` Markus Armbruster
@ 2021-03-19 10:17 ` Paolo Bonzini
2021-03-19 14:51 ` Alexander Bulekov
1 sibling, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2021-03-19 10:17 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-block, Li Qiang, Michael Tokarev, QEMU Developers,
Alexander Bulekov, John Snow
On 19/03/21 10:54, Markus Armbruster wrote:
> A commit message should tell me what the patch is trying to accomplish.
>
> This commit message's title tells me it's a test for a CVE. Okay. The
> body additionally gives me the reproducer. To be useful, a reproducer
> needs to come with actual and expected result. Yes, I can find those in
> the patch. But I could find the reproducer there, too. If you're nice
> enough to save me the trouble of digging through the patch for the
> reproducer (thanks), please consider saving me the trouble digging for
> the information I need to make use of it (thanks again). That's all:)
FWIW I don't think in this case there is an expected result other than
not crashing, but yeah it may be worth adding in the commit message "for
CVE-2020-25741, a {crash,undefined behavior,ASAN violation} in func_name".
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 9:54 ` Markus Armbruster
2021-03-19 10:17 ` Paolo Bonzini
@ 2021-03-19 14:51 ` Alexander Bulekov
1 sibling, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2021-03-19 14:51 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-block, Li Qiang, Michael Tokarev, QEMU Developers,
Paolo Bonzini, John Snow
On 210319 1054, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
> > On 19/03/21 06:53, Markus Armbruster wrote:
> >> I guess this is a reproducer. Please also describe actual and expected
> >> result. Same for PATCH 2.
> >
> > Isn't it in the patch itself?
>
> A commit message should tell me what the patch is trying to accomplish.
>
> This commit message's title tells me it's a test for a CVE. Okay. The
> body additionally gives me the reproducer. To be useful, a reproducer
> needs to come with actual and expected result. Yes, I can find those in
> the patch. But I could find the reproducer there, too. If you're nice
> enough to save me the trouble of digging through the patch for the
> reproducer (thanks), please consider saving me the trouble digging for
> the information I need to make use of it (thanks again). That's all :)
>
> [...]
>
Ok sounds good. I posted this in-reply-to patch [1] from August 2020,
which had a stacktrace, and I hoped that would provide enough context.
However, that depends on the email-viewer and I see how that context
would be lost if/once these reproducer patches are applied.
[1] https://lore.kernel.org/qemu-devel/20200827113806.1850687-1-ppandit@redhat.com/
(https://lists.nongnu.org/archive doesn't display this discussion
as a child of that message)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] floppy: add a regression test for CVE-2020-25741
2021-03-19 9:26 ` Paolo Bonzini
2021-03-19 9:54 ` Markus Armbruster
@ 2021-03-19 14:52 ` Alexander Bulekov
1 sibling, 0 replies; 12+ messages in thread
From: Alexander Bulekov @ 2021-03-19 14:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: qemu-block, Li Qiang, Michael Tokarev, Markus Armbruster,
QEMU Developers, John Snow
On 210319 1026, Paolo Bonzini wrote:
> On 19/03/21 06:53, Markus Armbruster wrote:
> > I guess this is a reproducer. Please also describe actual and expected
> > result. Same for PATCH 2.
>
> Isn't it in the patch itself?
>
> Alexander, I think these reproducers are self-contained enough (no writes to
> PCI configuration space to set up the device) that we can place them in
> fdc-test.c.
>
Will do
-Alex
> Paolo
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] fdc: check null block pointer before blk_pwrite
2020-08-27 11:38 [PATCH] fdc: check null block pointer before blk_pwrite P J P
` (2 preceding siblings ...)
2021-03-19 5:09 ` [PATCH 1/2] floppy: add a regression test for CVE-2020-25741 Alexander Bulekov
@ 2021-05-18 17:30 ` John Snow
3 siblings, 0 replies; 12+ messages in thread
From: John Snow @ 2021-05-18 17:30 UTC (permalink / raw)
To: P J P; +Cc: Ruhr-University, QEMU Developers, qemu-block, Prasad J Pandit
On 8/27/20 7:38 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While transferring data via fdctrl_write_data(), check that
> current drive does not have a null block pointer. Avoid
> null pointer dereference.
>
Hi PJP. I assume this patch actually covers the exact same thing that
the other if cur_drv->blk patch we have been discussing recently does.
You may want to combine the Reported-By lines for both.
I am dropping this patch from my queue in favor of pursuing our other,
more recent and active thread, which I am also now tracking via this
gitlab issue:
https://gitlab.com/qemu-project/qemu/-/issues/338
> -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Ffdc_nullptr1
> ==1658854==Hint: address points to the zero page.
> #0 blk_inc_in_flight block/block-backend.c:1327
> #1 blk_prw block/block-backend.c:1299
> #2 blk_pwrite block/block-backend.c:1464
> #3 fdctrl_write_data hw/block/fdc.c:2418
> #4 fdctrl_write hw/block/fdc.c:962
> #5 portio_write ioport.c:205
> #6 memory_region_write_accessor memory.c:483
> #7 access_with_adjusted_size memory.c:544
> #8 memory_region_dispatch_write memory.c:1476
>
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/block/fdc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e9ed3eef45..dedadac68a 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2419,7 +2419,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
> if (pos == FD_SECTOR_LEN - 1 ||
> fdctrl->data_pos == fdctrl->data_len) {
> cur_drv = get_cur_drv(fdctrl);
> - if (blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> + if (cur_drv->blk
> + && blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> BDRV_SECTOR_SIZE, 0) < 0) {
> FLOPPY_DPRINTF("error writing sector %d\n",
> fd_sector(cur_drv));
>
^ permalink raw reply [flat|nested] 12+ messages in thread