Message ID | 20200827114428.1850912-1-ppandit@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/ide: check null block pointer before blk_drain | expand |
Le jeu. 27 août 2020 13:47, P J P <ppandit@redhat.com> a écrit : > From: Prasad J Pandit <pjp@fedoraproject.org> > > While cancelling an i/o operation via ide_cancel_dma_sync(), > check for null block pointer before calling blk_drain(). 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index d997a78e47..038af1cd6b 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -718,7 +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. > */ > - 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. Don't we already have a Launchpad bug for this BTW? trace_ide_cancel_dma_sync_remaining(); > blk_drain(s->blk); > assert(s->bus->dma->aiocb == NULL); > -- > 2.26.2 > > >
+-- 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; + } } 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
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 >
+-- On Thu, 3 Sep 2020, Philippe Mathieu-Daudé wrote --+ | > + 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). Oh, okay will keep it out of the if(s->blk) block. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff --git a/hw/ide/core.c b/hw/ide/core.c index d997a78e47..038af1cd6b 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -718,7 +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. */ - if (s->bus->dma->aiocb) { + if (s->blk && s->bus->dma->aiocb) { trace_ide_cancel_dma_sync_remaining(); blk_drain(s->blk); assert(s->bus->dma->aiocb == NULL);