Re: [PATCH v2] memory: prevent dma-reentracy issues

[David Hildenbrand]

On 09.06.22 15:58, Alexander Bulekov wrote:
> Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA.
> This flag is set/checked prior to calling a device's MemoryRegion
> handlers, and set when device code initiates DMA.  The purpose of this
> flag is to prevent two types of DMA-based reentrancy issues:
> 
> 1.) mmio -> dma -> mmio case
> 2.) bh -> dma write -> mmio case
> 
> These issues have led to problems such as stack-exhaustion and
> use-after-frees.
> 
> Summary of the problem from Peter Maydell:
> https://lore.kernel.org/qemu-devel/CAFEAcA_23vc7hE3iaM-JVA6W38LK4hJoWae5KcknhPRD5fPBZA@mail.gmail.com
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/62
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/540
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/541
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/556
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/557
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/827
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  include/hw/pci/pci.h   | 13 +++++++++++--
>  include/hw/qdev-core.h |  3 +++
>  softmmu/dma-helpers.c  | 12 ++++++++++++
>  softmmu/memory.c       | 15 +++++++++++++++
>  softmmu/trace-events   |  1 +
>  5 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 44dacfa224..ab1ad0f7a8 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -834,8 +834,17 @@ static inline MemTxResult pci_dma_rw(PCIDevice *dev, 
> dma_addr_t addr,
>                                       void *buf, dma_addr_t len,
>                                       DMADirection dir, MemTxAttrs attrs)
>  {
> -    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
> -                         dir, attrs);
> +    bool prior_engaged_state;
> +    MemTxResult result;
> +
> +    prior_engaged_state = dev->qdev.engaged_in_io;
> +
> +    dev->qdev.engaged_in_io = true;
> +    result = dma_memory_rw(pci_get_address_space(dev), addr, buf, len,
> +                           dir, attrs);
> +    dev->qdev.engaged_in_io = prior_engaged_state;
> +
> +    return result;
>  }
>  
>  /**
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 92c3d65208..6474dc51fa 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -193,6 +193,9 @@ struct DeviceState {
>      int instance_id_alias;
>      int alias_required_for_version;
>      ResettableState reset;
> +
> +    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy 
> */
> +    int engaged_in_io;
>  };
>  
>  struct DeviceListener {
> diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
> index 7820fec54c..7a4f1fb9b3 100644
> --- a/softmmu/dma-helpers.c
> +++ b/softmmu/dma-helpers.c
> @@ -288,8 +288,16 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t len, 
> dma_addr_t *residual,
>      uint8_t *ptr = buf;
>      dma_addr_t xresidual;
>      int sg_cur_index;
> +    DeviceState *dev;
> +    bool prior_engaged_state;
>      MemTxResult res = MEMTX_OK;
>  
> +    dev = sg->dev;
> +    if (dev) {
> +        prior_engaged_state = dev->engaged_in_io;
> +        dev->engaged_in_io = true;
> +    }
> +
>      xresidual = sg->size;
>      sg_cur_index = 0;
>      len = MIN(len, xresidual);
> @@ -302,6 +310,10 @@ static MemTxResult dma_buf_rw(void *buf, dma_addr_t len, 
> dma_addr_t *residual,
>          xresidual -= xfer;
>      }
>  
> +    if (dev) {
> +        dev->engaged_in_io = prior_engaged_state;
> +    }
> +
>      if (residual) {
>          *residual = xresidual;
>      }
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 7ba2048836..44a14bb4f5 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -532,6 +532,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>      uint64_t access_mask;
>      unsigned access_size;
>      unsigned i;
> +    DeviceState *dev = NULL;
>      MemTxResult r = MEMTX_OK;
>  
>      if (!access_size_min) {
> @@ -541,6 +542,17 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>          access_size_max = 4;
>      }
>  
> +    /* Do not allow more than one simultanous access to a device's IO 
> Regions */
> +    if (mr->owner &&
> +        !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {

Would it make sense to define some helper function like
memory_region_is_XXX (I assume XXX -> DEVICE_IO), to make that code
easier to be consumed by humans?

Unfortunately I cannot really comment on the sanity of the approach,
because the underlying problem isn't completely clear to me (I think
other people on CC were involved in the discussions around DMA reentry
and failed attempts in the past). Having that said, that approach
doesn't look wrong to me.

-- 
Thanks,

David / dhildenb