[RFC PATCH 0/3] hw/sd/sdhci: Fix DMA re-entrancy issue

[RFC PATCH 0/3] hw/sd/sdhci: Fix DMA re-entrancy issue

[Philippe Mathieu-Daudé]

Hi,

This series is an attempt to fix the DMA re-entrancy problem
on the SDHCI device. OSS-Fuzz found it and Alexander generated
a helpful reproducer.

By setting the MemTxAttrs::memory bit before doing DMA transactions,
the flatview API will return MEMTX_BUS_ERROR if the transaction
targets a non-memory (a device), which is usually how DMA-reentrancy
bugs are exploited.

On real hardware, the checks are on the interconnect bus, not in
the SDHCI block. However QEMU blocks aren't modelled that way.
Using the flatview API seems (to me) the simplest and closer
to hardware, it is a generic API and we can use it to trace
bus transactions on all blocks.

Note this series is simply one example to fix the generic
issues. The important changes are in the previous series:
https://lore.kernel.org/qemu-devel/20211215182421.418374-1-philmd@redhat.com/
Based-on: <20211215182421.418374-1-philmd@redhat.com>
"physmem: Have flatview API check bus permission from MemTxAttrs"

Cc: Mauro Matteo Cascella <mcascell@redhat.com>
Cc: Qiuhao Li <Qiuhao.Li@outlook.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Li Qiang <liq3ea@gmail.com>
Cc: Thomas Huth <thuth@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>
Cc: Bandan Das <bsd@redhat.com>
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Cc: Darren Kenny <darren.kenny@oracle.com>
Cc: Bin Meng <bin.meng@windriver.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Alexander Bulekov <alxndr@bu.edu>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

Philippe Mathieu-Daudé (3):
  hw/sd/sdhci: Honor failed DMA transactions
  hw/sd/sdhci: Prohibit DMA accesses to devices
  tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue
    29225)

 hw/sd/sdhci.c                  | 35 ++++++++++++----
 tests/qtest/fuzz-sdcard-test.c | 76 ++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 9 deletions(-)

-- 
2.33.1

[RFC PATCH 1/3] hw/sd/sdhci: Honor failed DMA transactions

[Philippe Mathieu-Daudé]

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

DMA transactions might fail. The DMA API returns a MemTxResult,
indicating such failures. Do not ignore it. On failure, raise
the ADMA error flag and eventually triggering an IRQ (see spec
chapter 1.13.5: "ADMA2 States").

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index e0bbc903446..fe2f21f0c37 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -742,6 +742,7 @@ static void sdhci_do_adma(SDHCIState *s)
     unsigned int begin, length;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
     ADMADescr dscr = {};
+    MemTxResult res;
     int i;
 
     if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
@@ -790,10 +791,13 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_write(s->dma_as, dscr.addr,
-                                     &s->fifo_buffer[begin],
-                                     s->data_count - begin,
-                                     MEMTXATTRS_UNSPECIFIED);
+                    res = dma_memory_write(s->dma_as, dscr.addr,
+                                           &s->fifo_buffer[begin],
+                                           s->data_count - begin,
+                                           MEMTXATTRS_UNSPECIFIED);
+                    if (res != MEMTX_OK) {
+                        break;
+                    }
                     dscr.addr += s->data_count - begin;
                     if (s->data_count == block_size) {
                         s->data_count = 0;
@@ -816,10 +820,13 @@ static void sdhci_do_adma(SDHCIState *s)
                         s->data_count = block_size;
                         length -= block_size - begin;
                     }
-                    dma_memory_read(s->dma_as, dscr.addr,
-                                    &s->fifo_buffer[begin],
-                                    s->data_count - begin,
-                                    MEMTXATTRS_UNSPECIFIED);
+                    res = dma_memory_read(s->dma_as, dscr.addr,
+                                          &s->fifo_buffer[begin],
+                                          s->data_count - begin,
+                                          MEMTXATTRS_UNSPECIFIED);
+                    if (res != MEMTX_OK) {
+                        break;
+                    }
                     dscr.addr += s->data_count - begin;
                     if (s->data_count == block_size) {
                         sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size);
@@ -833,7 +840,16 @@ static void sdhci_do_adma(SDHCIState *s)
                     }
                 }
             }
-            s->admasysaddr += dscr.incr;
+            if (res != MEMTX_OK) {
+                if (s->errintstsen & SDHC_EISEN_ADMAERR) {
+                    trace_sdhci_error("Set ADMA error flag");
+                    s->errintsts |= SDHC_EIS_ADMAERR;
+                    s->norintsts |= SDHC_NIS_ERR;
+                }
+                sdhci_update_irq(s);
+            } else {
+                s->admasysaddr += dscr.incr;
+            }
             break;
         case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
             s->admasysaddr = dscr.addr;
-- 
2.33.1

[RFC PATCH 2/3] hw/sd/sdhci: Prohibit DMA accesses to devices

[Philippe Mathieu-Daudé]

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

The issue reported by OSS-Fuzz produces the following backtrace:

  ==447470==ERROR: AddressSanitizer: heap-buffer-overflow
  READ of size 1 at 0x61500002a080 thread T0
      #0 0x71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
      #1 0x7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
      #2 0x721b937b in memory_region_read_accessor softmmu/memory.c:440:11
      #3 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
      #4 0x7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
      #5 0x7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
      #6 0x7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
      #7 0x7212f958 in flatview_read softmmu/physmem.c:2921:12
      #8 0x7212f418 in address_space_read_full softmmu/physmem.c:2934:18
      #9 0x721305a9 in address_space_rw softmmu/physmem.c:2962:16
      #10 0x7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
      #11 0x7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
      #12 0x71759684 in dma_memory_read include/sysemu/dma.h:152:12
      #13 0x7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
      #14 0x7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
      #15 0x7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
      #16 0x717629ee in sdhci_write hw/sd/sdhci.c:1212:9
      #17 0x72172513 in memory_region_write_accessor softmmu/memory.c:492:5
      #18 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
      #19 0x72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
      #20 0x721419ee in flatview_write_continue softmmu/physmem.c:2812:23
      #21 0x721301eb in flatview_write softmmu/physmem.c:2854:12
      #22 0x7212fca8 in address_space_write softmmu/physmem.c:2950:18
      #23 0x721d9a53 in qtest_process_command softmmu/qtest.c:727:9

A DMA descriptor is previously filled in RAM. An I/O access to the
device (frames #22 to #16) start the DMA engine (frame #13). The
engine fetch the descriptor and execute the request, which itself
accesses the SDHCI I/O registers (frame #1 and #0), triggering a
re-entrancy issue.

Fix by prohibit transactions from the DMA to devices. The DMA engine
is thus restricted to memories.

Reported-by: OSS-Fuzz (Issue 36391)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/451
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sdhci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index fe2f21f0c37..0e5e988927e 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -741,6 +741,7 @@ static void sdhci_do_adma(SDHCIState *s)
 {
     unsigned int begin, length;
     const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
+    const MemTxAttrs attrs = { .memory = true };
     ADMADescr dscr = {};
     MemTxResult res;
     int i;
@@ -794,7 +795,7 @@ static void sdhci_do_adma(SDHCIState *s)
                     res = dma_memory_write(s->dma_as, dscr.addr,
                                            &s->fifo_buffer[begin],
                                            s->data_count - begin,
-                                           MEMTXATTRS_UNSPECIFIED);
+                                           attrs);
                     if (res != MEMTX_OK) {
                         break;
                     }
@@ -823,7 +824,7 @@ static void sdhci_do_adma(SDHCIState *s)
                     res = dma_memory_read(s->dma_as, dscr.addr,
                                           &s->fifo_buffer[begin],
                                           s->data_count - begin,
-                                          MEMTXATTRS_UNSPECIFIED);
+                                          attrs);
                     if (res != MEMTX_OK) {
                         break;
                     }
-- 
2.33.1

[RFC PATCH 3/3] tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225)

[Philippe Mathieu-Daudé]

Include the qtest reproducer provided by Alexander Bulekov
in https://gitlab.com/qemu-project/qemu/-/issues/451. Without
the previous commit, we get:

  $ make check-qtest-i386
  ...
  Running test qtest-i386/fuzz-sdcard-test
  ==447470==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61500002a080 at pc 0x564c71766d48 bp 0x7ffc126c62b0 sp 0x7ffc126c62a8
  READ of size 1 at 0x61500002a080 thread T0
      #0 0x564c71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
      #1 0x564c7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
      #2 0x564c721b937b in memory_region_read_accessor softmmu/memory.c:440:11
      #3 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
      #4 0x564c7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
      #5 0x564c7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
      #6 0x564c7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
      #7 0x564c7212f958 in flatview_read softmmu/physmem.c:2921:12
      #8 0x564c7212f418 in address_space_read_full softmmu/physmem.c:2934:18
      #9 0x564c721305a9 in address_space_rw softmmu/physmem.c:2962:16
      #10 0x564c7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
      #11 0x564c7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
      #12 0x564c71759684 in dma_memory_read include/sysemu/dma.h:152:12
      #13 0x564c7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
      #14 0x564c7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
      #15 0x564c7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
      #16 0x564c717629ee in sdhci_write hw/sd/sdhci.c:1212:9
      #17 0x564c72172513 in memory_region_write_accessor softmmu/memory.c:492:5
      #18 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
      #19 0x564c72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
      #20 0x564c721419ee in flatview_write_continue softmmu/physmem.c:2812:23
      #21 0x564c721301eb in flatview_write softmmu/physmem.c:2854:12
      #22 0x564c7212fca8 in address_space_write softmmu/physmem.c:2950:18
      #23 0x564c721d9a53 in qtest_process_command softmmu/qtest.c:727:9

  0x61500002a080 is located 0 bytes to the right of 512-byte region [0x615000029e80,0x61500002a080)
  allocated by thread T0 here:
      #0 0x564c708e1737 in __interceptor_calloc (qemu-system-i386+0x1e6a737)
      #1 0x7ff05567b5e0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a5e0)
      #2 0x564c71774adb in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5

  SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:474:18 in sdhci_read_dataport
  Shadow bytes around the buggy address:
    0x0c2a7fffd3c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c2a7fffd3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c2a7fffd3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c2a7fffd3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0c2a7fffd400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  =>0x0c2a7fffd410:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0c2a7fffd420: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2a7fffd430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2a7fffd440: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2a7fffd450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
    0x0c2a7fffd460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Heap left redzone:       fa
    Freed heap region:       fd
  ==447470==ABORTING
  Broken pipe
  ERROR qtest-i386/fuzz-sdcard-test - too few tests run (expected 3, got 2)

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/fuzz-sdcard-test.c | 76 ++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index ae14305344a..6dfe26e983c 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -87,6 +87,81 @@ static void oss_fuzz_36217(void)
     qtest_quit(s);
 }
 
+/*
+ * https://gitlab.com/qemu-project/qemu/-/issues/451
+ * Used to trigger a heap buffer overflow.
+ */
+static void oss_fuzz_36391(void)
+{
+    QTestState *s;
+
+    s = qtest_init(" -display none -m 4G -nodefaults -nographic"
+                   " -device sdhci-pci,sd-spec-version=3"
+                   " -device sd-card,drive=drv"
+                   " -drive if=none,index=0,file=null-co://,format=raw,id=drv");
+    qtest_outl(s, 0xcf8, 0x80001010);
+    qtest_outl(s, 0xcfc, 0xe0000000);
+    qtest_outl(s, 0xcf8, 0x80001004);
+    qtest_outw(s, 0xcfc, 0x7);
+    qtest_bufwrite(s, 0xe0000005, "\x73", 0x1);
+    qtest_bufwrite(s, 0xe0000028, "\x55", 0x1);
+    qtest_bufwrite(s, 0xe000002c, "\x55", 0x1);
+    qtest_bufwrite(s, 0x0, "\x65", 0x1);
+    qtest_bufwrite(s, 0x7, "\x69", 0x1);
+    qtest_bufwrite(s, 0x8, "\x65", 0x1);
+    qtest_bufwrite(s, 0xf, "\x69", 0x1);
+    qtest_bufwrite(s, 0x10, "\x65", 0x1);
+    qtest_bufwrite(s, 0x17, "\x69", 0x1);
+    qtest_bufwrite(s, 0x18, "\x65", 0x1);
+    qtest_bufwrite(s, 0x1f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x20, "\x65", 0x1);
+    qtest_bufwrite(s, 0x27, "\x69", 0x1);
+    qtest_bufwrite(s, 0x28, "\x65", 0x1);
+    qtest_bufwrite(s, 0x2f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x30, "\x65", 0x1);
+    qtest_bufwrite(s, 0x37, "\x69", 0x1);
+    qtest_bufwrite(s, 0x38, "\x65", 0x1);
+    qtest_bufwrite(s, 0x3f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x40, "\x65", 0x1);
+    qtest_bufwrite(s, 0x47, "\x69", 0x1);
+    qtest_bufwrite(s, 0x48, "\x65", 0x1);
+    qtest_bufwrite(s, 0xe000000c, "\x55", 0x1);
+    qtest_bufwrite(s, 0xe000000e, "\x2c", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x5b", 0x1);
+    qtest_bufwrite(s, 0xe0000010, "\x06\x46", 0x2);
+    qtest_bufwrite(s, 0x50, "\x65", 0x1);
+    qtest_bufwrite(s, 0x57, "\x69", 0x1);
+    qtest_bufwrite(s, 0x58, "\x65", 0x1);
+    qtest_bufwrite(s, 0x5f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x60, "\x65", 0x1);
+    qtest_bufwrite(s, 0x67, "\x69", 0x1);
+    qtest_bufwrite(s, 0x68, "\x65", 0x1);
+    qtest_bufwrite(s, 0x6f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x70, "\x65", 0x1);
+    qtest_bufwrite(s, 0x77, "\x69", 0x1);
+    qtest_bufwrite(s, 0x78, "\x65", 0x1);
+    qtest_bufwrite(s, 0x7f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x80, "\x65", 0x1);
+    qtest_bufwrite(s, 0x87, "\x69", 0x1);
+    qtest_bufwrite(s, 0x88, "\x65", 0x1);
+    qtest_bufwrite(s, 0x8f, "\x69", 0x1);
+    qtest_bufwrite(s, 0x90, "\x65", 0x1);
+    qtest_bufwrite(s, 0x97, "\x69", 0x1);
+    qtest_bufwrite(s, 0x98, "\x65", 0x1);
+    qtest_bufwrite(s, 0xe0000026, "\x5a\x06", 0x2);
+    qtest_bufwrite(s, 0xe0000028, "\x46\xc0\xc9\xc9", 0x4);
+    qtest_bufwrite(s, 0xe0000028, "\x55", 0x1);
+    qtest_bufwrite(s, 0xe000002a, "\x5a", 0x1);
+    qtest_bufwrite(s, 0xa0, "\x65", 0x1);
+    qtest_bufwrite(s, 0xa5, "\xff", 0x1);
+    qtest_bufwrite(s, 0xa6, "\xff", 0x1);
+    qtest_bufwrite(s, 0xa7, "\xdf", 0x1);
+    qtest_bufwrite(s, 0xe000000c, "\x27", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x55", 0x1);
+
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -96,6 +171,7 @@ int main(int argc, char **argv)
    if (strcmp(arch, "i386") == 0) {
         qtest_add_func("fuzz/sdcard/oss_fuzz_29225", oss_fuzz_29225);
         qtest_add_func("fuzz/sdcard/oss_fuzz_36217", oss_fuzz_36217);
+        qtest_add_func("fuzz/sdcard/oss_fuzz_36391", oss_fuzz_36391);
    }
 
    return g_test_run();
-- 
2.33.1

Re: [RFC PATCH 2/3] hw/sd/sdhci: Prohibit DMA accesses to devices

[Jason Wang]

On Thu, Dec 16, 2021 at 4:57 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> The issue reported by OSS-Fuzz produces the following backtrace:
>
>   ==447470==ERROR: AddressSanitizer: heap-buffer-overflow
>   READ of size 1 at 0x61500002a080 thread T0
>       #0 0x71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
>       #1 0x7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
>       #2 0x721b937b in memory_region_read_accessor softmmu/memory.c:440:11
>       #3 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>       #4 0x7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
>       #5 0x7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
>       #6 0x7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
>       #7 0x7212f958 in flatview_read softmmu/physmem.c:2921:12
>       #8 0x7212f418 in address_space_read_full softmmu/physmem.c:2934:18
>       #9 0x721305a9 in address_space_rw softmmu/physmem.c:2962:16
>       #10 0x7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
>       #11 0x7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
>       #12 0x71759684 in dma_memory_read include/sysemu/dma.h:152:12
>       #13 0x7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
>       #14 0x7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
>       #15 0x7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
>       #16 0x717629ee in sdhci_write hw/sd/sdhci.c:1212:9
>       #17 0x72172513 in memory_region_write_accessor softmmu/memory.c:492:5
>       #18 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>       #19 0x72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
>       #20 0x721419ee in flatview_write_continue softmmu/physmem.c:2812:23
>       #21 0x721301eb in flatview_write softmmu/physmem.c:2854:12
>       #22 0x7212fca8 in address_space_write softmmu/physmem.c:2950:18
>       #23 0x721d9a53 in qtest_process_command softmmu/qtest.c:727:9
>
> A DMA descriptor is previously filled in RAM. An I/O access to the
> device (frames #22 to #16) start the DMA engine (frame #13). The
> engine fetch the descriptor and execute the request, which itself
> accesses the SDHCI I/O registers (frame #1 and #0), triggering a
> re-entrancy issue.
>
> Fix by prohibit transactions from the DMA to devices. The DMA engine
> is thus restricted to memories.
>
> Reported-by: OSS-Fuzz (Issue 36391)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/451
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sdhci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index fe2f21f0c37..0e5e988927e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -741,6 +741,7 @@ static void sdhci_do_adma(SDHCIState *s)
>  {
>      unsigned int begin, length;
>      const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> +    const MemTxAttrs attrs = { .memory = true };
>      ADMADescr dscr = {};
>      MemTxResult res;
>      int i;
> @@ -794,7 +795,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                      res = dma_memory_write(s->dma_as, dscr.addr,
>                                             &s->fifo_buffer[begin],
>                                             s->data_count - begin,
> -                                           MEMTXATTRS_UNSPECIFIED);
> +                                           attrs);
>                      if (res != MEMTX_OK) {
>                          break;
>                      }
> @@ -823,7 +824,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                      res = dma_memory_read(s->dma_as, dscr.addr,
>                                            &s->fifo_buffer[begin],
>                                            s->data_count - begin,
> -                                          MEMTXATTRS_UNSPECIFIED);
> +                                          attrs);
>                      if (res != MEMTX_OK) {
>                          break;
>                      }

I wonder how we can fix this for other devices, as this seems to be a
known issue for many years. We've received many reports from the
networking side.

It looks like this patch simply forbids p2p which is probably not the
case for other devices.

I remember there's ideas like using bh from Paolo or detecting
reentrancy in the memory core, both of them seems more general than
this?

Thanks

> --
> 2.33.1
>

Re: [RFC PATCH 3/3] tests/qtest/fuzz-sdcard-test: Add reproducer for OSS-Fuzz (Issue 29225)

[Thomas Huth]

On 15/12/2021 21.56, Philippe Mathieu-Daudé wrote:
> Include the qtest reproducer provided by Alexander Bulekov
> in https://gitlab.com/qemu-project/qemu/-/issues/451. Without
> the previous commit, we get:
> 
>    $ make check-qtest-i386
>    ...
>    Running test qtest-i386/fuzz-sdcard-test
>    ==447470==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61500002a080 at pc 0x564c71766d48 bp 0x7ffc126c62b0 sp 0x7ffc126c62a8
>    READ of size 1 at 0x61500002a080 thread T0
>        #0 0x564c71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
>        #1 0x564c7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
>        #2 0x564c721b937b in memory_region_read_accessor softmmu/memory.c:440:11
>        #3 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>        #4 0x564c7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
>        #5 0x564c7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
>        #6 0x564c7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
>        #7 0x564c7212f958 in flatview_read softmmu/physmem.c:2921:12
>        #8 0x564c7212f418 in address_space_read_full softmmu/physmem.c:2934:18
>        #9 0x564c721305a9 in address_space_rw softmmu/physmem.c:2962:16
>        #10 0x564c7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
>        #11 0x564c7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
>        #12 0x564c71759684 in dma_memory_read include/sysemu/dma.h:152:12
>        #13 0x564c7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
>        #14 0x564c7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
>        #15 0x564c7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
>        #16 0x564c717629ee in sdhci_write hw/sd/sdhci.c:1212:9
>        #17 0x564c72172513 in memory_region_write_accessor softmmu/memory.c:492:5
>        #18 0x564c72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>        #19 0x564c72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
>        #20 0x564c721419ee in flatview_write_continue softmmu/physmem.c:2812:23
>        #21 0x564c721301eb in flatview_write softmmu/physmem.c:2854:12
>        #22 0x564c7212fca8 in address_space_write softmmu/physmem.c:2950:18
>        #23 0x564c721d9a53 in qtest_process_command softmmu/qtest.c:727:9
> 
>    0x61500002a080 is located 0 bytes to the right of 512-byte region [0x615000029e80,0x61500002a080)
>    allocated by thread T0 here:
>        #0 0x564c708e1737 in __interceptor_calloc (qemu-system-i386+0x1e6a737)
>        #1 0x7ff05567b5e0 in g_malloc0 (/lib64/libglib-2.0.so.0+0x5a5e0)
>        #2 0x564c71774adb in sdhci_pci_realize hw/sd/sdhci-pci.c:36:5
> 
>    SUMMARY: AddressSanitizer: heap-buffer-overflow hw/sd/sdhci.c:474:18 in sdhci_read_dataport
>    Shadow bytes around the buggy address:
>      0x0c2a7fffd3c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c2a7fffd3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c2a7fffd3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c2a7fffd3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>      0x0c2a7fffd400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>    =>0x0c2a7fffd410:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>      0x0c2a7fffd420: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c2a7fffd430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c2a7fffd440: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c2a7fffd450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c2a7fffd460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>    Shadow byte legend (one shadow byte represents 8 application bytes):
>      Addressable:           00
>      Heap left redzone:       fa
>      Freed heap region:       fd
>    ==447470==ABORTING
>    Broken pipe
>    ERROR qtest-i386/fuzz-sdcard-test - too few tests run (expected 3, got 2)
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qtest/fuzz-sdcard-test.c | 76 ++++++++++++++++++++++++++++++++++
>   1 file changed, 76 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [RFC PATCH 1/3] hw/sd/sdhci: Honor failed DMA transactions

[Thomas Huth]

On 15/12/2021 21.56, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> DMA transactions might fail. The DMA API returns a MemTxResult,
> indicating such failures. Do not ignore it. On failure, raise
> the ADMA error flag and eventually triggering an IRQ (see spec
> chapter 1.13.5: "ADMA2 States").
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/sd/sdhci.c | 34 +++++++++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index e0bbc903446..fe2f21f0c37 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -742,6 +742,7 @@ static void sdhci_do_adma(SDHCIState *s)
>       unsigned int begin, length;
>       const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
>       ADMADescr dscr = {};
> +    MemTxResult res;
>       int i;
>   
>       if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
> @@ -790,10 +791,13 @@ static void sdhci_do_adma(SDHCIState *s)
>                           s->data_count = block_size;
>                           length -= block_size - begin;
>                       }
> -                    dma_memory_write(s->dma_as, dscr.addr,
> -                                     &s->fifo_buffer[begin],
> -                                     s->data_count - begin,
> -                                     MEMTXATTRS_UNSPECIFIED);
> +                    res = dma_memory_write(s->dma_as, dscr.addr,
> +                                           &s->fifo_buffer[begin],
> +                                           s->data_count - begin,
> +                                           MEMTXATTRS_UNSPECIFIED);
> +                    if (res != MEMTX_OK) {
> +                        break;
> +                    }
>                       dscr.addr += s->data_count - begin;
>                       if (s->data_count == block_size) {
>                           s->data_count = 0;
> @@ -816,10 +820,13 @@ static void sdhci_do_adma(SDHCIState *s)
>                           s->data_count = block_size;
>                           length -= block_size - begin;
>                       }
> -                    dma_memory_read(s->dma_as, dscr.addr,
> -                                    &s->fifo_buffer[begin],
> -                                    s->data_count - begin,
> -                                    MEMTXATTRS_UNSPECIFIED);
> +                    res = dma_memory_read(s->dma_as, dscr.addr,
> +                                          &s->fifo_buffer[begin],
> +                                          s->data_count - begin,
> +                                          MEMTXATTRS_UNSPECIFIED);
> +                    if (res != MEMTX_OK) {
> +                        break;
> +                    }
>                       dscr.addr += s->data_count - begin;
>                       if (s->data_count == block_size) {
>                           sdbus_write_data(&s->sdbus, s->fifo_buffer, block_size);
> @@ -833,7 +840,16 @@ static void sdhci_do_adma(SDHCIState *s)
>                       }
>                   }
>               }
> -            s->admasysaddr += dscr.incr;
> +            if (res != MEMTX_OK) {
> +                if (s->errintstsen & SDHC_EISEN_ADMAERR) {
> +                    trace_sdhci_error("Set ADMA error flag");
> +                    s->errintsts |= SDHC_EIS_ADMAERR;
> +                    s->norintsts |= SDHC_NIS_ERR;
> +                }
> +                sdhci_update_irq(s);
> +            } else {
> +                s->admasysaddr += dscr.incr;
> +            }
>               break;
>           case SDHC_ADMA_ATTR_ACT_LINK:   /* link to next descriptor table */
>               s->admasysaddr = dscr.addr;

Patch looks sane to me:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Are you still considering it or did you drop this from your TODO list? 
(since it was just marked as RFC?)

  Thomas

Re: [RFC PATCH 2/3] hw/sd/sdhci: Prohibit DMA accesses to devices

[Thomas Huth]

On 15/12/2021 21.56, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> The issue reported by OSS-Fuzz produces the following backtrace:
> 
>    ==447470==ERROR: AddressSanitizer: heap-buffer-overflow
>    READ of size 1 at 0x61500002a080 thread T0
>        #0 0x71766d47 in sdhci_read_dataport hw/sd/sdhci.c:474:18
>        #1 0x7175f139 in sdhci_read hw/sd/sdhci.c:1022:19
>        #2 0x721b937b in memory_region_read_accessor softmmu/memory.c:440:11
>        #3 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>        #4 0x7216f47c in memory_region_dispatch_read1 softmmu/memory.c:1424:16
>        #5 0x7216ebb9 in memory_region_dispatch_read softmmu/memory.c:1452:9
>        #6 0x7212db5d in flatview_read_continue softmmu/physmem.c:2879:23
>        #7 0x7212f958 in flatview_read softmmu/physmem.c:2921:12
>        #8 0x7212f418 in address_space_read_full softmmu/physmem.c:2934:18
>        #9 0x721305a9 in address_space_rw softmmu/physmem.c:2962:16
>        #10 0x7175a392 in dma_memory_rw_relaxed include/sysemu/dma.h:89:12
>        #11 0x7175a0ea in dma_memory_rw include/sysemu/dma.h:132:12
>        #12 0x71759684 in dma_memory_read include/sysemu/dma.h:152:12
>        #13 0x7175518c in sdhci_do_adma hw/sd/sdhci.c:823:27
>        #14 0x7174bf69 in sdhci_data_transfer hw/sd/sdhci.c:935:13
>        #15 0x7176aaa7 in sdhci_send_command hw/sd/sdhci.c:376:9
>        #16 0x717629ee in sdhci_write hw/sd/sdhci.c:1212:9
>        #17 0x72172513 in memory_region_write_accessor softmmu/memory.c:492:5
>        #18 0x72171e51 in access_with_adjusted_size softmmu/memory.c:554:18
>        #19 0x72170766 in memory_region_dispatch_write softmmu/memory.c:1504:16
>        #20 0x721419ee in flatview_write_continue softmmu/physmem.c:2812:23
>        #21 0x721301eb in flatview_write softmmu/physmem.c:2854:12
>        #22 0x7212fca8 in address_space_write softmmu/physmem.c:2950:18
>        #23 0x721d9a53 in qtest_process_command softmmu/qtest.c:727:9
> 
> A DMA descriptor is previously filled in RAM. An I/O access to the
> device (frames #22 to #16) start the DMA engine (frame #13). The
> engine fetch the descriptor and execute the request, which itself
> accesses the SDHCI I/O registers (frame #1 and #0), triggering a
> re-entrancy issue.
> 
> Fix by prohibit transactions from the DMA to devices. The DMA engine
> is thus restricted to memories.
> 
> Reported-by: OSS-Fuzz (Issue 36391)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/451
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/sd/sdhci.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index fe2f21f0c37..0e5e988927e 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -741,6 +741,7 @@ static void sdhci_do_adma(SDHCIState *s)
>   {
>       unsigned int begin, length;
>       const uint16_t block_size = s->blksize & BLOCK_SIZE_MASK;
> +    const MemTxAttrs attrs = { .memory = true };
>       ADMADescr dscr = {};
>       MemTxResult res;
>       int i;
> @@ -794,7 +795,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                       res = dma_memory_write(s->dma_as, dscr.addr,
>                                              &s->fifo_buffer[begin],
>                                              s->data_count - begin,
> -                                           MEMTXATTRS_UNSPECIFIED);
> +                                           attrs);
>                       if (res != MEMTX_OK) {
>                           break;
>                       }
> @@ -823,7 +824,7 @@ static void sdhci_do_adma(SDHCIState *s)
>                       res = dma_memory_read(s->dma_as, dscr.addr,
>                                             &s->fifo_buffer[begin],
>                                             s->data_count - begin,
> -                                          MEMTXATTRS_UNSPECIFIED);
> +                                          attrs);
>                       if (res != MEMTX_OK) {
>                           break;
>                       }

Looks sane to me!

Reviewed-by: Thomas Huth <thuth@redhat.com>