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

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

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.