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

[RFC PATCH-for-5.1 v2] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [RFC PATCH-for-5.1 v2] 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:53 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.
> 

Fixes: 8ccad811e6 ("use AIO for DMA transfers - enabled DMA for CDROMs")

> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..f7affafb0c 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -804,7 +804,7 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
>  
>  void ide_set_inactive(IDEState *s, bool more)
>  {
> -    s->bus->dma->aiocb = NULL;
> +    ide_cancel_dma_sync(s);
>      ide_clear_retry(s);
>      if (s->bus->dma->ops->set_inactive) {
>          s->bus->dma->ops->set_inactive(s->bus->dma, more);
>

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

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

On 7/17/20 12:08 PM, Philippe Mathieu-Daudé wrote:
> On 7/17/20 9:53 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.
>>
> 

Sorry I also forgot:

Reported-by: Alexander Bulekov <alxndr@bu.edu>

> Fixes: 8ccad811e6 ("use AIO for DMA transfers - enabled DMA for CDROMs")
> 
>> 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 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index d997a78e47..f7affafb0c 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -804,7 +804,7 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
>>  
>>  void ide_set_inactive(IDEState *s, bool more)
>>  {
>> -    s->bus->dma->aiocb = NULL;
>> +    ide_cancel_dma_sync(s);
>>      ide_clear_retry(s);
>>      if (s->bus->dma->ops->set_inactive) {
>>          s->bus->dma->ops->set_inactive(s->bus->dma, more);
>>
>

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

Posted by John Snow 3 years, 9 months ago

On 7/17/20 3:53 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index d997a78e47..f7affafb0c 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -804,7 +804,7 @@ void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
>   
>   void ide_set_inactive(IDEState *s, bool more)
>   {

Generally, ide_set_inactive is meant to be used as the normative 
function to transition to the idle state; not something that performs a 
cancellation.

(It should probably assert that there are no pending BHs.)

...Let's run through the reproducer!
In my annotation here,

0x1F0 - Primary Bus I/O
0x3F6 - Primary Bus Control
   [0] Primary Bus, dev0
   [1] Primary Bus, dev1
0x170 - Secondary Bus I/O
0x376 - Secondary Bus Control
   [2] Secondary Bus, dev0
   [3] Secondary Bus, dev1


 > outw 0x176 0x3538

   [2].select = 0x38 [0011 1000]
                         ^ select secondary device
   [3].command = 0x35
                   ^ WRITE DMA EXT

outw 0x376 0x6007

   [3].control = 0x07 [0000 0111]
                             ^- +SRST
   # 0x06 goes into the void?

outw 0x376 0x6b6b

   [3].control = 0x6b; [0110 1011]
                              ^- -SRST
   # Oops, this does a Software Reset without cancelling the DMA again.
   # second write goes into the void?

outw 0x176 0x985c

   [3].select = 0x5c; [0101 1100]
   [3].command = 0x98; CHECK POWER MODE
              (Note: Deprecated in ATA4!)
   # Oops, this command shouldn't start when another one is in-process.
   # It also has the bad effect of setting the nsector register to 0xff!

outl 0xcf8 0x80000903
outl 0xcfc 0x2f2931
outl 0xcf8 0x80000920
outb 0xcfc 0x6b
		^- PCI stuff. I'm not as fast at reading hex here.
                    My brain has adapted to ATA only.

outb 0x68 0x7
   # Not entirely sure where this goes, but it seems to kick the DMA BH.
   # ... The pending DMA BH that belongs to WRITE_DMA_EXT.

outw 0x176 0x2530

     [3].select = 0x30 [0011 0000]
                           ^ select drive1
     [3].command = 0x25 (READ DMA EXT)

... At this point, it explodes because there's a pending DMA already, 
and the sector registers are all wrong.

This bug actually seems to have the same root cause as the other one: 
SRST does not perform the SRST sufficiently.


> -    s->bus->dma->aiocb = NULL;
> +    ide_cancel_dma_sync(s);
>       ide_clear_retry(s);
>       if (s->bus->dma->ops->set_inactive) {
>           s->bus->dma->ops->set_inactive(s->bus->dma, more);
>