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;
--
2.26.2
+-- 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 >
© 2016 - 2024 Red Hat, Inc.