[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: John Snow
Subject: Re: [RFC PATCH-for-5.1 v2] hw/ide: Cancel pending DMA requests before setting as inactive
Date: Wed, 22 Jul 2020 00:19:25 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0

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

  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);
      if (s->bus->dma->ops->set_inactive) {
          s->bus->dma->ops->set_inactive(s->bus->dma, more);

reply via email to

[Prev in Thread] Current Thread [Next in Thread]