| Message ID | 20240730092138.32443-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/sd: SDcard & SDHCI fixes | expand |
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.
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"
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(-)