Re: [RFC PATCH 00/12] hw: Forbid DMA write accesses to MMIO regions

[Li Qiang <liq3ea@gmail.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> 于2020年9月3日周四 下午7:09写道：
>
> Hi,
>
> I'm not suppose to work on this but I couldn't sleep so kept
> wondering about this problem the whole night and eventually
> woke up to write this quickly, so comments are scarce, sorry.
>
> The first part is obvious anyway, simply pass MemTxAttrs argument.
>
> The main patch is:
> "exec/memattrs: Introduce MemTxAttrs::direct_access field".
> This way we can restrict accesses to ROM/RAM by setting the
> 'direct_access' field. Illegal accesses return MEMTX_BUS_ERROR.
>
> Next patch restrict PCI DMA accesses by setting the direct_access
> field.
>
> Finally we add an assertion for any DMA write access to indirect
> memory to kill a class of bug recently found by Alexander while
> fuzzing.
>

Hi Philippe,

I have reviewed your patches.
Your patch just deny the DMA write to MMIO for PCI device.

1. The DMA write to MMIO is allowed for P2P. Unconditionally deny
is not right I think. Maybe we can add some flag for the device as property
so the device can indicate whether it supports DMA to MMIO.
But this method needs define we should apply the restrict to
DMA to MMIO initiator or target. If the target, we need to find the
target PCI device.

2. I think the MMIO read maybe also suffers the reentrant issue If the
MMIO read handler
does complicated work.

3. As your patch just consider the PCI case. This reentrant is quite
complicated if we consider
the no-PCI the qemu_irq cases. I agree to address the PCI cases first.

Thanks,
Li Qiang



> Regards,
>
> Phil.
>
> Klaus Jensen (1):
>   pci: pass along the return value of dma_memory_rw
>
> Philippe Mathieu-Daudé (11):
>   dma: Let dma_memory_valid() take MemTxAttrs argument
>   dma: Let dma_memory_set() take MemTxAttrs argument
>   dma: Let dma_memory_rw_relaxed() take MemTxAttrs argument
>   dma: Let dma_memory_rw() take MemTxAttrs argument
>   dma: Let dma_memory_read/write() take MemTxAttrs argument
>   dma: Let dma_memory_map() take MemTxAttrs argument
>   docs/devel/loads-stores: Add regexp for DMA functions
>   dma: Let load/store DMA functions take MemTxAttrs argument
>   exec/memattrs: Introduce MemTxAttrs::direct_access field
>   hw/pci: Only allow PCI slave devices to write to direct memory
>   dma: Assert when device writes to indirect memory (such MMIO regions)
>
>  docs/devel/loads-stores.rst   |  2 ++
>  include/exec/memattrs.h       |  3 ++
>  include/hw/pci/pci.h          | 21 ++++++++++---
>  include/hw/ppc/spapr_vio.h    | 26 +++++++++------
>  include/sysemu/dma.h          | 59 +++++++++++++++++++++--------------
>  dma-helpers.c                 | 12 ++++---
>  exec.c                        |  8 +++++
>  hw/arm/musicpal.c             | 13 ++++----
>  hw/arm/smmu-common.c          |  3 +-
>  hw/arm/smmuv3.c               | 14 ++++++---
>  hw/core/generic-loader.c      |  3 +-
>  hw/display/virtio-gpu.c       |  8 +++--
>  hw/dma/pl330.c                | 12 ++++---
>  hw/dma/sparc32_dma.c          | 16 ++++++----
>  hw/dma/xlnx-zynq-devcfg.c     |  6 ++--
>  hw/dma/xlnx_dpdma.c           | 10 +++---
>  hw/hyperv/vmbus.c             |  8 +++--
>  hw/i386/amd_iommu.c           | 16 +++++-----
>  hw/i386/intel_iommu.c         | 28 ++++++++++-------
>  hw/ide/ahci.c                 |  9 ++++--
>  hw/ide/macio.c                |  2 +-
>  hw/intc/pnv_xive.c            |  7 +++--
>  hw/intc/spapr_xive.c          |  3 +-
>  hw/intc/xive.c                |  7 +++--
>  hw/misc/bcm2835_property.c    |  3 +-
>  hw/misc/macio/mac_dbdma.c     | 10 +++---
>  hw/net/allwinner-sun8i-emac.c | 21 ++++++++-----
>  hw/net/ftgmac100.c            | 25 +++++++++------
>  hw/net/imx_fec.c              | 32 ++++++++++++-------
>  hw/nvram/fw_cfg.c             | 16 ++++++----
>  hw/pci-host/pnv_phb3.c        |  5 +--
>  hw/pci-host/pnv_phb3_msi.c    |  9 ++++--
>  hw/pci-host/pnv_phb4.c        |  7 +++--
>  hw/sd/allwinner-sdhost.c      | 14 +++++----
>  hw/sd/sdhci.c                 | 35 +++++++++++++--------
>  hw/usb/hcd-dwc2.c             |  8 ++---
>  hw/usb/hcd-ehci.c             |  6 ++--
>  hw/usb/hcd-ohci.c             | 28 ++++++++++-------
>  hw/usb/libhw.c                |  3 +-
>  hw/virtio/virtio.c            |  6 ++--
>  trace-events                  |  1 +
>  41 files changed, 334 insertions(+), 191 deletions(-)
>
> --
> 2.26.2
>
>