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

[Thomas Huth <thuth@redhat.com>]

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?

  Thomas