Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues

[Alexander Bulekov]

On 230428 1015, Thomas Huth wrote:
> On 28/04/2023 10.12, Daniel P. Berrangé wrote:
> > 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 ?
> 
> Maybe it's better to use warn_report_once() here instead?

Sounds good - should I respin the series to change this?
-Alex