[v1] hw/ide: Cancel pending DMA requests before setting as inactive

[RFC PATCH-for-5.1] hw/ide: Cancel pending DMA requests before setting as inactive

Philippe Mathieu-Daudé posted 1 patch 3 years, 9 months ago

Diff against v2
Download mbox

Test checkpatch passed

Test docker-mingw@fedora passed

Test FreeBSD passed

Test docker-quick@centos7 failed

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200717074748.2528-1-f4bug@amsat.org

There is a newer version of this series

hw/ide/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Expand all Fold all

[RFC PATCH-for-5.1] hw/ide: Cancel pending DMA requests before setting as inactive

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

libFuzzer found a case where requests are queued for later in the
AIO context, but a command set the bus inactive, then when finally
the requests are processed by the DMA it aborts because it is
inactive:

 include/hw/ide/pci.h:59: IDEState *bmdma_active_if(BMDMAState *): Assertion `bmdma->bus->retry_unit != (uint8_t)-1' failed.

Reproducer available on the BugLink.

Fix by draining the pending DMA requests before inactivating the bus.

BugLink: https://bugs.launchpad.net/qemu/+bug/1887303
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because I don't have much clue about block drive and IDE,
so block-team please be very careful while reviewing this bug.
---
 hw/ide/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d997a78e47..b21d28f99c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -804,11 +804,11 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 
 void ide_set_inactive(IDEState *s, bool more)
 {
-    s->bus->dma->aiocb = NULL;
-    ide_clear_retry(s);
     if (s->bus->dma->ops->set_inactive) {
         s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
+    ide_cancel_dma_sync(s);
+    ide_clear_retry(s);
     ide_cmd_done(s);
 }
 
-- 
2.21.3

Re: [RFC PATCH-for-5.1] hw/ide: Cancel pending DMA requests before setting as inactive

Posted by Philippe Mathieu-Daudé 3 years, 9 months ago

On 7/17/20 9:47 AM, Philippe Mathieu-Daudé wrote:
> libFuzzer found a case where requests are queued for later in the
> AIO context, but a command set the bus inactive, then when finally
> the requests are processed by the DMA it aborts because it is
> inactive:
> 
>  include/hw/ide/pci.h:59: IDEState *bmdma_active_if(BMDMAState *): Assertion `bmdma->bus->retry_unit != (uint8_t)-1' failed.
> 
> Reproducer available on the BugLink.
> 
> Fix by draining the pending DMA requests before inactivating the bus.
> 
> BugLink: https://bugs.launchpad.net/qemu/+bug/1887303
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because I don't have much clue about block drive and IDE,
> so block-team please be very careful while reviewing this bug.
> ---
>  hw/ide/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..b21d28f99c 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -804,11 +804,11 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
>  
>  void ide_set_inactive(IDEState *s, bool more)
>  {
> -    s->bus->dma->aiocb = NULL;
> -    ide_clear_retry(s);
>      if (s->bus->dma->ops->set_inactive) {
>          s->bus->dma->ops->set_inactive(s->bus->dma, more);
>      }
> +    ide_cancel_dma_sync(s);
> +    ide_clear_retry(s);

Oops this is not the one I wanted to send. I'll send v2 shortly =)

>      ide_cmd_done(s);
>  }
>  
>