| Message ID | 20200903183138.2161977-1-ppandit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/ide: check null block before _cancel_dma_sync | expand |
+-- On Fri, 4 Sep 2020, P J P wrote --+
| 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;
Ping...! (needs review)
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
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 > >
+-- On Fri, 18 Sep 2020, Li Qiang wrote --+ | Update v2: use an assert() call | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html ... | 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'. * Yes, earlier patch v1 above does the same. * From Peter's reply in another thread of similar issue I gather, issue is setting 'blk' to NULL, even erroneously. So (blk == NULL) check should be done where 'blk' is set to null, rather than where it is dereferenced. * At the dereference point, assert(3) is good. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
P J P <ppandit@redhat.com> 于2020年9月18日周五 下午6:26写道： > > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | Update v2: use an assert() call > | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html > ... > | 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'. > > * Yes, earlier patch v1 above does the same. > > * From Peter's reply in another thread of similar issue I gather, issue is > setting 'blk' to NULL, even erroneously. So (blk == NULL) check should be > done where 'blk' is set to null, rather than where it is dereferenced. > I don't find anywhere set the 'blk' to NULL. I think this NULL deference is caused by following: In 'pci_ide_create_devs' we call 'ide_create_drive', in the latter function it will create the 's->blk'. However it is not for every IDEBus. IDEBus[0]: ifs[2] IDEBus[1]: ifs[2] The 'ide_create_drive' will only initialize the 'IDEBus[0].ifs[0]' and 'IDEBus[1].ifs[0]' from the reproducer command line. So the 'IDEBus[0].ifs[1]' and 'IDEBus[1].ifs[1]' s blk will be NULL. In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. If the guest set this to 1. Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' will be NULL. So from your (Peter's) saying, we need to check the value in 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest set a valid 'bus->unit'. This can also work I think. As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is enough and also this is more consistent with the other functions. 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of the 'ide_cmd_table' handler. BTW, where is the Peter's email saying this, just want to learn something, :). Thanks, Li Qiang > * At the dereference point, assert(3) is good. > > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D >
P J P <ppandit@redhat.com> 于2020年9月29日周二 下午2:22写道： > > Hello Li, > > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | P J P <ppandit@redhat.com> 于2020年9月18日周五 下午6:26写道： > | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+ > | > | Update v2: use an assert() call > | > | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html > | > | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue > | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs. > | If the guest set this to 1. > | > | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk' > | will be NULL. > > Right, guest does select the drive via > > portio_write > ->ide_ioport_write > case ATA_IOPORT_WR_DEVICE_HEAD: > /* FIXME: HOB readback uses bit 7 */ > bus->ifs[0].select = (val & ~0x10) | 0xa0; > bus->ifs[1].select = (val | 0x10) | 0xa0; > /* select drive */ > bus->unit = (val >> 4) & 1; <== set bus->unit=0x1 > break; > > > | So from your (Peter's) saying, we need to check the value in > | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest > | set a valid 'bus->unit'. This can also work I think. > > Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails. > > === > diff --git a/hw/ide/core.c b/hw/ide/core.c > index f76f7e5234..cb55cc8b0f 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr, > uint_) > bus->ifs[0].select = (val & ~0x10) | 0xa0; > bus->ifs[1].select = (val | 0x10) | 0xa0; > /* select drive */ > + uint8_t bu = bus->unit; > bus->unit = (val >> 4) & 1; > + if (!bus->ifs[bus->unit].blk) { > + bus->unit = bu; > + } > break; > default: > > qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion `s->bus->dma->aiocb == NULL' failed. > Aborted (core dumped) This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It also change the 'buf->ifs[0].select'. Also there maybe some other corner case that causes some inconsistent. And if we choice this method we need to deep into the more ahci-spec to know how things really going. > === > > | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the > | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is > | enough and also this is more consistent with the other functions. > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of > | the 'ide_cmd_table' handler. > > Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in > ide_cancel_dma_sync(). I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' method. Some little different with your earlier patch. Anyway, let the maintainer do the choices. Thanks, Li Qiang > > | BTW, where is the Peter's email saying this, just want to learn something, > | :). > > -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05820.html > > Thank you. > -- > Prasad J Pandit / Red Hat Product Security Team > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
+-- On Tue, 29 Sep 2020, Li Qiang wrote --+
| P J P <ppandit@redhat.com> 于2020年9月29日周二 下午2:22写道：
| > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | P J P <ppandit@redhat.com> 于2020年9月18日周五 下午6:26写道：
| > | > +-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| > | > | Update v2: use an assert() call
| > | > | ->https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg08336.html
| > |
| > | In 'ide_ioport_write' the guest can set 'bus->unit' to 0 or 1 by issue
| > | 'ATA_IOPORT_WR_DEVICE_HEAD'. So this case the guest can set the active ifs.
| > | If the guest set this to 1.
| > |
| > | Then in 'idebus_active_if' will return 'IDEBus.ifs[1]' and thus the 's->blk'
| > | will be NULL.
| >
| > Right, guest does select the drive via
| >
| > portio_write
| > ->ide_ioport_write
| > case ATA_IOPORT_WR_DEVICE_HEAD:
| > /* FIXME: HOB readback uses bit 7 */
| > bus->ifs[0].select = (val & ~0x10) | 0xa0;
| > bus->ifs[1].select = (val | 0x10) | 0xa0;
| > /* select drive */
| > bus->unit = (val >> 4) & 1; <== set bus->unit=0x1
| > break;
| >
| >
| > | So from your (Peter's) saying, we need to check the value in
| > | 'ATA_IOPORT_WR_DEVICE_HEAD' handler. To say if the guest
| > | set a valid 'bus->unit'. This can also work I think.
| >
| > Yes, with the following fix, an assert(3) in ide_cancel_dma_sync fails.
| >
| > ===
| > diff --git a/hw/ide/core.c b/hw/ide/core.c
| > index f76f7e5234..cb55cc8b0f 100644
| > --- a/hw/ide/core.c
| > +++ b/hw/ide/core.c
| > @@ -1300,7 +1300,11 @@ void ide_ioport_write(void *opaque, uint32_t addr,
| > uint_)
| > bus->ifs[0].select = (val & ~0x10) | 0xa0;
| > bus->ifs[1].select = (val | 0x10) | 0xa0;
| > /* select drive */
| > + uint8_t bu = bus->unit;
| > bus->unit = (val >> 4) & 1;
| > + if (!bus->ifs[bus->unit].blk) {
| > + bus->unit = bu;
| > + }
| > break;
| > default:
| >
| > qemu-system-x86_64: ../hw/ide/core.c:724: ide_cancel_dma_sync: Assertion `s->bus->dma->aiocb == NULL' failed.
| > Aborted (core dumped)
|
| This is what I am worried, in the 'ide_ioport_write' set the 'bus->unit'. It
| also change the 'buf->ifs[0].select'. Also there maybe some other corner
| case that causes some inconsistent. And if we choice this method we need to
| deep into the more ahci-spec to know how things really going.
|
| > ===
| >
| > | As we the 'ide_exec_cmd' and other functions in 'hw/ide/core.c' check the
| > | 's->blk' directly. I think we just check it in 'ide_cancel_dma_sync' is
| > | enough and also this is more consistent with the other functions.
| > | 'ide_cancel_dma_sync' is also called by 'cmd_device_reset' which is one of
| > | the 'ide_cmd_table' handler.
| >
| > Yes, I'm okay with either approach. Earlier patch v1 checks 's->blk' in
| > ide_cancel_dma_sync().
|
| I prefer the 'check the s->blk in the beginning of ide_cancel_dma_sync' method.
| Some little different with your earlier patch.
|
| Anyway, let the maintainer do the choices.
|
@John ...wdyt?
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 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;