diff mbox series

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

Message ID 20211215205656.488940-3-philmd@redhat.com
State New
Headers show
Series hw/sd/sdhci: Fix DMA re-entrancy issue | expand

Commit Message

Philippe Mathieu-Daudé Dec. 15, 2021, 8:56 p.m. UTC 
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(-)

Comments

Jason Wang Dec. 16, 2021, 3:14 a.m. UTC | #1 
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
>
Thomas Huth March 18, 2022, 6:38 p.m. UTC | #2 
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>
diff mbox series

Patch

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