1. Patchew
  2. QEMU
  3. View series

[PATCH v2] hw/ide: check null block before _cancel_dma_sync

P J P posted 1 patch 5 years, 5 months ago
Test docker-quick@centos7 failed
Test docker-mingw@fedora failed
Test checkpatch failed
Test FreeBSD failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200903183138.2161977-1-ppandit@redhat.com
Maintainers: John Snow <jsnow@redhat.com>
hw/ide/core.c | 1 +
hw/ide/pci.c  | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
[PATCH v2] hw/ide: check null block before _cancel_dma_sync
Posted by P J P 5 years, 5 months ago 
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
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
Posted by P J P 5 years, 4 months ago 
+-- 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
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
Posted by Li Qiang 5 years, 4 months ago 
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
>
>
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
Posted by P J P 5 years, 4 months ago 
+-- 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
Re: [PATCH v2] hw/ide: check null block before _cancel_dma_sync
Posted by Li Qiang 5 years, 4 months ago 
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
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.