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

[John Snow <jsnow@redhat.com>]

On 1/23/21 5:03 AM, 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;
> +    }

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