Re: [PATCH] hw/ide: check null block pointer before blk_drain

[Philippe Mathieu-Daudé]

On 9/3/20 1:05 PM, P J P wrote:
> +-- On Mon, 31 Aug 2020, Philippe Mathieu-Daudé wrote --+
> | > +++ b/hw/ide/core.c
> | > @@ -718,7 +718,7 @@ void ide_cancel_dma_sync(IDEState *s)
> | > -    if (s->bus->dma->aiocb) {
> | > +    if (s->blk && s->bus->dma->aiocb) {
> | 
> | But s->blk mustn't be null here... IMHO we should assert() here and add a
> | check earlier.
> 
> ===
> 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..51bb9c9abc 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -295,8 +295,11 @@ 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));
> -            bm->status &= ~BM_STATUS_DMAING;
> +            IDEState *s = idebus_active_if(bm->bus);
> +            if (s->blk) {
> +                ide_cancel_dma_sync(s);
> +                bm->status &= ~BM_STATUS_DMAING;

If you don't clear this bit the guest might keep retrying (looping).

> +            }
>          } else {
>              bm->cur_addr = bm->addr;
>              if (!(bm->status & BM_STATUS_DMAING)) {
> ===
> 
> 
> Does it look okay?
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
>