Re: [RFC PATCH-for-5.1 v2] hw/ide: Cancel pending DMA requests before setting as inactive
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é
---
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 [ 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 0x8903
outl 0xcfc 0x2f2931
outl 0xcf8 0x8920
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 ]
^ 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);
Re: [RFC PATCH-for-5.1 v2] hw/ide: Cancel pending DMA requests before setting as inactive
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
> 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é
>> ---
>> 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
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é
> ---
> 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);
>
[RFC PATCH-for-5.1 v2] hw/ide: Cancel pending DMA requests before setting as inactive
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é
---
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
