9ae3a8
From 6a5797ecb4e557c746356a2f8636cceafa787851 Mon Sep 17 00:00:00 2001
9ae3a8
From: John Snow <jsnow@redhat.com>
9ae3a8
Date: Fri, 26 Jun 2015 21:52:47 +0200
9ae3a8
Subject: [PATCH 2/2] ide: Correct handling of malformed/short PRDTs
9ae3a8
9ae3a8
Message-id: <1435355567-29641-3-git-send-email-jsnow@redhat.com>
9ae3a8
Patchwork-id: 66536
9ae3a8
O-Subject: [RHEL-7.2 qemu-kvm PATCH 2/2] ide: Correct handling of malformed/short PRDTs
9ae3a8
Bugzilla: 1205100
9ae3a8
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
9ae3a8
RH-Acked-by: Juan Quintela <quintela@redhat.com>
9ae3a8
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
9ae3a8
This impacts both BMDMA and AHCI HBA interfaces for IDE.
9ae3a8
Currently, we confuse the difference between a PRDT having
9ae3a8
"0 bytes" and a PRDT having "0 complete sectors."
9ae3a8
9ae3a8
When we receive an incomplete sector, inconsistent error checking
9ae3a8
leads to an infinite loop wherein the call succeeds, but it
9ae3a8
didn't give us enough bytes -- leading us to re-call the
9ae3a8
DMA chain over and over again. This leads to, in the BMDMA case,
9ae3a8
leaked memory for short PRDTs, and infinite loops and resource
9ae3a8
usage in the AHCI case.
9ae3a8
9ae3a8
The .prepare_buf() callback is reworked to return the number of
9ae3a8
bytes that it successfully prepared. 0 is a valid, non-error
9ae3a8
answer that means the table was empty and described no bytes.
9ae3a8
-1 indicates an error.
9ae3a8
9ae3a8
Our current implementation uses the io_buffer in IDEState to
9ae3a8
ultimately describe the size of a prepared scatter-gather list.
9ae3a8
Even though the AHCI PRDT/SGList can be as large as 256GiB, the
9ae3a8
AHCI command header limits transactions to just 4GiB. ATA8-ACS3,
9ae3a8
however, defines the largest transaction to be an LBA48 command
9ae3a8
that transfers 65,536 sectors. With a 512 byte sector size, this
9ae3a8
is just 32MiB.
9ae3a8
9ae3a8
Since our current state structures use the int type to describe
9ae3a8
the size of the buffer, and this state is migrated as int32, we
9ae3a8
are limited to describing 2GiB buffer sizes unless we change the
9ae3a8
migration protocol.
9ae3a8
9ae3a8
For this reason, this patch begins to unify the assertions in the
9ae3a8
IDE pathways that the scatter-gather list provided by either the
9ae3a8
AHCI PRDT or the PCI BMDMA PRDs can only describe, at a maximum,
9ae3a8
2GiB. This should be resilient enough unless we need a sector
9ae3a8
size that exceeds 32KiB.
9ae3a8
9ae3a8
Further, the likelihood of any guest operating system actually
9ae3a8
attempting to transfer this much data in a single operation is
9ae3a8
very slim.
9ae3a8
9ae3a8
To this end, the IDEState variables have been updated to more
9ae3a8
explicitly clarify our maximum supported size. Callers to the
9ae3a8
prepare_buf callback have been reworked to understand the new
9ae3a8
return code, and all versions of the prepare_buf callback have
9ae3a8
been adjusted accordingly.
9ae3a8
9ae3a8
Lastly, the ahci_populate_sglist helper, relied upon by the
9ae3a8
AHCI implementation of .prepare_buf() as well as the PCI
9ae3a8
implementation of the callback have had overflow assertions
9ae3a8
added to help make clear the reasonings behind the various
9ae3a8
type changes.
9ae3a8
9ae3a8
[Added %d -> %"PRId64" fix John sent because off_pos changed from int to
9ae3a8
int64_t.
9ae3a8
--Stefan]
9ae3a8
9ae3a8
Signed-off-by: John Snow <jsnow@redhat.com>
9ae3a8
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
Message-id: 1414785819-26209-4-git-send-email-jsnow@redhat.com
9ae3a8
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
(cherry picked from commit 3251bdcf1c67427d964517053c3d185b46e618e8)
9ae3a8
Signed-off-by: John Snow <jsnow@redhat.com>
9ae3a8
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
9ae3a8
Conflicts:
9ae3a8
        hw/ide/ahci.c:     Conflicts arising from not backporting bef1301ac
9ae3a8
                           ahci: unify sglist preparation
9ae3a8
        hw/ide/core.c:     Conflicts from not backporting IDEDMAOps changes,
9ae3a8
                           including 9898586d8 and dependencies.
9ae3a8
        hw/ide/internal.h: Same as hw/ide/core.c.
9ae3a8
        hw/ide/macio.c:    Conflicts from not backporting (many) IDEDMAOps
9ae3a8
                           refactorings, 4aa3510f6 and others.
9ae3a8
        hw/ide/pci.c:      Conflicts arising form not backporting f6c11d564
9ae3a8
                           ide: Introduce abstract QOM type for PCIIDEState
9ae3a8
9ae3a8
Signed-off-by: John Snow <jsnow@redhat.com>
9ae3a8
---
9ae3a8
 hw/ide/ahci.c     | 33 ++++++++++++++++++++++++++-------
9ae3a8
 hw/ide/core.c     | 10 ++++++++--
9ae3a8
 hw/ide/internal.h | 13 +++++++------
9ae3a8
 hw/ide/pci.c      | 26 +++++++++++++++++++++-----
9ae3a8
 4 files changed, 62 insertions(+), 20 deletions(-)
9ae3a8
9ae3a8
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
9ae3a8
index 7f3927a..e951ba0 100644
9ae3a8
--- a/hw/ide/ahci.c
9ae3a8
+++ b/hw/ide/ahci.c
9ae3a8
@@ -640,7 +640,8 @@ static int prdt_tbl_entry_size(const AHCI_SG *tbl)
9ae3a8
     return (le32_to_cpu(tbl->flags_size) & AHCI_PRDT_SIZE_MASK) + 1;
9ae3a8
 }
9ae3a8
 
9ae3a8
-static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
9ae3a8
+static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist,
9ae3a8
+                                int32_t offset)
9ae3a8
 {
9ae3a8
     AHCICmdHdr *cmd = ad->cur_cmd;
9ae3a8
     uint32_t opts = le32_to_cpu(cmd->opts);
9ae3a8
@@ -651,11 +652,19 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
9ae3a8
     uint8_t *prdt;
9ae3a8
     int i;
9ae3a8
     int r = 0;
9ae3a8
-    int sum = 0;
9ae3a8
+    uint64_t sum = 0;
9ae3a8
     int off_idx = -1;
9ae3a8
-    int off_pos = -1;
9ae3a8
+    int64_t off_pos = -1;
9ae3a8
     int tbl_entry_size;
9ae3a8
 
9ae3a8
+    /*
9ae3a8
+     * Note: AHCI PRDT can describe up to 256GiB. SATA/ATA only support
9ae3a8
+     * transactions of up to 32MiB as of ATA8-ACS3 rev 1b, assuming a
9ae3a8
+     * 512 byte sector size. We limit the PRDT in this implementation to
9ae3a8
+     * a reasonably large 2GiB, which can accommodate the maximum transfer
9ae3a8
+     * request for sector sizes up to 32K.
9ae3a8
+     */
9ae3a8
+
9ae3a8
     if (!sglist_alloc_hint) {
9ae3a8
         DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);
9ae3a8
         return -1;
9ae3a8
@@ -690,7 +699,7 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
9ae3a8
         }
9ae3a8
         if ((off_idx == -1) || (off_pos < 0) || (off_pos > tbl_entry_size)) {
9ae3a8
             DPRINTF(ad->port_no, "%s: Incorrect offset! "
9ae3a8
-                            "off_idx: %d, off_pos: %d\n",
9ae3a8
+                            "off_idx: %d, off_pos: %"PRId64"\n",
9ae3a8
                             __func__, off_idx, off_pos);
9ae3a8
             r = -1;
9ae3a8
             goto out;
9ae3a8
@@ -704,6 +713,13 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
9ae3a8
             /* flags_size is zero-based */
9ae3a8
             qemu_sglist_add(sglist, le64_to_cpu(tbl[i].addr),
9ae3a8
                             prdt_tbl_entry_size(&tbl[i]));
9ae3a8
+            if (sglist->size > INT32_MAX) {
9ae3a8
+                error_report("AHCI Physical Region Descriptor Table describes "
9ae3a8
+                             "more than 2 GiB.\n");
9ae3a8
+                qemu_sglist_destroy(sglist);
9ae3a8
+                r = -1;
9ae3a8
+                goto out;
9ae3a8
+            }
9ae3a8
         }
9ae3a8
     }
9ae3a8
 
9ae3a8
@@ -1047,16 +1063,19 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
9ae3a8
     dma_cb(s, 0);
9ae3a8
 }
9ae3a8
 
9ae3a8
-static int ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
+static int32_t ahci_dma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
 {
9ae3a8
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
9ae3a8
     IDEState *s = &ad->port.ifs[0];
9ae3a8
 
9ae3a8
-    ahci_populate_sglist(ad, &s->sg, 0);
9ae3a8
+    if (ahci_populate_sglist(ad, &s->sg, 0) == -1) {
9ae3a8
+        DPRINTF(ad->port_no, "ahci_dma_prepare_buf failed.\n");
9ae3a8
+        return -1;
9ae3a8
+    }
9ae3a8
     s->io_buffer_size = s->sg.size;
9ae3a8
 
9ae3a8
     DPRINTF(ad->port_no, "len=%#x\n", s->io_buffer_size);
9ae3a8
-    return s->io_buffer_size != 0;
9ae3a8
+    return s->io_buffer_size;
9ae3a8
 }
9ae3a8
 
9ae3a8
 static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
9ae3a8
diff --git a/hw/ide/core.c b/hw/ide/core.c
9ae3a8
index 9a22425..d9fdb03 100644
9ae3a8
--- a/hw/ide/core.c
9ae3a8
+++ b/hw/ide/core.c
9ae3a8
@@ -690,10 +690,11 @@ void ide_dma_cb(void *opaque, int ret)
9ae3a8
     n = s->nsector;
9ae3a8
     s->io_buffer_index = 0;
9ae3a8
     s->io_buffer_size = n * 512;
9ae3a8
-    if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) == 0) {
9ae3a8
+    if (s->bus->dma->ops->prepare_buf(s->bus->dma, ide_cmd_is_read(s)) < 512) {
9ae3a8
         /* The PRDs were too short. Reset the Active bit, but don't raise an
9ae3a8
          * interrupt. */
9ae3a8
         s->status = READY_STAT | SEEK_STAT;
9ae3a8
+        dma_buf_commit(s);
9ae3a8
         goto eot;
9ae3a8
     }
9ae3a8
 
9ae3a8
@@ -2131,6 +2132,11 @@ static int ide_nop_int(IDEDMA *dma, int x)
9ae3a8
     return 0;
9ae3a8
 }
9ae3a8
 
9ae3a8
+static int32_t ide_nop_int32(IDEDMA *dma, int x)
9ae3a8
+{
9ae3a8
+    return 0;
9ae3a8
+}
9ae3a8
+
9ae3a8
 static void ide_nop_restart(void *opaque, int x, RunState y)
9ae3a8
 {
9ae3a8
 }
9ae3a8
@@ -2138,7 +2144,7 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
9ae3a8
 static const IDEDMAOps ide_dma_nop_ops = {
9ae3a8
     .start_dma      = ide_nop_start,
9ae3a8
     .start_transfer = ide_nop,
9ae3a8
-    .prepare_buf    = ide_nop_int,
9ae3a8
+    .prepare_buf    = ide_nop_int32,
9ae3a8
     .rw_buf         = ide_nop_int,
9ae3a8
     .set_unit       = ide_nop_int,
9ae3a8
     .add_status     = ide_nop_int,
9ae3a8
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
9ae3a8
index 0a2d6bc..f8fb564 100644
9ae3a8
--- a/hw/ide/internal.h
9ae3a8
+++ b/hw/ide/internal.h
9ae3a8
@@ -323,6 +323,7 @@ typedef void EndTransferFunc(IDEState *);
9ae3a8
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
9ae3a8
 typedef int DMAFunc(IDEDMA *);
9ae3a8
 typedef int DMAIntFunc(IDEDMA *, int);
9ae3a8
+typedef int32_t DMAInt32Func(IDEDMA *, int);
9ae3a8
 typedef void DMARestartFunc(void *, int, RunState);
9ae3a8
 
9ae3a8
 struct unreported_events {
9ae3a8
@@ -384,7 +385,7 @@ struct IDEState {
9ae3a8
     uint8_t cdrom_changed;
9ae3a8
     int packet_transfer_size;
9ae3a8
     int elementary_transfer_size;
9ae3a8
-    int io_buffer_index;
9ae3a8
+    int32_t io_buffer_index;
9ae3a8
     int lba;
9ae3a8
     int cd_sector_size;
9ae3a8
     int atapi_dma; /* true if dma is requested for the packet cmd */
9ae3a8
@@ -393,8 +394,8 @@ struct IDEState {
9ae3a8
     struct iovec iov;
9ae3a8
     QEMUIOVector qiov;
9ae3a8
     /* ATA DMA state */
9ae3a8
-    int io_buffer_offset;
9ae3a8
-    int io_buffer_size;
9ae3a8
+    int32_t io_buffer_offset;
9ae3a8
+    int32_t io_buffer_size;
9ae3a8
     QEMUSGList sg;
9ae3a8
     /* PIO transfer handling */
9ae3a8
     int req_nb_sectors; /* number of sectors per interrupt */
9ae3a8
@@ -404,8 +405,8 @@ struct IDEState {
9ae3a8
     uint8_t *io_buffer;
9ae3a8
     /* PIO save/restore */
9ae3a8
     int32_t io_buffer_total_len;
9ae3a8
-    int cur_io_buffer_offset;
9ae3a8
-    int cur_io_buffer_len;
9ae3a8
+    int32_t cur_io_buffer_offset;
9ae3a8
+    int32_t cur_io_buffer_len;
9ae3a8
     uint8_t end_transfer_fn_idx;
9ae3a8
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
9ae3a8
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
9ae3a8
@@ -429,7 +430,7 @@ struct IDEState {
9ae3a8
 struct IDEDMAOps {
9ae3a8
     DMAStartFunc *start_dma;
9ae3a8
     DMAFunc *start_transfer;
9ae3a8
-    DMAIntFunc *prepare_buf;
9ae3a8
+    DMAInt32Func *prepare_buf;
9ae3a8
     DMAIntFunc *rw_buf;
9ae3a8
     DMAIntFunc *set_unit;
9ae3a8
     DMAIntFunc *add_status;
9ae3a8
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
9ae3a8
index cf7acb0..03b2081 100644
9ae3a8
--- a/hw/ide/pci.c
9ae3a8
+++ b/hw/ide/pci.c
9ae3a8
@@ -28,6 +28,7 @@
9ae3a8
 #include <hw isa="" isa.h="">
9ae3a8
 #include "block/block.h"
9ae3a8
 #include "sysemu/dma.h"
9ae3a8
+#include "qemu/error-report.h"
9ae3a8
 
9ae3a8
 #include <hw ide="" pci.h="">
9ae3a8
 
9ae3a8
@@ -51,8 +52,11 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
9ae3a8
     }
9ae3a8
 }
9ae3a8
 
9ae3a8
-/* return 0 if buffer completed */
9ae3a8
-static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
+/**
9ae3a8
+ * Return the number of bytes successfully prepared.
9ae3a8
+ * -1 on error.
9ae3a8
+ */
9ae3a8
+static int32_t bmdma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
 {
9ae3a8
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
9ae3a8
     IDEState *s = bmdma_active_if(bm);
9ae3a8
@@ -69,8 +73,9 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
         if (bm->cur_prd_len == 0) {
9ae3a8
             /* end of table (with a fail safe of one page) */
9ae3a8
             if (bm->cur_prd_last ||
9ae3a8
-                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE)
9ae3a8
-                return s->io_buffer_size != 0;
9ae3a8
+                (bm->cur_addr - bm->addr) >= BMDMA_PAGE_SIZE) {
9ae3a8
+                return s->io_buffer_size;
9ae3a8
+            }
9ae3a8
             pci_dma_read(&bm->pci_dev->dev, bm->cur_addr, &prd, 8);
9ae3a8
             bm->cur_addr += 8;
9ae3a8
             prd.addr = le32_to_cpu(prd.addr);
9ae3a8
@@ -85,12 +90,23 @@ static int bmdma_prepare_buf(IDEDMA *dma, int is_write)
9ae3a8
         l = bm->cur_prd_len;
9ae3a8
         if (l > 0) {
9ae3a8
             qemu_sglist_add(&s->sg, bm->cur_prd_addr, l);
9ae3a8
+
9ae3a8
+            /* Note: We limit the max transfer to be 2GiB.
9ae3a8
+             * This should accommodate the largest ATA transaction
9ae3a8
+             * for LBA48 (65,536 sectors) and 32K sector sizes. */
9ae3a8
+            if (s->sg.size > INT32_MAX) {
9ae3a8
+                error_report("IDE: sglist describes more than 2GiB.\n");
9ae3a8
+                break;
9ae3a8
+            }
9ae3a8
             bm->cur_prd_addr += l;
9ae3a8
             bm->cur_prd_len -= l;
9ae3a8
             s->io_buffer_size += l;
9ae3a8
         }
9ae3a8
     }
9ae3a8
-    return 1;
9ae3a8
+
9ae3a8
+    qemu_sglist_destroy(&s->sg);
9ae3a8
+    s->io_buffer_size = 0;
9ae3a8
+    return -1;
9ae3a8
 }
9ae3a8
 
9ae3a8
 /* return 0 if buffer completed */
9ae3a8
-- 
9ae3a8
1.8.3.1
9ae3a8