Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)

From: Alexander Bulekov <alxndr@bu.edu>
To: P J P <ppandit@redhat.com>
Cc: Gaoning Pan <pgn@zju.edu.cn>, John Snow <jsnow@redhat.com>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Prasad J Pandit <pjp@fedoraproject.org>
Subject: Re: [PATCH] fdc: check drive block device before usage (CVE-2021-20196)
Date: Sat, 23 Jan 2021 12:47:25 -0500	[thread overview]
Message-ID: <20210123174725.slddy75u7egga43n@mozz.bu.edu> (raw)
In-Reply-To: <20210123100345.642933-1-ppandit@redhat.com>

Hello,
Here is a reproducer for this bug found by the OSS-Fuzz fuzzer:

cat << EOF | qemu-system-i386 -display none -machine accel=qtest \
-m 512M -machine pc -device floppy,unit=1,id=floppy0,drive=disk0 \
-drive id=disk0,file=null-co://,file.read-zeroes=on,if=none,format=raw \
-qtest stdio
outw 0x3f4 0x2500
outb 0x3f5 0x81
outb 0x3f5 0x0
outb 0x3f5 0x0
outb 0x3f5 0x0
outw 0x3f4 0x0
outw 0x3f4 0x4000
outw 0x3f4 0x13
outb 0x3f5 0x1
outw 0x3f2 0x1405
outw 0x3f4 0x0
EOF

-Alex

On 210123 1533, P J P wrote:
> From: Prasad J Pandit <pjp@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 <pgn@zju.edu.cn>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1912780
> Signed-off-by: Prasad J Pandit <pjp@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;
> +    }
>  
>      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;
> +    }
>      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,
>                             BDRV_SECTOR_SIZE, 0) < 0) {
>                  FLOPPY_DPRINTF("error writing sector %d\n",
>                                 fd_sector(cur_drv));
> -- 
> 2.29.2
> 
> 