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

On 30/7/24 11:21, Philippe Mathieu-Daudé wrote:
> 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 = {};

'dscr' is only zero-initialized here, ...

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

... then filled in this loop being reused without clearing ...

> -        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) {

... checking error condition here.

By reducing its scope, it becomes zero-initialized each time and we can
safely check the ATTR_VALID bit, with no need for duplicated information
with MEMTX, thus reducing this patch size.

>               /* Indicate that error occurred in ST_FDS state */
>               s->admaerr &= ~SDHC_ADMAERR_STATE_MASK;
>               s->admaerr |= SDHC_ADMAERR_STATE_ST_FDS;

ST_FDS is indeed the correct error to report, since s->admasysaddr
isn't increased and points to the faulty address.