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

[Thomas Huth <thuth@redhat.com>]

On 28/04/2023 11.11, Alexander Bulekov wrote:
> 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?

Not necessary, I've got v10 already queued, I'll fix it up there

  Thomas