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

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

[P J P]

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

[P J P]

+-- 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

[Li Qiang]

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

[P J P]

+-- 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

[Li Qiang]

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
>

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

[P J P]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

  Hello Li,

+-- On Fri, 18 Sep 2020, Li Qiang wrote --+
| P J P <ppandit@redhat.com> 

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

[Li Qiang]

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

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

[P J P]

[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]

+-- 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