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
>
