Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync

[Li Qiang]

P J P <ppandit@redhat.com> 于2020年9月4日周五 上午2:34写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> When cancelling an i/o operation via ide_cancel_dma_sync(),
> a block pointer may be null. Add check to avoid null pointer
> dereference.
>
>  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fide_nullptr1
>  ==1803100==Hint: address points to the zero page.
>  #0 blk_bs ../block/block-backend.c:714
>  #1 blk_drain ../block/block-backend.c:1715
>  #2 ide_cancel_dma_sync ../hw/ide/core.c:723
>  #3 bmdma_cmd_writeb ../hw/ide/pci.c:298
>  #4 bmdma_write ../hw/ide/piix.c:75
>  #5 memory_region_write_accessor ../softmmu/memory.c:483
>  #6 access_with_adjusted_size ../softmmu/memory.c:544
>  #7 memory_region_dispatch_write ../softmmu/memory.c:1465
>  #8 flatview_write_continue ../exec.c:3176
>  ...
>
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/ide/core.c | 1 +
>  hw/ide/pci.c  | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> Update v2: use an assert() call
>  -> https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index f76f7e5234..8105187f49 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -718,6 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
>       * whole DMA operation will be submitted to disk with a single
>       * aio operation with preadv/pwritev.
>       */
> +    assert(s->blk);
>      if (s->bus->dma->aiocb) {
>          trace_ide_cancel_dma_sync_remaining();
>          blk_drain(s->blk);
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index b50091b615..b47e675456 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -295,7 +295,10 @@ void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val)
>      /* Ignore writes to SSBM if it keeps the old value */
>      if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
>          if (!(val & BM_CMD_START)) {
> -            ide_cancel_dma_sync(idebus_active_if(bm->bus));
> +            IDEState *s = idebus_active_if(bm->bus);
> +            if (s->blk) {
> +                ide_cancel_dma_sync(s);
> +            }
>              bm->status &= ~BM_STATUS_DMAING;
>          } else {
>              bm->cur_addr = bm->addr;
> --

Hello Prasad,
'bmdma_cmd_writeb' is in the bmdma layer, I think it is better to not
touch the IDE layer(check the 's->blk').

I think it is better to defer this check to 'ide_cancel_dma_sync'.
'ide_cancel_dma_sync' is also called by 'cmd_device_reset' and all of
the handlers of 'ide_cmd_table' will check
whether the 's->blk' is NULL in the beginning of 'ide_exec_cmd'.

So I think it is reasonable to check 's->blk' at the begining of
'ide_cancel_dma_sync'.

I'm not a blk expert, please correct me if I'm wrong.

Thanks,
Li Qiang

> 2.26.2
>
>