[PATCH-for-9.1 5/5] hw/sd/sdhci: Check ADMA descriptors can be accessed

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Since malicious guest can write invalid addresses to
the ADMASYSADDR register, we need to check whether the
descriptor could be correctly filled or not.

Cc: qemu-stable@nongnu.org
Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c      | 86 ++++++++++++++++++++++++++--------------------
 hw/sd/trace-events |  2 +-
 2 files changed, 49 insertions(+), 39 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 66b9364e9e..eb0476b9aa 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -698,53 +698,62 @@ static void trace_adma_description(const char *type, const ADMADescr *dscr)
     trace_sdhci_adma_desc(type, dscr->addr, dscr->length, dscr->attr, dscr->incr);
 }
 
-static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
+static MemTxResult get_adma_description(SDHCIState *s, ADMADescr *dscr)
 {
     uint32_t adma1 = 0;
     uint64_t adma2 = 0;
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
+    MemTxResult res;
+
     switch (SDHC_DMA_TYPE(s->hostctl1)) {
     case SDHC_CTRL_ADMA2_32:
-        dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
-                        MEMTXATTRS_UNSPECIFIED);
-        adma2 = le64_to_cpu(adma2);
-        /* The spec does not specify endianness of descriptor table.
-         * We currently assume that it is LE.
-         */
-        dscr->addr = (hwaddr)extract64(adma2, 32, 32) & ~0x3ull;
-        dscr->length = (uint16_t)extract64(adma2, 16, 16);
-        dscr->attr = (uint8_t)extract64(adma2, 0, 7);
-        dscr->incr = 8;
-        trace_adma_description("ADMA2_32", dscr);
+        res = dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
+                              MEMTXATTRS_UNSPECIFIED);
+        if (res == MEMTX_OK) {
+            adma2 = le64_to_cpu(adma2);
+            /* The spec does not specify endianness of descriptor table.
+             * We currently assume that it is LE.
+             */
+            dscr->addr = (hwaddr)extract64(adma2, 32, 32) & ~0x3ull;
+            dscr->length = (uint16_t)extract64(adma2, 16, 16);
+            dscr->attr = (uint8_t)extract64(adma2, 0, 7);
+            dscr->incr = 8;
+            trace_adma_description("ADMA2_32", dscr);
+        }
         break;
     case SDHC_CTRL_ADMA1_32:
-        dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
-                        MEMTXATTRS_UNSPECIFIED);
-        adma1 = le32_to_cpu(adma1);
-        dscr->addr = (hwaddr)(adma1 & 0xFFFFF000);
-        dscr->attr = (uint8_t)extract32(adma1, 0, 7);
-        dscr->incr = 4;
-        if ((dscr->attr & SDHC_ADMA_ATTR_ACT_MASK) == SDHC_ADMA_ATTR_SET_LEN) {
-            dscr->length = (uint16_t)extract32(adma1, 12, 16);
-        } else {
-            dscr->length = 4 * KiB;
+        res = dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
+                              MEMTXATTRS_UNSPECIFIED);
+        if (res == MEMTX_OK) {
+            adma1 = le32_to_cpu(adma1);
+            dscr->addr = (hwaddr)(adma1 & ~0xfff);
+            dscr->attr = (uint8_t)extract32(adma1, 0, 7);
+            dscr->incr = 4;
+            if ((dscr->attr & SDHC_ADMA_ATTR_ACT_MASK) == SDHC_ADMA_ATTR_SET_LEN) {
+                dscr->length = (uint16_t)extract32(adma1, 12, 16);
+            } else {
+                dscr->length = 4 * KiB;
+            }
+            trace_adma_description("ADMA1_32", dscr);
         }
-        trace_adma_description("ADMA1_32", dscr);
         break;
     case SDHC_CTRL_ADMA2_64:
-        dma_memory_read(s->dma_as, entry_addr, &dscr->attr, 1,
-                        MEMTXATTRS_UNSPECIFIED);
-        dma_memory_read(s->dma_as, entry_addr + 2, &dscr->length, 2,
-                        MEMTXATTRS_UNSPECIFIED);
-        dscr->length = le16_to_cpu(dscr->length);
-        dma_memory_read(s->dma_as, entry_addr + 4, &dscr->addr, 8,
-                        MEMTXATTRS_UNSPECIFIED);
-        dscr->addr = le64_to_cpu(dscr->addr);
-        dscr->attr &= (uint8_t) ~0xC0;
-        dscr->incr = 12;
-        trace_adma_description("ADMA2_64", dscr);
+        res = dma_memory_read(s->dma_as, entry_addr, &dscr->attr, 1,
+                              MEMTXATTRS_UNSPECIFIED);
+        res |= dma_memory_read(s->dma_as, entry_addr + 2, &dscr->length, 2,
+                               MEMTXATTRS_UNSPECIFIED);
+        res |= dma_memory_read(s->dma_as, entry_addr + 4, &dscr->addr, 8,
+                               MEMTXATTRS_UNSPECIFIED);
+        if (res == MEMTX_OK) {
+            dscr->length = le16_to_cpu(dscr->length);
+            dscr->addr = le64_to_cpu(dscr->addr);
+            dscr->attr &= (uint8_t) ~0xc0;
+            dscr->incr = 12;
+            trace_adma_description("ADMA2_64", dscr);
+        }
         break;
     }
+    return res;
 }
 
 /* Advanced DMA data transfer */
@@ -755,7 +764,6 @@ static void sdhci_do_adma(SDHCIState *s)
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
-    MemTxResult res;
     int i;
 
     if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
@@ -765,12 +773,14 @@ static void sdhci_do_adma(SDHCIState *s)
     }
 
     for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
+        MemTxResult res;
+
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
-        get_adma_description(s, &dscr);
-        trace_sdhci_adma_loop(dscr.addr, dscr.length, dscr.attr);
+        res = get_adma_description(s, &dscr);
+        trace_sdhci_adma_loop(dscr.addr, dscr.length, dscr.attr, res);
 
-        if ((dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
+        if (res != MEMTX_OK || (dscr.attr & SDHC_ADMA_ATTR_VALID) == 0) {
             /* Indicate that error occurred in ST_FDS state */
             s->admaerr &= ~SDHC_ADMAERR_STATE_MASK;
             s->admaerr |= SDHC_ADMAERR_STATE_ST_FDS;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 3d3f5c1cb7..a802a717b9 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -29,7 +29,7 @@ sdhci_response4(uint32_t r0) "RSPREG[31..0]=0x%08x"
 sdhci_response16(uint32_t r3, uint32_t r2, uint32_t r1, uint32_t r0) "RSPREG[127..96]=0x%08x, RSPREG[95..64]=0x%08x, RSPREG[63..32]=0x%08x, RSPREG[31..0]=0x%08x"
 sdhci_end_transfer(uint8_t cmd, uint32_t arg) "Automatically issue CMD%02u 0x%08x"
 sdhci_adma(const char *desc, uint32_t sysad) "%s: admasysaddr=0x%" PRIx32
-sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr) "addr=0x%08" PRIx64 ", len=%d, attr=0x%x"
+sdhci_adma_loop(uint64_t addr, uint16_t length, uint8_t attr, uint32_t res) "addr=0x%08" PRIx64 ", len=%d, attr=0x%x, res=%" PRIu32
 sdhci_adma_transfer_completed(void) ""
 sdhci_access(const char *access, unsigned int size, uint64_t offset, const char *dir, uint64_t val, uint64_t val2) "%s%u: addr[0x%04" PRIx64 "] %s 0x%08" PRIx64 " (%" PRIu64 ")"
 sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
-- 
2.45.2