[PATCH v2 0/6] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[PATCH v2 0/6] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[Bin Meng]

This series includes several fixes to CVE-2020-17380, CVE-2020-25085
and CVE-2021-3409 that are heap-based buffer overflow issues existing
in the sdhci model.

These CVEs are pretty much similar, and were filed using different
reproducers. With this series, current known reproducers I have
cannot be reproduced any more.

The implementation of this model still has some issues, e.g.: some codes
do not seem to strictly match the spec, but since this series only aimes
to address CVEs, such issue should be fixed in a separate series in the
future, if I have time :)

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

Bin Meng (6):
  hw/sd: sdhci: Don't transfer any data when command time out
  hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in
    progress
  hw/sd: sdhci: Correctly set the controller status for ADMA
  hw/sd: sdhci: Simplify updating s->prnsts in
    sdhci_sdma_transfer_multi_blocks()
  hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is
    writable
  hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a
    different block size is programmed

 hw/sd/sdhci.c | 60 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 39 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v2 1/6] hw/sd: sdhci: Don't transfer any data when command time out

[Bin Meng]

At the end of sdhci_send_command(), it starts a data transfer if the
command register indicates data is associated. But the data transfer
should only be initiated when the command execution has succeeded.

With this fix, the following reproducer:

outl 0xcf8 0x80001810
outl 0xcfc 0xe1068000
outl 0xcf8 0x80001804
outw 0xcfc 0x7
write 0xe106802c 0x1 0x0f
write 0xe1068004 0xc 0x2801d10101fffffbff28a384
write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
write 0xe1068003 0x1 0xfe

cannot be reproduced with the following QEMU command line:

$ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
      -device sdhci-pci,sd-spec-version=3 \
      -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
      -device sd-card,drive=mydrive \
      -monitor none -serial none -qtest stdio

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>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

(no changes since v1)

 hw/sd/sdhci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 8ffa539..1c5ab26 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
     SDRequest request;
     uint8_t response[16];
     int rlen;
+    bool timeout = false;
 
     s->errintsts = 0;
     s->acmd12errsts = 0;
@@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
             trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
                                    s->rspreg[1], s->rspreg[0]);
         } else {
+            timeout = true;
             trace_sdhci_error("timeout waiting for command response");
             if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
                 s->errintsts |= SDHC_EIS_CMDTIMEOUT;
@@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
 
     sdhci_update_irq(s);
 
-    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
+    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
         s->data_count = 0;
         sdhci_data_transfer(s);
     }
-- 
2.7.4

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

[Bin Meng]

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

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

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);
+                }
             }
         }
         break;
-- 
2.7.4

[PATCH v2 3/6] hw/sd: sdhci: Correctly set the controller status for ADMA

[Bin Meng]

When an ADMA transfer is started, the codes forget to set the
controller status to indicate a transfer is in progress.

With this fix, the following 2 reproducers:

https://paste.debian.net/plain/1185136
https://paste.debian.net/plain/1185141

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

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 05cb281..0b0ca6f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -769,7 +769,9 @@ static void sdhci_do_adma(SDHCIState *s)
 
         switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
         case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
+            s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
             if (s->trnmod & SDHC_TRNS_READ) {
+                s->prnsts |= SDHC_DOING_READ;
                 while (length) {
                     if (s->data_count == 0) {
                         sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
@@ -797,6 +799,7 @@ static void sdhci_do_adma(SDHCIState *s)
                     }
                 }
             } else {
+                s->prnsts |= SDHC_DOING_WRITE;
                 while (length) {
                     begin = s->data_count;
                     if ((length + begin) < block_size) {
-- 
2.7.4

[PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Bin Meng]

s->prnsts is updated in both branches of the if () else () statement.
Move the common bits outside so that it is cleaner.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

(no changes since v1)

 hw/sd/sdhci.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 0b0ca6f..7a2003b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -598,9 +598,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         page_aligned = true;
     }
 
+    s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
     if (s->trnmod & SDHC_TRNS_READ) {
-        s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
-                SDHC_DAT_LINE_ACTIVE;
+        s->prnsts |= SDHC_DOING_READ;
         while (s->blkcnt) {
             if (s->data_count == 0) {
                 sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
@@ -627,8 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
             }
         }
     } else {
-        s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
-                SDHC_DAT_LINE_ACTIVE;
+        s->prnsts |= SDHC_DOING_WRITE;
         while (s->blkcnt) {
             begin = s->data_count;
             if (((boundary_count + begin) < block_size) && page_aligned) {
-- 
2.7.4

[PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Bin Meng]

The codes to limit the maximum block size is only necessary when
SDHC_BLKSIZE register is writable.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

 hw/sd/sdhci.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 7a2003b..d0c8e29 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1142,15 +1142,15 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         if (!TRANSFERRING_DATA(s->prnsts)) {
             MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
             MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
-        }
 
-        /* Limit block size to the maximum buffer size */
-        if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
-                          "the maximum buffer 0x%x\n", __func__, s->blksize,
-                          s->buf_maxsz);
+            /* Limit block size to the maximum buffer size */
+            if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
+                              "the maximum buffer 0x%x\n", __func__, s->blksize,
+                              s->buf_maxsz);
 
-            s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+                s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
+            }
         }
 
         break;
-- 
2.7.4

[PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Bin Meng]

If the block size is programmed to a different value from the
previous one, reset the data pointer of s->fifo_buffer[] so that
s->fifo_buffer[] can be filled in using the new block size in
the next transfer.

With this fix, the following reproducer:

outl 0xcf8 0x80001010
outl 0xcfc 0xe0000000
outl 0xcf8 0x80001001
outl 0xcfc 0x06000000
write 0xe000002c 0x1 0x05
write 0xe0000005 0x1 0x02
write 0xe0000007 0x1 0x01
write 0xe0000028 0x1 0x10
write 0x0 0x1 0x23
write 0x2 0x1 0x08
write 0xe000000c 0x1 0x01
write 0xe000000e 0x1 0x20
write 0xe000000f 0x1 0x00
write 0xe000000c 0x1 0x32
write 0xe0000004 0x2 0x0200
write 0xe0000028 0x1 0x00
write 0xe0000003 0x1 0x40

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

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>

---

Changes in v2:
- new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

 hw/sd/sdhci.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index d0c8e29..5b86781 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         break;
     case SDHC_BLKSIZE:
         if (!TRANSFERRING_DATA(s->prnsts)) {
+            uint16_t blksize = s->blksize;
+
             MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
             MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
 
@@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
 
                 s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
             }
+
+            /*
+             * If the block size is programmed to a different value from
+             * the previous one, reset the data pointer of s->fifo_buffer[]
+             * so that s->fifo_buffer[] can be filled in using the new block
+             * size in the next transfer.
+             */
+            if (blksize != s->blksize) {
+                s->data_count = 0;
+            }
         }
 
         break;
-- 
2.7.4

Re: [PATCH v2 0/6] hw/sd: sdhci: Fixes to CVE-2020-17380, CVE-2020-25085, CVE-2021-3409

[Alexander Bulekov]

Hi Bin,
For this series,
Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thank you
-Alex



On 210216 1146, Bin Meng wrote:
> This series includes several fixes to CVE-2020-17380, CVE-2020-25085
> and CVE-2021-3409 that are heap-based buffer overflow issues existing
> in the sdhci model.
> 
> These CVEs are pretty much similar, and were filed using different
> reproducers. With this series, current known reproducers I have
> cannot be reproduced any more.
> 
> The implementation of this model still has some issues, e.g.: some codes
> do not seem to strictly match the spec, but since this series only aimes
> to address CVEs, such issue should be fixed in a separate series in the
> future, if I have time :)
> 
> Changes in v2:
> - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
> - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed
> 
> Bin Meng (6):
>   hw/sd: sdhci: Don't transfer any data when command time out
>   hw/sd: sdhci: Don't write to SDHC_SYSAD register when transfer is in
>     progress
>   hw/sd: sdhci: Correctly set the controller status for ADMA
>   hw/sd: sdhci: Simplify updating s->prnsts in
>     sdhci_sdma_transfer_multi_blocks()
>   hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is
>     writable
>   hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a
>     different block size is programmed
> 
>  hw/sd/sdhci.c | 60 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 39 insertions(+), 21 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Alexander Bulekov]

On 210216 1146, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 0b0ca6f..7a2003b 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -598,9 +598,9 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>          page_aligned = true;
>      }
>  
> +    s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
>      if (s->trnmod & SDHC_TRNS_READ) {
> -        s->prnsts |= SDHC_DOING_READ | SDHC_DATA_INHIBIT |
> -                SDHC_DAT_LINE_ACTIVE;
> +        s->prnsts |= SDHC_DOING_READ;
>          while (s->blkcnt) {
>              if (s->data_count == 0) {
>                  sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
> @@ -627,8 +627,7 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
>              }
>          }
>      } else {
> -        s->prnsts |= SDHC_DOING_WRITE | SDHC_DATA_INHIBIT |
> -                SDHC_DAT_LINE_ACTIVE;
> +        s->prnsts |= SDHC_DOING_WRITE;
>          while (s->blkcnt) {
>              begin = s->data_count;
>              if (((boundary_count + begin) < block_size) && page_aligned) {
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/6] hw/sd: sdhci: Don't transfer any data when command time out

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> At the end of sdhci_send_command(), it starts a data transfer if the
> command register indicates data is associated. But the data transfer
> should only be initiated when the command execution has succeeded.
> 
> With this fix, the following reproducer:
> 
> outl 0xcf8 0x80001810
> outl 0xcfc 0xe1068000
> outl 0xcf8 0x80001804
> outw 0xcfc 0x7
> write 0xe106802c 0x1 0x0f
> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
> write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
> write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
> write 0xe1068003 0x1 0xfe
> 
> cannot be reproduced with the following QEMU command line:
> 
> $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
>       -device sdhci-pci,sd-spec-version=3 \
>       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
>       -device sd-card,drive=mydrive \
>       -monitor none -serial none -qtest stdio

Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
instead, similarly to tests/qtest/fuzz-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>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 8ffa539..1c5ab26 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
>      SDRequest request;
>      uint8_t response[16];
>      int rlen;
> +    bool timeout = false;
>  
>      s->errintsts = 0;
>      s->acmd12errsts = 0;
> @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
>              trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
>                                     s->rspreg[1], s->rspreg[0]);
>          } else {
> +            timeout = true;
>              trace_sdhci_error("timeout waiting for command response");
>              if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
>                  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
>  
>      sdhci_update_irq(s);
>  
> -    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> +    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {

No need to add 'timeout':

       if ((s->errintsts & SDHC_EIS_CMDTIMEOUT)
               && s->blksize
               && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {

>          s->data_count = 0;
>          sdhci_data_transfer(s);
>      }
> 

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;
> 

Re: [PATCH v2 1/6] hw/sd: sdhci: Don't transfer any data when command time out

[Philippe Mathieu-Daudé]

On 2/18/21 5:25 PM, Philippe Mathieu-Daudé wrote:
> On 2/16/21 4:46 AM, Bin Meng wrote:
>> At the end of sdhci_send_command(), it starts a data transfer if the
>> command register indicates data is associated. But the data transfer
>> should only be initiated when the command execution has succeeded.
>>
>> With this fix, the following reproducer:
>>
>> outl 0xcf8 0x80001810
>> outl 0xcfc 0xe1068000
>> outl 0xcf8 0x80001804
>> outw 0xcfc 0x7
>> write 0xe106802c 0x1 0x0f
>> write 0xe1068004 0xc 0x2801d10101fffffbff28a384
>> write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
>> write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
>> write 0xe1068003 0x1 0xfe
>>
>> cannot be reproduced with the following QEMU command line:
>>
>> $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
>>       -device sdhci-pci,sd-spec-version=3 \
>>       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
>>       -device sd-card,drive=mydrive \
>>       -monitor none -serial none -qtest stdio
> 
> Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
> instead, similarly to tests/qtest/fuzz-test.c?

Hold on, Alexander will send a RFC series to have that conversion
done automatically.

Re: [PATCH v2 3/6] hw/sd: sdhci: Correctly set the controller status for ADMA

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> When an ADMA transfer is started, the codes forget to set the
> controller status to indicate a transfer is in progress.
> 
> With this fix, the following 2 reproducers:
> 
> https://paste.debian.net/plain/1185136
> https://paste.debian.net/plain/1185141
> 
> 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
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 05cb281..0b0ca6f 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -769,7 +769,9 @@ static void sdhci_do_adma(SDHCIState *s)
>  
>          switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
>          case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
> +            s->prnsts |= SDHC_DATA_INHIBIT | SDHC_DAT_LINE_ACTIVE;
>              if (s->trnmod & SDHC_TRNS_READ) {
> +                s->prnsts |= SDHC_DOING_READ;
>                  while (length) {
>                      if (s->data_count == 0) {
>                          sdbus_read_data(&s->sdbus, s->fifo_buffer, block_size);
> @@ -797,6 +799,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                      }
>                  }
>              } else {
> +                s->prnsts |= SDHC_DOING_WRITE;
>                  while (length) {
>                      begin = s->data_count;
>                      if ((length + begin) < block_size) {

Nice fix.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> The codes to limit the maximum block size is only necessary when
> SDHC_BLKSIZE register is writable.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> 
> ---
> 
> Changes in v2:
> - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
> 
>  hw/sd/sdhci.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Philippe Mathieu-Daudé]

On 2/18/21 6:09 PM, Philippe Mathieu-Daudé wrote:
> On 2/16/21 4:46 AM, Bin Meng wrote:
>> The codes to limit the maximum block size is only necessary when
>> SDHC_BLKSIZE register is writable.

Per "SD Command Generation":

  The Host Driver should not read the SDMA System Address, Block Size
  and Block Count registers during a data transaction unless the
  transfer is stopped because the value is changing and not stable.
  To prevent destruction of registers using data transfer when issuing
  command, the 32-bit Block Count, Block Size, 16-bit Block Count and
  Transfer Mode registers shall be write protected by the Host
  Controller while Command Inhibit (DAT) is set to 1 in the Present
  State register.

Shouldn't we check for !(s->prnsts & SDHC_DATA_INHIBIT) instead?

>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>
>> ---
>>
>> Changes in v2:
>> - new patch: sdhci: Limit block size only when SDHC_BLKSIZE register is writable
>>
>>  hw/sd/sdhci.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

Re: [PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Philippe Mathieu-Daudé]

Hi Bin,

On 2/16/21 4:46 AM, Bin Meng wrote:
> If the block size is programmed to a different value from the
> previous one, reset the data pointer of s->fifo_buffer[] so that
> s->fifo_buffer[] can be filled in using the new block size in
> the next transfer.
> 
> With this fix, the following reproducer:
> 
> outl 0xcf8 0x80001010
> outl 0xcfc 0xe0000000
> outl 0xcf8 0x80001001
> outl 0xcfc 0x06000000
> write 0xe000002c 0x1 0x05
> write 0xe0000005 0x1 0x02
> write 0xe0000007 0x1 0x01
> write 0xe0000028 0x1 0x10
> write 0x0 0x1 0x23
> write 0x2 0x1 0x08
> write 0xe000000c 0x1 0x01
> write 0xe000000e 0x1 0x20
> write 0xe000000f 0x1 0x00
> write 0xe000000c 0x1 0x32
> write 0xe0000004 0x2 0x0200
> write 0xe0000028 0x1 0x00
> write 0xe0000003 0x1 0x40
> 
> 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
> 
> 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>
> 
> ---
> 
> Changes in v2:
> - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed
> 
>  hw/sd/sdhci.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index d0c8e29..5b86781 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>          break;
>      case SDHC_BLKSIZE:
>          if (!TRANSFERRING_DATA(s->prnsts)) {
> +            uint16_t blksize = s->blksize;
> +
>              MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
>              MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
>  
> @@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
>  
>                  s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
>              }
> +
> +            /*
> +             * If the block size is programmed to a different value from
> +             * the previous one, reset the data pointer of s->fifo_buffer[]
> +             * so that s->fifo_buffer[] can be filled in using the new block
> +             * size in the next transfer.
> +             */
> +            if (blksize != s->blksize) {
> +                s->data_count = 0;

I doubt the hardware works that way. Shouldn't we reset the FIFO
each time BLKSIZE is accessed, regardless of its previous value?

> +            }
>          }
>  
>          break;
> 

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
> 
> 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

Without the rest applied, I still can :(

AddressSanitizer: heap-buffer-overflow

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/18/21 7:23 PM, Philippe Mathieu-Daudé wrote:
> 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
>>
>> 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
> 
> Without the rest applied, I still can :(
> 
> AddressSanitizer: heap-buffer-overflow

Err, I used an old build for this test, sorry...

Re: [PATCH v2 1/6] hw/sd: sdhci: Don't transfer any data when command time out

[Bin Meng]

Hi Philippe,

On Fri, Feb 19, 2021 at 12:25 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/16/21 4:46 AM, Bin Meng wrote:
> > At the end of sdhci_send_command(), it starts a data transfer if the
> > command register indicates data is associated. But the data transfer
> > should only be initiated when the command execution has succeeded.
> >
> > With this fix, the following reproducer:
> >
> > outl 0xcf8 0x80001810
> > outl 0xcfc 0xe1068000
> > outl 0xcf8 0x80001804
> > outw 0xcfc 0x7
> > write 0xe106802c 0x1 0x0f
> > write 0xe1068004 0xc 0x2801d10101fffffbff28a384
> > write 0xe106800c 0x1f 0x9dacbbcad9e8f7061524334251606f7e8d9cabbac9d8e7f60514233241505f
> > write 0xe1068003 0x28 0x80d000251480d000252280d000253080d000253e80d000254c80d000255a80d000256880d0002576
> > write 0xe1068003 0x1 0xfe
> >
> > cannot be reproduced with the following QEMU command line:
> >
> > $ qemu-system-x86_64 -nographic -M pc-q35-5.0 \
> >       -device sdhci-pci,sd-spec-version=3 \
> >       -drive if=sd,index=0,file=null-co://,format=raw,id=mydrive \
> >       -device sd-card,drive=mydrive \
> >       -monitor none -serial none -qtest stdio
>
> Can you directly add the reproducer in tests/qtest/fuzz-sdhci-test.c
> instead, similarly to tests/qtest/fuzz-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>
> > Acked-by: Alistair Francis <alistair.francis@wdc.com>
> > Tested-by: Alexander Bulekov <alxndr@bu.edu>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >
> > (no changes since v1)
> >
> >  hw/sd/sdhci.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index 8ffa539..1c5ab26 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -326,6 +326,7 @@ static void sdhci_send_command(SDHCIState *s)
> >      SDRequest request;
> >      uint8_t response[16];
> >      int rlen;
> > +    bool timeout = false;
> >
> >      s->errintsts = 0;
> >      s->acmd12errsts = 0;
> > @@ -349,6 +350,7 @@ static void sdhci_send_command(SDHCIState *s)
> >              trace_sdhci_response16(s->rspreg[3], s->rspreg[2],
> >                                     s->rspreg[1], s->rspreg[0]);
> >          } else {
> > +            timeout = true;
> >              trace_sdhci_error("timeout waiting for command response");
> >              if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT) {
> >                  s->errintsts |= SDHC_EIS_CMDTIMEOUT;
> > @@ -369,7 +371,7 @@ static void sdhci_send_command(SDHCIState *s)
> >
> >      sdhci_update_irq(s);
> >
> > -    if (s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
> > +    if (!timeout && s->blksize && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>
> No need to add 'timeout':

But s->errintsts only gets updated if (s->errintstsen & SDHC_EISEN_CMDTIMEOUT).

>
>        if ((s->errintsts & SDHC_EIS_CMDTIMEOUT)
>                && s->blksize
>                && (s->cmdreg & SDHC_CMD_DATA_PRESENT)) {
>
> >          s->data_count = 0;
> >          sdhci_data_transfer(s);
> >      }

Regards,
Bin

Re: [PATCH v2 4/6] hw/sd: sdhci: Simplify updating s->prnsts in sdhci_sdma_transfer_multi_blocks()

[Philippe Mathieu-Daudé]

On 2/16/21 4:46 AM, Bin Meng wrote:
> s->prnsts is updated in both branches of the if () else () statement.
> Move the common bits outside so that it is cleaner.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  hw/sd/sdhci.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

As there are some questions in this series and it makes sense to
merge all patches at once to help downstream distributions track
the CVE fixes, I'm queuing this single patch to sdmmc-next tree.

Thanks,

Phil.

Re: [PATCH v2 6/6] hw/sd: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed

[Bin Meng]

Hi Philippe,

On Fri, Feb 19, 2021 at 2:06 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Bin,
>
> On 2/16/21 4:46 AM, Bin Meng wrote:
> > If the block size is programmed to a different value from the
> > previous one, reset the data pointer of s->fifo_buffer[] so that
> > s->fifo_buffer[] can be filled in using the new block size in
> > the next transfer.
> >
> > With this fix, the following reproducer:
> >
> > outl 0xcf8 0x80001010
> > outl 0xcfc 0xe0000000
> > outl 0xcf8 0x80001001
> > outl 0xcfc 0x06000000
> > write 0xe000002c 0x1 0x05
> > write 0xe0000005 0x1 0x02
> > write 0xe0000007 0x1 0x01
> > write 0xe0000028 0x1 0x10
> > write 0x0 0x1 0x23
> > write 0x2 0x1 0x08
> > write 0xe000000c 0x1 0x01
> > write 0xe000000e 0x1 0x20
> > write 0xe000000f 0x1 0x00
> > write 0xe000000c 0x1 0x32
> > write 0xe0000004 0x2 0x0200
> > write 0xe0000028 0x1 0x00
> > write 0xe0000003 0x1 0x40
> >
> > 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
> >
> > 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>
> >
> > ---
> >
> > Changes in v2:
> > - new patch: sdhci: Reset the data pointer of s->fifo_buffer[] when a different block size is programmed
> >
> >  hw/sd/sdhci.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index d0c8e29..5b86781 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -1140,6 +1140,8 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> >          break;
> >      case SDHC_BLKSIZE:
> >          if (!TRANSFERRING_DATA(s->prnsts)) {
> > +            uint16_t blksize = s->blksize;
> > +
> >              MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
> >              MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
> >
> > @@ -1151,6 +1153,16 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
> >
> >                  s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
> >              }
> > +
> > +            /*
> > +             * If the block size is programmed to a different value from
> > +             * the previous one, reset the data pointer of s->fifo_buffer[]
> > +             * so that s->fifo_buffer[] can be filled in using the new block
> > +             * size in the next transfer.
> > +             */
> > +            if (blksize != s->blksize) {
> > +                s->data_count = 0;
>
> I doubt the hardware works that way.

Me too, because s->data_count is not exposed by the hardware as a
register or descriptor, so it's purely our internal implementation. A
hardware might implement like that, but we really don't know unless
some hardware guys who designed a SDHC could jump out and comment :)

> Shouldn't we reset the FIFO each time BLKSIZE is accessed, regardless of its previous value?

If we do that, we will end up rewriting the logic of the data transfer
functions. I looked at the current implementation and I think there
are some spec violations about handling page boundaries, and that part
is related to sd->data-count. But like I said in the cover letter
these should be addressed in future patches.

>
> > +            }
> >          }
> >
> >          break;
> >

Regards,
Bin

Re: [PATCH v2 5/6] hw/sd: sdhci: Limit block size only when SDHC_BLKSIZE register is writable

[Bin Meng]

Hi Philippe,

On Fri, Feb 19, 2021 at 2:03 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 2/18/21 6:09 PM, Philippe Mathieu-Daudé wrote:
> > On 2/16/21 4:46 AM, Bin Meng wrote:
> >> The codes to limit the maximum block size is only necessary when
> >> SDHC_BLKSIZE register is writable.
>
> Per "SD Command Generation":
>
>   The Host Driver should not read the SDMA System Address, Block Size
>   and Block Count registers during a data transaction unless the
>   transfer is stopped because the value is changing and not stable.
>   To prevent destruction of registers using data transfer when issuing
>   command, the 32-bit Block Count, Block Size, 16-bit Block Count and
>   Transfer Mode registers shall be write protected by the Host
>   Controller while Command Inhibit (DAT) is set to 1 in the Present
>   State register.
>
> Shouldn't we check for !(s->prnsts & SDHC_DATA_INHIBIT) instead?

Yes, for accurate emulation I think we should.

Current implementation uses !(s->prnsts & (SDHC_DOING_READ |
SDHC_DOING_WRITE)) which eventually is correct, because:

SDHC_DATA_INHIBIT bit is set if either SDHC_DAT_LINE_ACTIVE or
SDHC_DOING_READ is set (SD Host Controller Spec v7.00 chapter 2.2.9
Present State Register)

SDHC_DAT_LINE_ACTIVE bit is set after the end bit of read or write
command, and after end bit of read or write command will generate
SDHC_DOING_READ or SDHC_DOING_WRITE (SD Host Controller Spec v7.00
chapter 2.2.9 Present State Register)

Regards,
Bin