[Qemu-devel] [PATCH 5/6] ide: Correct handling of malformed/short PRDTs

[John Snow]

This impacts both BMDMA and AHCI HBA interfaces for IDE.
Currently, we confuse the difference between a PRD having
"0 bytes" and a PRD having "0 complete sectors."

This leads to, in the BMDMA case, leaked memory for short PRDTs,
and infinite loops in the AHCI case.

the "prepare_buf" callback is reworked to return 0 if it could
not allocate a full sector's worth of buffer space, instead of
returning non-zero if it allocated any number of bytes.

ide_dma_cb adds a call to commit_buf in order to delete
the short PRDT that it will not attempt to use to finish
the DMA operation.

This patch corrects both occurrences and adds an assertion to
prevent future regression. This assertion is tested in the
existing ide-test, and is covered in a forthcoming AHCI test.

Signed-off-by: John Snow <js...@redhat.com>
---
 dma-helpers.c | 3 +++
 hw/ide/ahci.c | 2 +-
 hw/ide/core.c | 1 +
 hw/ide/pci.c  | 5 +++--
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 7f86e18..f51d6ee 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -38,6 +38,9 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t 
c, dma_addr_t len)
 void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
                       AddressSpace *as)
 {
+    /* If this is true, you're leaking memory. */
+    assert(qsg->sg == NULL);
+
     qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
     qsg->nsg = 0;
     qsg->nalloc = alloc_hint;
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 16cd248..67c1e36 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1147,7 +1147,7 @@ static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
     s->io_buffer_size = s->sg.size;
 
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
-    return s->io_buffer_size != 0;
+    return s->io_buffer_size / 512 != 0;
 }
 
 /**
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7c1929e..82d01e8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -732,6 +732,7 @@ void ide_dma_cb(void *opaque, int ret)
         /* The PRDs were too short. Reset the Active bit, but don't raise an
          * interrupt. */
         s->status = READY_STAT | SEEK_STAT;
+        dma_buf_commit(s, false);
         goto eot;
     }
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 2397f35..3f643c2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -74,8 +74,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
         if (bm->cur_prd_len == 0) {
             /* end of table (with a fail safe of one page) */
             if (bm->cur_prd_last ||
-                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
-                return s->io_buffer_size != 0;
+                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
+                return (s->io_buffer_size / 512) != 0;
+            }
             pci_dma_read(pci_dev, bm->cur_addr, &prd, 8);
             bm->cur_addr += 8;
             prd.addr = le32_to_cpu(prd.addr);
-- 
1.9.3