[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
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);