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

[Thomas Huth <thuth@redhat.com>]

On 06/05/2023 11.25, Song Gao wrote:
>   Hi Alexander
> 
> 在 2023/4/28 下午5:14, Thomas Huth 写道:
>> 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
>>
> This patch causes the loongarch virtual machine to fail to start the slave cpu.
> 
>      ./build/qemu-system-loongarch64 -machine virt -m 8G -cpu la464 \
>               -smp 4 -bios QEMU_EFI.fd -kernel vmlinuz.efi -initrd ramdisk   \
>                 -serial stdio   -monitor telnet:localhost:4495,server,nowait  \
>                 -append "root=/dev/ram rdinit=/sbin/init 
> console=ttyS0,115200"   --nographic
> 
> 
> ....
> qemu-system-loongarch64: warning: Blocked re-entrant IO on MemoryRegion: 
> loongarch_ipi_iocsr at addr: 0x24

Oh, another spot that needs special handling ... I see Alexander already 
sent a patch (thanks!), but anyway, this is a good indication that we're 
missing some test coverage in the CI.

Are there any loongarch kernel images available for public download 
somewhere? If so, we really should add an avocado regression test for this - 
since as far as I can see, we don't have any  tests for loongarch in 
tests/avocado yet?

  Thomas