On 1/23/21 5:03 AM, P J P wrote:
From: Prasad J Pandit <p...@fedoraproject.org>
While processing ioport command in 'fdctrl_write_dor', device
controller may select a drive which is not initialised with a
block device. This may result in a NULL pointer dereference.
Add checks to avoid it.
Fixes: CVE-2021-20196
Reported-by: Gaoning Pan <p...@zju.edu.cn>
Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
Signed-off-by: Prasad J Pandit <p...@fedoraproject.org>
---
hw/block/fdc.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 3636874432..13a9470d19 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -1429,7 +1429,9 @@ static void fdctrl_write_dor(FDCtrl *fdctrl, uint32_t
value)
}
}
/* Selected drive */
- fdctrl->cur_drv = value & FD_DOR_SELMASK;
+ if (fdctrl->drives[value & FD_DOR_SELMASK].blk) {
+ fdctrl->cur_drv = value & FD_DOR_SELMASK;
+ }
I don't think this is correct. If you look at get_cur_drv(), it uses the
TDR_BOOTSEL bit to change the logical mappings of "drive 0" or "drive 1"
to be reversed. You don't check that bit here, so you might be checking
the wrong drive.
Plus, the TDR bit can change later, so I think you shouldn't actually
protect the register write like this. Just delete this bit of code. We
ought to protect the drives when we go to use them instead of preventing
the registers from getting "the wrong values".
fdctrl->dor = value;
}
@@ -1894,6 +1896,10 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
uint32_t pos;
cur_drv = get_cur_drv(fdctrl);
+ if (!cur_drv->blk) {
+ FLOPPY_DPRINTF("No drive connected\n");
+ return 0;
+ }
This seems fine ... or at least not worse than the other error handling
we already have here. (Which seems to be ... basically, none. We just
ignore the write and do nothing, which seems wrong. I guess it's better
than a crash... but I don't have the time to do a proper audit of what
this is SUPPOSED to do in this case.)
fdctrl->dsr &= ~FD_DSR_PWRDOWN;
if (!(fdctrl->msr & FD_MSR_RQM) || !(fdctrl->msr & FD_MSR_DIO)) {
FLOPPY_DPRINTF("error: controller not ready for reading\n");
@@ -2420,7 +2426,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 == NULL
+ || blk_pwrite(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
Seems fine, but if we had a drive for the earlier check, will we really
be in a situation where we don't have one now?
BDRV_SECTOR_SIZE, 0) < 0) {
FLOPPY_DPRINTF("error writing sector %d\n",
fd_sector(cur_drv));
Ignore the bit I sent earlier about the qtest reproducer not correlating
to this patch -- it does, I was experiencing an unrelated crash.
--js
