[PATCH-for-9.1? v2 0/4] hw/sd/sdhci: Check ADMA descriptors can be accessed

[PATCH-for-9.1? v2 0/4] hw/sd/sdhci: Check ADMA descriptors can be accessed

[Philippe Mathieu-Daudé]

Since v1:
- split patch
- do not return MemTxResult from get_adma_description()
- single DMA read in SDHC_CTRL_ADMA2_64 case

Based-on: <20240730092138.32443-5-philmd@linaro.org>

Philippe Mathieu-Daudé (4):
  hw/sd/sdhci: Reduce variables scope in sdhci_do_adma()
  hw/sd/sdhci: Reduce variables scope in get_adma_description()
  hw/sd/sdhci: Read ADMA2_64 descriptor with a single dma_memory_read()
  hw/sd/sdhci: Check ADMA descriptors can be accessed

 hw/sd/sdhci.c | 117 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 47 deletions(-)

-- 
2.45.2

[PATCH-for-9.1? v2 1/4] hw/sd/sdhci: Reduce variables scope in sdhci_do_adma()

[Philippe Mathieu-Daudé]

All variables are only used within the for loop.
Declare them within it. In particular this resets
'dscr' on each iteration.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 66b9364e9e..773f2b284b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -751,20 +751,19 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 
 static void sdhci_do_adma(SDHCIState *s)
 {
-    unsigned int begin, length;
-    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) {
         /* Stop Multiple Transfer */
         sdhci_end_transfer(s);
         return;
     }
 
-    for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
+    for (int i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
+        unsigned int begin, length;
+        const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
+        const MemTxAttrs attrs = { .memory = true };
+        ADMADescr dscr = { };
+        MemTxResult res;
+
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
         get_adma_description(s, &dscr);
-- 
2.45.2

[PATCH-for-9.1? v2 2/4] hw/sd/sdhci: Reduce variables scope in get_adma_description()

[Philippe Mathieu-Daudé]

The 'adma1' variable is only used in the SDHC_CTRL_ADMA1_32
case, and 'adma2' in SDHC_CTRL_ADMA2_32. Add braces in the
switch case to use local declarations.
Do the same in the SDHC_CTRL_ADMA2_64 case because we'll add
a local variable there in the next commit.

Replace 0xFFFFF000 -> ~0xfff to align with our codebase style.

No functional change intended.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 773f2b284b..0a95f49b93 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -700,50 +700,59 @@ static void trace_adma_description(const char *type, const ADMADescr *dscr)
 
 static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 {
-    uint32_t adma1 = 0;
-    uint64_t adma2 = 0;
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
     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);
-        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;
+        {
+            uint64_t adma2 = 0;
+
+            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);
+            break;
+        }
+    case SDHC_CTRL_ADMA1_32:
+        {
+            uint32_t adma1 = 0;
+
+            dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
+                            MEMTXATTRS_UNSPECIFIED);
+            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);
+            break;
         }
-        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);
-        break;
+        {
+            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);
+            break;
+        }
     }
 }
 
-- 
2.45.2

[PATCH-for-9.1? v2 3/4] hw/sd/sdhci: Read ADMA2_64 descriptor with a single dma_memory_read()

[Philippe Mathieu-Daudé]

Instead of 3 consecutive dma_memory_read() calls, use
a packed structure to read the descriptor in a single
call.

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0a95f49b93..2d8fa3151a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -740,16 +740,20 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         }
     case SDHC_CTRL_ADMA2_64:
         {
-            dma_memory_read(s->dma_as, entry_addr, &dscr->attr, 1,
+            struct {
+                uint8_t attr;
+                uint8_t pad;
+                uint16_t length;
+                uint64_t addr;
+            } QEMU_PACKED adma2;
+            QEMU_BUILD_BUG_ON(sizeof(adma2) != 12);
+
+            dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
                             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;
+            dscr->length = le16_to_cpu(adma2.length);
+            dscr->addr = le64_to_cpu(adma2.addr);
+            dscr->attr = adma2.attr & (uint8_t) ~0xc0;
+            dscr->incr = sizeof(adma2);
             trace_adma_description("ADMA2_64", dscr);
             break;
         }
-- 
2.45.2

[PATCH-for-9.1? v2 4/4] 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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 2d8fa3151a..6794ee2267 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -701,13 +701,18 @@ static void trace_adma_description(const char *type, const ADMADescr *dscr)
 static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
 {
     hwaddr entry_addr = (hwaddr)s->admasysaddr;
+    MemTxResult res;
+
     switch (SDHC_DMA_TYPE(s->hostctl1)) {
     case SDHC_CTRL_ADMA2_32:
         {
             uint64_t adma2 = 0;
 
-            dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
-                            MEMTXATTRS_UNSPECIFIED);
+            res = dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
+                                  MEMTXATTRS_UNSPECIFIED);
+            if (res != MEMTX_OK) {
+                break;
+            }
             adma2 = le64_to_cpu(adma2);
             /*
              * The spec does not specify endianness of descriptor table.
@@ -724,8 +729,11 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         {
             uint32_t adma1 = 0;
 
-            dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
-                            MEMTXATTRS_UNSPECIFIED);
+            res = dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
+                                  MEMTXATTRS_UNSPECIFIED);
+            if (res != MEMTX_OK) {
+                break;
+            }
             adma1 = le32_to_cpu(adma1);
             dscr->addr = (hwaddr)(adma1 & ~0xfff);
             dscr->attr = (uint8_t)extract32(adma1, 0, 7);
@@ -748,8 +756,11 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
             } QEMU_PACKED adma2;
             QEMU_BUILD_BUG_ON(sizeof(adma2) != 12);
 
-            dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
-                            MEMTXATTRS_UNSPECIFIED);
+            res = dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
+                                  MEMTXATTRS_UNSPECIFIED);
+            if (res != MEMTX_OK) {
+                break;
+            }
             dscr->length = le16_to_cpu(adma2.length);
             dscr->addr = le64_to_cpu(adma2.addr);
             dscr->attr = adma2.attr & (uint8_t) ~0xc0;
-- 
2.45.2

Re: [PATCH-for-9.1? v2 0/4] hw/sd/sdhci: Check ADMA descriptors can be accessed

[Philippe Mathieu-Daudé]

On 31/7/24 23:24, Philippe Mathieu-Daudé wrote:
> Since v1:
> - split patch
> - do not return MemTxResult from get_adma_description()
> - single DMA read in SDHC_CTRL_ADMA2_64 case
> 
> Based-on: <20240730092138.32443-5-philmd@linaro.org>
> 
> Philippe Mathieu-Daudé (4):
>    hw/sd/sdhci: Reduce variables scope in sdhci_do_adma()
>    hw/sd/sdhci: Reduce variables scope in get_adma_description()
>    hw/sd/sdhci: Read ADMA2_64 descriptor with a single dma_memory_read()
>    hw/sd/sdhci: Check ADMA descriptors can be accessed
> 
>   hw/sd/sdhci.c | 117 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 70 insertions(+), 47 deletions(-)
> 

Ping?

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

[Michael Tokarev]

01.08.2024 00:25, 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.

Ping?  This has been about the 9.1 release, now 9.2 is out already
and we're working on 10.0...

Thanks,

/mjt

> 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 | 23 +++++++++++++++++------
>   1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 2d8fa3151a..6794ee2267 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -701,13 +701,18 @@ static void trace_adma_description(const char *type, const ADMADescr *dscr)
>   static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>   {
>       hwaddr entry_addr = (hwaddr)s->admasysaddr;
> +    MemTxResult res;
> +
>       switch (SDHC_DMA_TYPE(s->hostctl1)) {
>       case SDHC_CTRL_ADMA2_32:
>           {
>               uint64_t adma2 = 0;
>   
> -            dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
> -                            MEMTXATTRS_UNSPECIFIED);
> +            res = dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
> +                                  MEMTXATTRS_UNSPECIFIED);
> +            if (res != MEMTX_OK) {
> +                break;
> +            }
>               adma2 = le64_to_cpu(adma2);
>               /*
>                * The spec does not specify endianness of descriptor table.
> @@ -724,8 +729,11 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>           {
>               uint32_t adma1 = 0;
>   
> -            dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
> -                            MEMTXATTRS_UNSPECIFIED);
> +            res = dma_memory_read(s->dma_as, entry_addr, &adma1, sizeof(adma1),
> +                                  MEMTXATTRS_UNSPECIFIED);
> +            if (res != MEMTX_OK) {
> +                break;
> +            }
>               adma1 = le32_to_cpu(adma1);
>               dscr->addr = (hwaddr)(adma1 & ~0xfff);
>               dscr->attr = (uint8_t)extract32(adma1, 0, 7);
> @@ -748,8 +756,11 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
>               } QEMU_PACKED adma2;
>               QEMU_BUILD_BUG_ON(sizeof(adma2) != 12);
>   
> -            dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
> -                            MEMTXATTRS_UNSPECIFIED);
> +            res = dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
> +                                  MEMTXATTRS_UNSPECIFIED);
> +            if (res != MEMTX_OK) {
> +                break;
> +            }
>               dscr->length = le16_to_cpu(adma2.length);
>               dscr->addr = le64_to_cpu(adma2.addr);
>               dscr->attr = adma2.attr & (uint8_t) ~0xc0;


-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH-for-9.1? v2 0/4] hw/sd/sdhci: Check ADMA descriptors can be accessed

[Philippe Mathieu-Daudé]

Cc'ing more SDHCI developers.

On 31/7/24 23:24, Philippe Mathieu-Daudé wrote:
> Since v1:
> - split patch
> - do not return MemTxResult from get_adma_description()
> - single DMA read in SDHC_CTRL_ADMA2_64 case
> 
> Based-on: <20240730092138.32443-5-philmd@linaro.org>
> 
> Philippe Mathieu-Daudé (4):
>    hw/sd/sdhci: Reduce variables scope in sdhci_do_adma()
>    hw/sd/sdhci: Reduce variables scope in get_adma_description()
>    hw/sd/sdhci: Read ADMA2_64 descriptor with a single dma_memory_read()
>    hw/sd/sdhci: Check ADMA descriptors can be accessed
> 
>   hw/sd/sdhci.c | 117 ++++++++++++++++++++++++++++++--------------------
>   1 file changed, 70 insertions(+), 47 deletions(-)
>