| Message ID | 20200827113806.1850687-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fdc: check null block pointer before blk_pwrite | expand |
+-- 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
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 > >
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.
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
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 :) [...]
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
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)
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 >
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)); >
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));