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

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