Re: [PATCH v2] fdc: check null block pointer before r/w data transfer

[John Snow <jsnow@redhat.com>]

On 9/22/20 5:27 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While transferring data via fdctrl_read/write_data() routines,
> 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 | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Update v2: treat NULL blk pointer as an error
>    -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg06642.html
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 224bac504f..bf968dc66f 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1923,7 +1923,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
>                                      fd_sector(cur_drv));
>                       return 0;
>                   }
> -            if (blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
> +            if (!cur_drv->blk
> +                || blk_pread(cur_drv->blk, fd_offset(cur_drv), fdctrl->fifo,
>                             BDRV_SECTOR_SIZE)
>                   < 0) {
>                   FLOPPY_DPRINTF("error getting sector %d\n",
> @@ -2427,7 +2428,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));
> --
> 2.26.2
> 

This patch actually looks good to me. Though again, I assume it can be 
rolled into the most recent issue that actually grabbed my attention:

https://gitlab.com/qemu-project/qemu/-/issues/338

And we can credit both reporters (and Alexander) and solve all of these 
issues all at once.

I am therefore dropping this patch from my queue. Please let me know if 
I am mistaken.

Thanks,
--js