[PATCH-for-9.1 0/5] hw/sd: SDcard & SDHCI fixes

[PATCH-for-9.1 0/5] hw/sd: SDcard & SDHCI fixes

[Philippe Mathieu-Daudé]

3 fixes (2 fuzzed).

Philippe Mathieu-Daudé (5):
  hw/sd/sdcard: Explicit dummy byte value
  hw/sd/sdcard: Do not abort when reading DAT lines on invalid cmd state
  hw/sd/sdhci: Reset @data_count index on invalid ADMA transfers
  hw/sd/sdhci: Trace ADMA descriptors
  hw/sd/sdhci: Check ADMA descriptors can be accessed

 hw/sd/sd.c         | 16 +++++----
 hw/sd/sdhci.c      | 89 ++++++++++++++++++++++++++++------------------
 hw/sd/trace-events |  3 +-
 3 files changed, 66 insertions(+), 42 deletions(-)

-- 
2.45.2

[PATCH-for-9.1 1/5] hw/sd/sdcard: Explicit dummy byte value

[Philippe Mathieu-Daudé]

On error the DAT lines are left unmodified to their
previous states. QEMU returns 0x00 for convenience.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 07cb97d88c..c02f04f1ea 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2478,20 +2478,22 @@ void sd_write_byte(SDState *sd, uint8_t value)
 uint8_t sd_read_byte(SDState *sd)
 {
     /* TODO: Append CRCs */
+    static const uint8_t dummy_byte = 0x00;
     uint8_t ret;
     uint32_t io_len;
 
     if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable)
-        return 0x00;
+        return dummy_byte;
 
     if (sd->state != sd_sendingdata_state) {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: not in Sending-Data state\n", __func__);
-        return 0x00;
+        return dummy_byte;
     }
 
-    if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
-        return 0x00;
+    if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION)) {
+        return dummy_byte;
+    }
 
     io_len = sd_blk_len(sd);
 
@@ -2517,7 +2519,7 @@ uint8_t sd_read_byte(SDState *sd)
         if (sd->data_offset == 0) {
             if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
                                   sd->data_start, io_len)) {
-                return 0x00;
+                return dummy_byte;
             }
             sd_blk_read(sd, sd->data_start, io_len);
         }
-- 
2.45.2

Re: [PATCH-for-9.1 1/5] hw/sd/sdcard: Explicit dummy byte value

[Richard Henderson]

On 7/30/24 19:21, Philippe Mathieu-Daudé wrote:
> On error the DAT lines are left unmodified to their
> previous states. QEMU returns 0x00 for convenience.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/sd/sd.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 07cb97d88c..c02f04f1ea 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2478,20 +2478,22 @@ void sd_write_byte(SDState *sd, uint8_t value)
>   uint8_t sd_read_byte(SDState *sd)
>   {
>       /* TODO: Append CRCs */
> +    static const uint8_t dummy_byte = 0x00;

A zero doesn't need to be static.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH-for-9.1 2/5] hw/sd/sdcard: Do not abort when reading DAT lines on invalid cmd state

[Philippe Mathieu-Daudé]

Guest should not try to read the DAT lines from invalid
command state. If it still insists to do so, return a
dummy value.

Cc: qemu-stable@nongnu.org
Fixes: e2dec2eab0 ("hw/sd/sdcard: Remove default case in read/write on DAT lines")
Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2454
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index c02f04f1ea..b1e6e36b44 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2540,7 +2540,9 @@ uint8_t sd_read_byte(SDState *sd)
         break;
 
     default:
-        g_assert_not_reached();
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: DAT read illegal for command %s\n",
+                                       __func__, sd->last_cmd_name);
+        return dummy_byte;
     }
 
     return ret;
-- 
2.45.2

Re: [PATCH-for-9.1 2/5] hw/sd/sdcard: Do not abort when reading DAT lines on invalid cmd state

[Richard Henderson]

On 7/30/24 19:21, Philippe Mathieu-Daudé wrote:
> Guest should not try to read the DAT lines from invalid
> command state. If it still insists to do so, return a
> dummy value.
> 
> Cc:qemu-stable@nongnu.org
> Fixes: e2dec2eab0 ("hw/sd/sdcard: Remove default case in read/write on DAT lines")
> Reported-by: Zheyu Ma<zheyuma97@gmail.com>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2454
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/sd/sd.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-9.1 3/5] hw/sd/sdhci: Reset @data_count index on invalid ADMA transfers

[Philippe Mathieu-Daudé]

We neglected to clear the @data_count index on ADMA error,
allowing to trigger assertion in sdhci_read_dataport() or
sdhci_write_dataport().

Cc: qemu-stable@nongnu.org
Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2455
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d02c3e3963..8293d83556 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -846,6 +846,7 @@ static void sdhci_do_adma(SDHCIState *s)
                 }
             }
             if (res != MEMTX_OK) {
+                s->data_count = 0;
                 if (s->errintstsen & SDHC_EISEN_ADMAERR) {
                     trace_sdhci_error("Set ADMA error flag");
                     s->errintsts |= SDHC_EIS_ADMAERR;
-- 
2.45.2

Re: [PATCH-for-9.1 3/5] hw/sd/sdhci: Reset @data_count index on invalid ADMA transfers

[Richard Henderson]

On 7/30/24 19:21, Philippe Mathieu-Daudé wrote:
> We neglected to clear the @data_count index on ADMA error,
> allowing to trigger assertion in sdhci_read_dataport() or
> sdhci_write_dataport().
> 
> Cc:qemu-stable@nongnu.org
> Fixes: d7dfca0807 ("hw/sdhci: introduce standard SD host controller")
> Reported-by: Zheyu Ma<zheyuma97@gmail.com>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2455
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/sd/sdhci.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH-for-9.1 4/5] hw/sd/sdhci: Trace ADMA descriptors

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/sd/sdhci.c      | 8 ++++++++
 hw/sd/trace-events | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8293d83556..66b9364e9e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -693,6 +693,11 @@ typedef struct ADMADescr {
     uint8_t incr;
 } ADMADescr;
 
+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)
 {
     uint32_t adma1 = 0;
@@ -710,6 +715,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         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),
@@ -723,6 +729,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         } else {
             dscr->length = 4 * KiB;
         }
+        trace_adma_description("ADMA1_32", dscr);
         break;
     case SDHC_CTRL_ADMA2_64:
         dma_memory_read(s->dma_as, entry_addr, &dscr->attr, 1,
@@ -735,6 +742,7 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dscr->addr = le64_to_cpu(dscr->addr);
         dscr->attr &= (uint8_t) ~0xC0;
         dscr->incr = 12;
+        trace_adma_description("ADMA2_64", dscr);
         break;
     }
 }
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 43671dc791..3d3f5c1cb7 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -35,6 +35,7 @@ sdhci_access(const char *access, unsigned int size, uint64_t offset, const char
 sdhci_read_dataport(uint16_t data_count) "all %u bytes of data have been read from input buffer"
 sdhci_write_dataport(uint16_t data_count) "write buffer filled with %u bytes of data"
 sdhci_capareg(const char *desc, uint16_t val) "%s: %u"
+sdhci_adma_desc(const char *type, uint64_t addr, uint16_t length, uint8_t attr, uint8_t incr) "%s addr:0x%08"PRIx64" len:0x%x, attr:0x%x, incr:%u"
 
 # sd.c
 sdcard_normal_command(const char *proto, const char *cmd_desc, uint8_t cmd, uint32_t arg, const char *state) "%s %20s/ CMD%02d arg 0x%08x (state %s)"
-- 
2.45.2

Re: [PATCH-for-9.1 4/5] hw/sd/sdhci: Trace ADMA descriptors

[Richard Henderson]

On 7/30/24 19:21, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org>
> ---
>   hw/sd/sdhci.c      | 8 ++++++++
>   hw/sd/trace-events | 1 +
>   2 files changed, 9 insertions(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8293d83556..66b9364e9e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -693,6 +693,11 @@ typedef struct ADMADescr {
>       uint8_t incr;
>   } ADMADescr;
>   
> +static void trace_adma_description(const char *type, const ADMADescr *dscr)
> +{
> +    trace_sdhci_adma_desc(type, dscr->addr, dscr->length, dscr->attr, dscr->incr);
> +}
> +

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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.

Re: [PATCH-for-9.1 0/5] hw/sd: SDcard & SDHCI fixes

[Philippe Mathieu-Daudé]

On 30/7/24 11:21, Philippe Mathieu-Daudé wrote:
> 3 fixes (2 fuzzed).
> 
> Philippe Mathieu-Daudé (5):
>    hw/sd/sdcard: Explicit dummy byte value
>    hw/sd/sdcard: Do not abort when reading DAT lines on invalid cmd state
>    hw/sd/sdhci: Reset @data_count index on invalid ADMA transfers

Patches #1-3 queued (removing 'static' in patch #1).