Re: [PATCH v2 2/6] hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in progress

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> Per "SD Host Controller Standard Specification Version 7.00"
> chapter 2.2.1 SDMA System Address Register:
> 
> This register can be accessed only if no transaction is executing
> (i.e., after a transaction has stopped).
> 
> With this fix, the following reproducer:
> 
> https://paste.debian.net/plain/1185137

The reproducer is not huge, so better bury it with the commit
description:

outl 0xcf8 0x80001010
outl 0xcfc 0xfbefff00
outl 0xcf8 0x80001001
outl 0xcfc 0x06000000
write 0xfbefff2c 0x1 0x05
write 0xfbefff0f 0x1 0x37
write 0xfbefff0a 0x1 0x01
write 0xfbefff0f 0x1 0x29
write 0xfbefff0f 0x1 0x02
write 0xfbefff0f 0x1 0x03
write 0xfbefff04 0x1 0x01
write 0xfbefff05 0x1 0x01
write 0xfbefff07 0x1 0x02
write 0xfbefff0c 0x1 0x33
write 0xfbefff0e 0x1 0x20
write 0xfbefff0f 0x1 0x00
write 0xfbefff2a 0x1 0x01
write 0xfbefff0c 0x1 0x00
write 0xfbefff03 0x1 0x00
write 0xfbefff05 0x1 0x00
write 0xfbefff2a 0x1 0x02
write 0xfbefff0c 0x1 0x32
write 0xfbefff01 0x1 0x01
write 0xfbefff02 0x1 0x01
write 0xfbefff03 0x1 0x01

> cannot be reproduced with the following QEMU command line:
> 
> $ qemu-system-x86_64 -nographic -machine accel=qtest -m 512M \
>        -nodefaults -device sdhci-pci,sd-spec-version=3 \
>        -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
>        -device sd-card,drive=mydrive -qtest stdio

and even better add as qtest in tests/qtest/fuzz-sdhci-test.c :)

> Cc: qemu-stable@nongnu.org
> Fixes: CVE-2020-17380
> Fixes: CVE-2020-25085
> Fixes: CVE-2021-3409
> Fixes: d7dfca0807a0 ("hw/sdhci: introduce standard SD host controller")
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Reported-by: Cornelius Aschermann (Ruhr-University Bochum)
> Reported-by: Muhammad Ramdhan
> Reported-by: Sergej Schumilo (Ruhr-University Bochum)
> Reported-by: Simon Wrner (Ruhr-University Bochum)
> Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909418
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1928146
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 1c5ab26..05cb281 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1122,15 +1122,17 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t 
> val, unsigned size)
>  
>      switch (offset & ~0x3) {
>      case SDHC_SYSAD:
> -        s->sdmasysad = (s->sdmasysad & mask) | value;
> -        MASKED_WRITE(s->sdmasysad, mask, value);
> -        /* Writing to last byte of sdmasysad might trigger transfer */
> -        if (!(mask & 0xFF000000) && TRANSFERRING_DATA(s->prnsts) && 
> s->blkcnt &&
> -                s->blksize && SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
> -            if (s->trnmod & SDHC_TRNS_MULTI) {
> -                sdhci_sdma_transfer_multi_blocks(s);
> -            } else {
> -                sdhci_sdma_transfer_single_block(s);
> +        if (!TRANSFERRING_DATA(s->prnsts)) {
> +            s->sdmasysad = (s->sdmasysad & mask) | value;
> +            MASKED_WRITE(s->sdmasysad, mask, value);
> +            /* Writing to last byte of sdmasysad might trigger transfer */
> +            if (!(mask & 0xFF000000) && s->blkcnt && s->blksize &&
> +                SDHC_DMA_TYPE(s->hostctl1) == SDHC_CTRL_SDMA) {
> +                if (s->trnmod & SDHC_TRNS_MULTI) {
> +                    sdhci_sdma_transfer_multi_blocks(s);
> +                } else {
> +                    sdhci_sdma_transfer_single_block(s);
> +                }

Can you add:

     } else {
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Transfer already in progress, can not update
SYSAD",
                       __func__);

>              }
>          }
>          break;
>