[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues |
Date: |
Fri, 28 Apr 2023 09:12:13 +0100 |
User-agent: |
Mutt/2.2.9 (2022-11-12) |
On Thu, Apr 27, 2023 at 05:10:06PM -0400, 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
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1282
> Resolves: CVE-2023-0330
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
> include/exec/memory.h | 5 +++++
> include/hw/qdev-core.h | 7 +++++++
> softmmu/memory.c | 16 ++++++++++++++++
> 3 files changed, 28 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 15ade918ba..e45ce6061f 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -767,6 +767,8 @@ struct MemoryRegion {
> bool is_iommu;
> RAMBlock *ram_block;
> Object *owner;
> + /* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access
> hotpath */
> + DeviceState *dev;
>
> const MemoryRegionOps *ops;
> void *opaque;
> @@ -791,6 +793,9 @@ struct MemoryRegion {
> unsigned ioeventfd_nb;
> MemoryRegionIoeventfd *ioeventfds;
> RamDiscardManager *rdm; /* Only for RAM */
> +
> + /* For devices designed to perform re-entrant IO into their own IO MRs */
> + bool disable_reentrancy_guard;
> };
>
> struct IOMMUMemoryRegion {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index bd50ad5ee1..7623703943 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -162,6 +162,10 @@ struct NamedClockList {
> QLIST_ENTRY(NamedClockList) node;
> };
>
> +typedef struct {
> + bool engaged_in_io;
> +} MemReentrancyGuard;
> +
> /**
> * DeviceState:
> * @realized: Indicates whether the device has been fully constructed.
> @@ -194,6 +198,9 @@ struct DeviceState {
> int alias_required_for_version;
> ResettableState reset;
> GSList *unplug_blockers;
> +
> + /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy
> */
> + MemReentrancyGuard mem_reentrancy_guard;
> };
>
> struct DeviceListener {
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index b1a6cae6f5..fe23f0e5ce 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -542,6 +542,18 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
> access_size_max = 4;
> }
>
> + /* Do not allow more than one simultaneous access to a device's IO
> Regions */
> + if (mr->dev && !mr->disable_reentrancy_guard &&
> + !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) {
> + if (mr->dev->mem_reentrancy_guard.engaged_in_io) {
> + warn_report("Blocked re-entrant IO on "
> + "MemoryRegion: %s at addr: 0x%" HWADDR_PRIX,
> + memory_region_name(mr), addr);
> + return MEMTX_ACCESS_ERROR;
If we issue this warn_report on every invalid memory access, is this
going to become a denial of service by flooding logs, or is the
return MEMTX_ACCESS_ERROR, sufficient to ensure this is only printed
*once* in the lifetime of the QEMU process ?
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
[PATCH v10 6/8] bcm2835_property: disable reentrancy detection for iomem, Alexander Bulekov, 2023/04/27
[PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API, Alexander Bulekov, 2023/04/27
[PATCH v10 7/8] raven: disable reentrancy detection for iomem, Alexander Bulekov, 2023/04/27
[PATCH v10 8/8] apic: disable reentrancy detection for apic-msi, Alexander Bulekov, 2023/04/27
[PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded, Alexander Bulekov, 2023/04/27