* [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-28 6:09 ` Thomas Huth
` (2 more replies)
2023-04-27 21:10 ` [PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API Alexander Bulekov
` (8 subsequent siblings)
9 siblings, 3 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
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;
+ }
+ mr->dev->mem_reentrancy_guard.engaged_in_io = true;
+ }
+
/* FIXME: support unaligned access? */
access_size = MAX(MIN(size, access_size_max), access_size_min);
access_mask = MAKE_64BIT_MASK(0, access_size * 8);
@@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
access_mask, attrs);
}
}
+ if (mr->dev) {
+ mr->dev->mem_reentrancy_guard.engaged_in_io = false;
+ }
return r;
}
@@ -1170,6 +1185,7 @@ static void memory_region_do_init(MemoryRegion *mr,
}
mr->name = g_strdup(name);
mr->owner = owner;
+ mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
mr->ram_block = NULL;
if (name) {
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 ` [PATCH v10 1/8] " Alexander Bulekov
@ 2023-04-28 6:09 ` Thomas Huth
2023-04-28 8:12 ` Daniel P. Berrangé
2024-08-21 13:25 ` Igor Mammedov
2 siblings, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2023-04-28 6:09 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
Marcel Apfelbaum, Daniel P . Berrangé, Eduardo Habkost,
Jon Maloy, Siqi Chen, Michael Tokarev
On 27/04/2023 23.10, 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);
Ack, a warn_report make sense here, at least initially, to make sure that
people get aware of related problems!
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 ` [PATCH v10 1/8] " Alexander Bulekov
2023-04-28 6:09 ` Thomas Huth
@ 2023-04-28 8:12 ` Daniel P. Berrangé
2023-04-28 8:15 ` Thomas Huth
2024-08-21 13:25 ` Igor Mammedov
2 siblings, 1 reply; 33+ messages in thread
From: Daniel P. Berrangé @ 2023-04-28 8:12 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Eduardo Habkost, Jon Maloy,
Siqi Chen, Michael Tokarev
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 :|
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-28 8:12 ` Daniel P. Berrangé
@ 2023-04-28 8:15 ` Thomas Huth
2023-04-28 9:11 ` Alexander Bulekov
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2023-04-28 8:15 UTC (permalink / raw)
To: Daniel P. Berrangé, Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
Marcel Apfelbaum, Eduardo Habkost, Jon Maloy, Siqi Chen,
Michael Tokarev
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-28 8:15 ` Thomas Huth
@ 2023-04-28 9:11 ` Alexander Bulekov
2023-04-28 9:14 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-28 9:11 UTC (permalink / raw)
To: Thomas Huth
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-28 9:11 ` Alexander Bulekov
@ 2023-04-28 9:14 ` Thomas Huth
2023-05-06 9:25 ` Song Gao
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2023-04-28 9:14 UTC (permalink / raw)
To: Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-28 9:14 ` Thomas Huth
@ 2023-05-06 9:25 ` Song Gao
2023-05-08 9:33 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Song Gao @ 2023-05-06 9:25 UTC (permalink / raw)
To: Thomas Huth, Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Richard Henderson, maobibo, Tianrui Zhao, Peter Maydell
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
....
[ 0.059284] smp: Bringing up secondary CPUs ...
[ 0.062540] Booting CPU#1...
[ 5.204340] CPU1: failed to start
[ 5.211435] Booting CPU#2...
[ 10.465762] CPU2: failed to start
[ 10.467757] Booting CPU#3...
[ 15.805430] CPU3: failed to start
[ 15.805980] smp: Brought up 1 node, 1 CPU
[ 15.818832] devtmpfs: initialized
[ 15.830287] clocksource: jiffies: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 7645041785100000 ns
[ 15.830429] futex hash table entries: 1024 (order: 2, 65536 bytes,
linear)
[ 15.840470] NET: Registered PF_NETLINK/PF_ROUTE protocol family
[ 15.845424] audit: initializing netlink subsys (disabled)
...
gdb
(gdb) n
qemu-system-loongarch64: warning: Blocked re-entrant IO on MemoryRegion:
loongarch_ipi_iocsr at addr: 0x24
552 return MEMTX_ACCESS_ERROR;
(gdb) bt
#0 0x0000555555ae93aa in access_with_adjusted_size
(addr=36, value=0x7fffd299f988, size=4, access_size_min=<optimized
out>, access_size_max=<optimized out>, access_fn=
0x555555ae98f0 <memory_region_write_accessor>, mr=0x555556e3a740,
attrs=...) at ../softmmu/memory.c:552
#1 0x0000555555ae962c in memory_region_dispatch_write
(mr=0x555556e3a740, addr=36, data=<optimized out>, op=<optimized out>,
attrs=...)
at ../softmmu/memory.c:1531
#2 0x0000555555af5739 in address_space_stl_internal
(endian=DEVICE_NATIVE_ENDIAN, result=0x0, attrs=..., val=0,
addr=<optimized out>, as=0x5555567b4bb8)
at
/home3/gaosong/code/vec/github/send/v2/qemu/include/exec/memory.h:2997
#3 0x0000555555af5739 in address_space_stl (as=0x5555567b4bb8,
addr=<optimized out>, val=0, attrs=..., result=0x0)
at /home3/gaosong/code/vec/github/send/v2/qemu/memory_ldst.c.inc:350
#4 0x0000555555ae99e8 in memory_region_write_accessor
(mr=0x555556e3ab80, addr=0, value=<optimized out>, size=8,
shift=<optimized out>, mask=<optimized out>, attrs=...) at
../softmmu/memory.c:493
#5 0x0000555555ae92ae in access_with_adjusted_size
(addr=0, value=0x7fffd299fb88, size=8, access_size_min=<optimized
out>, access_size_max=<optimized out>, access_fn=
0x555555ae98f0 <memory_region_write_accessor>, mr=0x555556e3ab80,
attrs=...) at ../softmmu/memory.c:567
#6 0x0000555555ae962c in memory_region_dispatch_write
(mr=0x555556e3ab80, addr=0, data=<optimized out>, op=<optimized out>,
attrs=...) at ../softmmu/memory.c:1531
#7 0x0000555555af2591 in address_space_stq_internal (as=<optimized
out>, addr=<optimized out>, val=2147483652, attrs=..., result=0x0,
endian=DEVICE_NATIVE_ENDIAN)
at
/home3/gaosong/code/vec/github/send/v2/qemu/include/exec/memory.h:2997
#8 0x00007fff8d922923 in code_gen_buffer ()
#9 0x0000555555b4b9e0 in cpu_tb_exec (tb_exit=<synthetic pointer>,
itb=<optimized out>, cpu=0x5555567a5d00) at ../accel/tcg/cpu-exec.c:460
#10 0x0000555555b4b9e0 in cpu_loop_exec_tb (tb_exit=<synthetic pointer>,
last_tb=<synthetic pointer>, pc=<optimized out>, tb=<optimized out>,
cpu=0x5555567a5d00)
at ../accel/tcg/cpu-exec.c:893
#11 0x0000555555b4b9e0 in cpu_exec_loop (cpu=0x5555567a5d00,
sc=<optimized out>) at ../accel/tcg/cpu-exec.c:1013
#12 0x0000555555b4c67d in cpu_exec_setjmp (cpu=0x5555567a5d00,
sc=0x7fffd29a0220) at ../accel/tcg/cpu-exec.c:1043
#13 0x0000555555b4c746 in cpu_exec (cpu=0x5555567a5d00) at
../accel/tcg/cpu-exec.c:1069
#14 0x0000555555b634bf in tcg_cpus_exec (cpu=0x5555567a5d00) at
../accel/tcg/tcg-accel-ops.c:81
#15 0x0000555555b63613 in mttcg_cpu_thread_fn (arg=0x5555567a5d00) at
../accel/tcg/tcg-accel-ops-mttcg.c:95
#16 0x0000555555cdefba in qemu_thread_start (args=<optimized out>) at
../util/qemu-thread-posix.c:541
#17 0x00007fffefcb717a in start_thread () at /lib64/libpthread.so.0
#18 0x00007fffef9e6dc3 in clone () at /lib64/libc.so.6
About LoongArch:
- docs/system/loongarch/virt.rst
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-06 9:25 ` Song Gao
@ 2023-05-08 9:33 ` Thomas Huth
2023-05-08 13:03 ` Song Gao
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2023-05-08 9:33 UTC (permalink / raw)
To: Song Gao, Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Richard Henderson, maobibo, Tianrui Zhao, Peter Maydell
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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-08 9:33 ` Thomas Huth
@ 2023-05-08 13:03 ` Song Gao
2023-05-08 13:12 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Song Gao @ 2023-05-08 13:03 UTC (permalink / raw)
To: Thomas Huth, Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Richard Henderson, maobibo, Tianrui Zhao, Peter Maydell
Hi, Thomas
在 2023/5/8 下午5:33, Thomas Huth 写道:
> 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?
>
we can get some binarys at:
https://github.com/yangxiaojuan-loongson/qemu-binary
I'm not sure that avacodo testing can be done using just the kernel.
Is a full loongarch system required?
if yes, unfortunately, there is not yet a well-developed loongarch
system.
And we are moving forward with loongarch support for debian systems.
There are also systems that already support loongarch, e.g archlinux.
https://mirrors.wsyu.edu.cn/loongarch/archlinux/images/
But I briefly tested that it doesn't work in the latest qemu. I've
already pushed the maintainer to update the system.
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-08 13:03 ` Song Gao
@ 2023-05-08 13:12 ` Thomas Huth
2023-05-09 2:13 ` Song Gao
2023-05-10 9:02 ` Song Gao
0 siblings, 2 replies; 33+ messages in thread
From: Thomas Huth @ 2023-05-08 13:12 UTC (permalink / raw)
To: Song Gao, Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Richard Henderson, maobibo, Tianrui Zhao, Peter Maydell
On 08/05/2023 15.03, Song Gao wrote:
> Hi, Thomas
>
> 在 2023/5/8 下午5:33, Thomas Huth 写道:
>> 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 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?
>>
> we can get some binarys at:
> https://github.com/yangxiaojuan-loongson/qemu-binary
>
> I'm not sure that avacodo testing can be done using just the kernel.
>
> Is a full loongarch system required?
No, you don't need a full distro installation, just a kernel with ramdisk
(which is also available there) is good enough for a basic test, e.g. just
check whether the kernel boots to a certain point is good enough to provide
a basic sanity test. If you then can also get even into a shell (of the
ramdisk), you can check some additional stuff in the sysfs or "dmesg"
output, see for example tests/avocado/machine_s390_ccw_virtio.py which does
such checks with a kernel and initrd on s390x.
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-08 13:12 ` Thomas Huth
@ 2023-05-09 2:13 ` Song Gao
2023-05-10 9:02 ` Song Gao
1 sibling, 0 replies; 33+ messages in thread
From: Song Gao @ 2023-05-09 2:13 UTC (permalink / raw)
To: Thomas Huth, Alexander Bulekov
Cc: Daniel P. Berrangé, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Gerd Hoffmann, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Richard Henderson, maobibo, Tianrui Zhao, Peter Maydell
在 2023/5/8 下午9:12, Thomas Huth 写道:
> On 08/05/2023 15.03, Song Gao wrote:
>> Hi, Thomas
>>
>> 在 2023/5/8 下午5:33, Thomas Huth 写道:
>>> 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 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?
>>>
>> we can get some binarys at:
>> https://github.com/yangxiaojuan-loongson/qemu-binary
> >
>> I'm not sure that avacodo testing can be done using just the kernel.
>>
>> Is a full loongarch system required?
>
> No, you don't need a full distro installation, just a kernel with
> ramdisk (which is also available there) is good enough for a basic
> test, e.g. just check whether the kernel boots to a certain point is
> good enough to provide a basic sanity test. If you then can also get
> even into a shell (of the ramdisk), you can check some additional
> stuff in the sysfs or "dmesg" output, see for example
> tests/avocado/machine_s390_ccw_virtio.py which does such checks with a
> kernel and initrd on s390x.
>
Thanks for you suggestion .
We will add a loongarch basic test on tests/avacode.
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-08 13:12 ` Thomas Huth
2023-05-09 2:13 ` Song Gao
@ 2023-05-10 9:02 ` Song Gao
2023-05-10 12:21 ` Thomas Huth
1 sibling, 1 reply; 33+ messages in thread
From: Song Gao @ 2023-05-10 9:02 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, maobibo
Hi, Thomas
在 2023/5/8 下午9:12, Thomas Huth 写道:
>
>>> 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?
>>>
>> we can get some binarys at:
>> https://github.com/yangxiaojuan-loongson/qemu-binary
> >
>> I'm not sure that avacodo testing can be done using just the kernel.
>>
>> Is a full loongarch system required?
>
> No, you don't need a full distro installation, just a kernel with
> ramdisk (which is also available there) is good enough for a basic
> test, e.g. just check whether the kernel boots to a certain point is
> good enough to provide a basic sanity test. If you then can also get
> even into a shell (of the ramdisk), you can check some additional
> stuff in the sysfs or "dmesg" output, see for example
> tests/avocado/machine_s390_ccw_virtio.py which does such checks with a
> kernel and initrd on s390x.
>
>
I have a few questions.
I run ' make check-avocado
AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1'
root@loongson-KVM:~/work/qemu#make check-avocado
AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1
changing dir to build for make "check-avocado"...
make[1]: Entering directory '/root/work/qemu/build'
(GIT="git" "/root/work/qemu/scripts/git-submodule.sh" update
ui/keycodemapdb meson tests/fp/berkeley-testfloat-3
tests/fp/berkeley-softfloat-3 dtc)
/root/work/qemu/build/tests/venv/bin/python3 -m avocado vmimage get
--distro=fedora --distro-version=31 --arch=s390x
The image was downloaded:
Provider Version Architecture File
fedora 31 s390x
/root/avocado/data/cache/by_location/8ee06cba5485a58b2203c2c000d6d2ff6da0f040/Fedora-Cloud-Base-31-1.9.s390x.qcow2
/root/work/qemu/build/tests/venv/bin/python3 -m avocado --show=app run
--job-results-dir=/root/work/qemu/build/tests/results
--filter-by-tags-include-empty --filter-by-tags-include-empty-key
--max-parallel-tasks 1 -t arch:loongarch64 -t arch:s390x --failfast
./tests/avocado/machine_s390_ccw_virtio.py
...
This test downloaded 'Fedora-Cloud-Base-31-1.9.s390x.qcow2' image.
but we don't have a 'Fedora-Cloud-Base-31-1.9.loongarch.qcow2' image.
Am I missing something?
One more question, How to get the 'kernel_hash' and 'initrd_hash'?
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-10 9:02 ` Song Gao
@ 2023-05-10 12:21 ` Thomas Huth
2023-05-11 8:53 ` Song Gao
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2023-05-10 12:21 UTC (permalink / raw)
To: Song Gao; +Cc: qemu-devel, maobibo
On 10/05/2023 11.02, Song Gao wrote:
> Hi, Thomas
>
> 在 2023/5/8 下午9:12, Thomas Huth 写道:
>>
>>>> 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?
>>>>
>>> we can get some binarys at:
>>> https://github.com/yangxiaojuan-loongson/qemu-binary
>> >
>>> I'm not sure that avacodo testing can be done using just the kernel.
>>>
>>> Is a full loongarch system required?
>>
>> No, you don't need a full distro installation, just a kernel with ramdisk
>> (which is also available there) is good enough for a basic test, e.g. just
>> check whether the kernel boots to a certain point is good enough to
>> provide a basic sanity test. If you then can also get even into a shell
>> (of the ramdisk), you can check some additional stuff in the sysfs or
>> "dmesg" output, see for example tests/avocado/machine_s390_ccw_virtio.py
>> which does such checks with a kernel and initrd on s390x.
>>
>>
> I have a few questions.
>
> I run ' make check-avocado
> AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1'
>
> root@loongson-KVM:~/work/qemu#make check-avocado
> AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1
> changing dir to build for make "check-avocado"...
> make[1]: Entering directory '/root/work/qemu/build'
> (GIT="git" "/root/work/qemu/scripts/git-submodule.sh" update ui/keycodemapdb
> meson tests/fp/berkeley-testfloat-3 tests/fp/berkeley-softfloat-3 dtc)
> /root/work/qemu/build/tests/venv/bin/python3 -m avocado vmimage get
> --distro=fedora --distro-version=31 --arch=s390x
> The image was downloaded:
> Provider Version Architecture File
> fedora 31 s390x
> /root/avocado/data/cache/by_location/8ee06cba5485a58b2203c2c000d6d2ff6da0f040/Fedora-Cloud-Base-31-1.9.s390x.qcow2
> /root/work/qemu/build/tests/venv/bin/python3 -m avocado --show=app run
> --job-results-dir=/root/work/qemu/build/tests/results
> --filter-by-tags-include-empty --filter-by-tags-include-empty-key
> --max-parallel-tasks 1 -t arch:loongarch64 -t arch:s390x --failfast
> ./tests/avocado/machine_s390_ccw_virtio.py
> ...
>
> This test downloaded 'Fedora-Cloud-Base-31-1.9.s390x.qcow2' image.
> but we don't have a 'Fedora-Cloud-Base-31-1.9.loongarch.qcow2' image.
>
> Am I missing something?
Hmm, that image is not required for those tests... not sure why they get
downloaded here... I think something in
tests/avocado/avocado_qemu/__init__.py or in tests/Makefile.include tries to
download the cloudinit stuff in advance for other tests, but it is certainly
unrelated to the machine_s390_ccw_virtio.py test that only uses a kernel and
initrd.
I think you can ignore that (unless there is an error since it's trying to
download the loongarch Cloud-Base image - then that's a bug).
> One more question, How to get the 'kernel_hash' and 'initrd_hash'?
I think it's a SHA1 hash by default, so you can for example get it with the
"sha1sum" tool on the command line.
But seems like it is also possible to specify different algorithms with the
"algorithm=..." parameter of fetch_asset().
HTH,
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-10 12:21 ` Thomas Huth
@ 2023-05-11 8:53 ` Song Gao
2023-05-11 8:58 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Song Gao @ 2023-05-11 8:53 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, maobibo
在 2023/5/10 下午8:21, Thomas Huth 写道:
> On 10/05/2023 11.02, Song Gao wrote:
>> Hi, Thomas
>>
>> 在 2023/5/8 下午9:12, Thomas Huth 写道:
>>>
>>>>> 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?
>>>>>
>>>> we can get some binarys at:
>>>> https://github.com/yangxiaojuan-loongson/qemu-binary
>>> >
>>>> I'm not sure that avacodo testing can be done using just the kernel.
>>>>
>>>> Is a full loongarch system required?
>>>
>>> No, you don't need a full distro installation, just a kernel with
>>> ramdisk (which is also available there) is good enough for a basic
>>> test, e.g. just check whether the kernel boots to a certain point is
>>> good enough to provide a basic sanity test. If you then can also get
>>> even into a shell (of the ramdisk), you can check some additional
>>> stuff in the sysfs or "dmesg" output, see for example
>>> tests/avocado/machine_s390_ccw_virtio.py which does such checks with
>>> a kernel and initrd on s390x.
>>>
>>>
>> I have a few questions.
>>
>> I run ' make check-avocado
>> AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1'
>>
>> root@loongson-KVM:~/work/qemu#make check-avocado
>> AVOCADO_TESTS=./tests/avocado/machine_s390_ccw_virtio.py V=1
>> changing dir to build for make "check-avocado"...
>> make[1]: Entering directory '/root/work/qemu/build'
>> (GIT="git" "/root/work/qemu/scripts/git-submodule.sh" update
>> ui/keycodemapdb meson tests/fp/berkeley-testfloat-3
>> tests/fp/berkeley-softfloat-3 dtc)
>> /root/work/qemu/build/tests/venv/bin/python3 -m avocado vmimage get
>> --distro=fedora --distro-version=31 --arch=s390x
>> The image was downloaded:
>> Provider Version Architecture File
>> fedora 31 s390x
>> /root/avocado/data/cache/by_location/8ee06cba5485a58b2203c2c000d6d2ff6da0f040/Fedora-Cloud-Base-31-1.9.s390x.qcow2
>> /root/work/qemu/build/tests/venv/bin/python3 -m avocado --show=app
>> run --job-results-dir=/root/work/qemu/build/tests/results
>> --filter-by-tags-include-empty --filter-by-tags-include-empty-key
>> --max-parallel-tasks 1 -t arch:loongarch64 -t arch:s390x --failfast
>> ./tests/avocado/machine_s390_ccw_virtio.py
>> ...
>>
>> This test downloaded 'Fedora-Cloud-Base-31-1.9.s390x.qcow2' image.
>> but we don't have a 'Fedora-Cloud-Base-31-1.9.loongarch.qcow2' image.
>>
>> Am I missing something?
>
> Hmm, that image is not required for those tests... not sure why they
> get downloaded here... I think something in
> tests/avocado/avocado_qemu/__init__.py or in tests/Makefile.include
> tries to download the cloudinit stuff in advance for other tests, but
> it is certainly unrelated to the machine_s390_ccw_virtio.py test that
> only uses a kernel and initrd.
>
> I think you can ignore that (unless there is an error since it's
> trying to download the loongarch Cloud-Base image - then that's a bug).
>
Yes, we can ignore, no error.
>> One more question, How to get the 'kernel_hash' and 'initrd_hash'?
>
> I think it's a SHA1 hash by default, so you can for example get it
> with the "sha1sum" tool on the command line.
>
> But seems like it is also possible to specify different algorithms
> with the "algorithm=..." parameter of fetch_asset().
>
Thanks for you help.
And
Should we need add ' @skipIf(os.getenv('GITLAB_CI'), 'Running on
GitLab')' ?
I see some tests add this.
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-11 8:53 ` Song Gao
@ 2023-05-11 8:58 ` Thomas Huth
2023-05-11 9:08 ` Song Gao
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2023-05-11 8:58 UTC (permalink / raw)
To: Song Gao; +Cc: qemu-devel, maobibo
On 11/05/2023 10.53, Song Gao wrote:
...
> And
> Should we need add ' @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')' ?
>
> I see some tests add this.
No, please don't add that unless there is a good reason. That marker is only
required if the test does not work reliable on gitlab, e.g. if it sometimes
fails due to race conditions or if it takes incredibly long to finish.
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-05-11 8:58 ` Thomas Huth
@ 2023-05-11 9:08 ` Song Gao
0 siblings, 0 replies; 33+ messages in thread
From: Song Gao @ 2023-05-11 9:08 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, maobibo
在 2023/5/11 下午4:58, Thomas Huth 写道:
> On 11/05/2023 10.53, Song Gao wrote:
> ...
>> And
>> Should we need add ' @skipIf(os.getenv('GITLAB_CI'), 'Running on
>> GitLab')' ?
>>
>> I see some tests add this.
>
> No, please don't add that unless there is a good reason. That marker
> is only required if the test does not work reliable on gitlab, e.g. if
> it sometimes fails due to race conditions or if it takes incredibly
> long to finish.
>
Ok.
Thanks.
Song Gao
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 ` [PATCH v10 1/8] " Alexander Bulekov
2023-04-28 6:09 ` Thomas Huth
2023-04-28 8:12 ` Daniel P. Berrangé
@ 2024-08-21 13:25 ` Igor Mammedov
2024-08-27 15:49 ` Igor Mammedov
2 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2024-08-21 13:25 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
On Thu, 27 Apr 2023 17:10:06 -0400
Alexander Bulekov <alxndr@bu.edu> 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
Alexander, with 9.0
we are getting
warning: Blocked re-entrant IO on MemoryRegion: acpi-cpu-hotplug at addr: 0x0
during CPU hot-unplug, to my knowledge there shouldn't be any DMA involved
there.
The only access should be either from guest kernel or firmware(this one is under SMM mode)).
Question is how this could happen on MMIO access which should be guarded by BQL?
And where to start digging to find out if it's a genuine issue,
or whether it's safe to use big hammer and disable reentrancy guard?
> 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;
> + }
> + mr->dev->mem_reentrancy_guard.engaged_in_io = true;
> + }
> +
> /* FIXME: support unaligned access? */
> access_size = MAX(MIN(size, access_size_max), access_size_min);
> access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> @@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
> access_mask, attrs);
> }
> }
> + if (mr->dev) {
> + mr->dev->mem_reentrancy_guard.engaged_in_io = false;
> + }
> return r;
> }
>
> @@ -1170,6 +1185,7 @@ static void memory_region_do_init(MemoryRegion *mr,
> }
> mr->name = g_strdup(name);
> mr->owner = owner;
> + mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
> mr->ram_block = NULL;
>
> if (name) {
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2024-08-21 13:25 ` Igor Mammedov
@ 2024-08-27 15:49 ` Igor Mammedov
2024-08-28 7:55 ` Gerd Hoffmann
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2024-08-27 15:49 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Gerd Hoffmann
On Wed, 21 Aug 2024 15:25:18 +0200
Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 27 Apr 2023 17:10:06 -0400
> Alexander Bulekov <alxndr@bu.edu> 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
>
> Alexander, with 9.0
> we are getting
>
> warning: Blocked re-entrant IO on MemoryRegion: acpi-cpu-hotplug at addr: 0x0
>
> during CPU hot-unplug, to my knowledge there shouldn't be any DMA involved
> there.
> The only access should be either from guest kernel or firmware(this one is under SMM mode)).
>
> Question is how this could happen on MMIO access which should be guarded by BQL?
For prosperity, reproducer (RHEL9.4 Haswell host + upstream QEMU/edk https://issues.redhat.com/browse/RHEL-56154):
./qemu-system-x86_64 --enable-kvm -smp 2,maxcpus=24,cores=12,threads=1,dies=1,sockets=2 \
-cpu host \
-blockdev '{"node-name": "file_ovmf_code", "driver": "file", "filename": "/tmp/qemu_build/pc-bios/edk2-x86_64-secure-code.fd", "auto-read-only": true, "discard": "unmap"}' \
-blockdev '{"node-name": "drive_ovmf_code", "driver": "raw", "read-only": true, "file": "file_ovmf_code"}' \
-blockdev '{"node-name": "file_ovmf_vars", "driver": "file", "filename": "/tmp/edk_VARS.raw", "auto-read-only": true, "discard": "unmap"}' \
-blockdev '{"node-name": "drive_ovmf_vars", "driver": "raw", "read-only": false, "file": "file_ovmf_vars"}' \
-M q35,pflash0=drive_ovmf_code,pflash1=drive_ovmf_vars \
-m 4G \
-monitor stdio \
rhel9.5.raw
once booted
(qemu) device_add host-x86_64-cpu,id=vcpu1,socket-id=1,core-id=10,die-id=0,thread-id=0
(qemu) device_add host-x86_64-cpu,id=vcpu2,socket-id=1,core-id=11,die-id=0,thread-id=0
(qemu) device_del vcpu1
(qemu) qemu-system-x86_64: warning: Blocked re-entrant IO on MemoryRegion: acpi-cpu-hotplug at addr: 0x0
> And where to start digging to find out if it's a genuine issue,
> or whether it's safe to use big hammer and disable reentrancy guard?
I'm hesitant to use hammer so far (though it would make problem nop).
What happens is that cpu_remove_sync() temporarily releases BQL
which lets conflicting access to happen.
But I think unexpected access shouldn't be there in the 1st place,
so guard looks pretty legit at this point.
Lets see what Gerd finds out from edk2 point of view.
>
>
> > 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;
> > + }
> > + mr->dev->mem_reentrancy_guard.engaged_in_io = true;
> > + }
> > +
> > /* FIXME: support unaligned access? */
> > access_size = MAX(MIN(size, access_size_max), access_size_min);
> > access_mask = MAKE_64BIT_MASK(0, access_size * 8);
> > @@ -556,6 +568,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
> > access_mask, attrs);
> > }
> > }
> > + if (mr->dev) {
> > + mr->dev->mem_reentrancy_guard.engaged_in_io = false;
> > + }
> > return r;
> > }
> >
> > @@ -1170,6 +1185,7 @@ static void memory_region_do_init(MemoryRegion *mr,
> > }
> > mr->name = g_strdup(name);
> > mr->owner = owner;
> > + mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
> > mr->ram_block = NULL;
> >
> > if (name) {
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 1/8] memory: prevent dma-reentracy issues
2024-08-27 15:49 ` Igor Mammedov
@ 2024-08-28 7:55 ` Gerd Hoffmann
0 siblings, 0 replies; 33+ messages in thread
From: Gerd Hoffmann @ 2024-08-28 7:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alexander Bulekov, qemu-devel, Stefan Hajnoczi,
Philippe Mathieu-Daudé, Mauro Matteo Cascella, Peter Xu,
Jason Wang, David Hildenbrand, Thomas Huth, Laurent Vivier,
Bandan Das, Edgar E . Iglesias, Darren Kenny, Bin Meng,
Paolo Bonzini, Michael S . Tsirkin, Marcel Apfelbaum,
Daniel P . Berrangé, Eduardo Habkost, Jon Maloy, Siqi Chen,
Michael Tokarev
Hi,
> But I think unexpected access shouldn't be there in the 1st place,
> so guard looks pretty legit at this point.
> Lets see what Gerd finds out from edk2 point of view.
CPU eject happens /after/ SMM syncronisation, when CPUs are on their way
back into normal mode:
* The boot processor will do the cpu hotplug register writes, from SMM
mode, so it obviously will be in SMM mode still.
* The processor to be unplugged will be parked in a halt loop in SMM
mode until the unplug completed, so that processor will be in SMM
mode too.
* All other processors may or may not be in SMM mode.
So parallel access is possible.
take care,
Gerd
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 1/8] " Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks Alexander Bulekov
` (7 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev, Fam Zheng,
Kevin Wolf, Hanna Reitz, open list:Block I/O path
Devices can pass their MemoryReentrancyGuard (from their DeviceState),
when creating new BHes. Then, the async API will toggle the guard
before/after calling the BH call-back. This prevents bh->mmio reentrancy
issues.
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
docs/devel/multiple-iothreads.txt | 7 +++++++
include/block/aio.h | 18 ++++++++++++++++--
include/qemu/main-loop.h | 7 +++++--
tests/unit/ptimer-test-stubs.c | 3 ++-
util/async.c | 18 +++++++++++++++++-
util/main-loop.c | 5 +++--
util/trace-events | 1 +
7 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt
index 343120f2ef..a3e949f6b3 100644
--- a/docs/devel/multiple-iothreads.txt
+++ b/docs/devel/multiple-iothreads.txt
@@ -61,6 +61,7 @@ There are several old APIs that use the main loop AioContext:
* LEGACY qemu_aio_set_event_notifier() - monitor an event notifier
* LEGACY timer_new_ms() - create a timer
* LEGACY qemu_bh_new() - create a BH
+ * LEGACY qemu_bh_new_guarded() - create a BH with a device re-entrancy guard
* LEGACY qemu_aio_wait() - run an event loop iteration
Since they implicitly work on the main loop they cannot be used in code that
@@ -72,8 +73,14 @@ Instead, use the AioContext functions directly (see include/block/aio.h):
* aio_set_event_notifier() - monitor an event notifier
* aio_timer_new() - create a timer
* aio_bh_new() - create a BH
+ * aio_bh_new_guarded() - create a BH with a device re-entrancy guard
* aio_poll() - run an event loop iteration
+The qemu_bh_new_guarded/aio_bh_new_guarded APIs accept a "MemReentrancyGuard"
+argument, which is used to check for and prevent re-entrancy problems. For
+BHs associated with devices, the reentrancy-guard is contained in the
+corresponding DeviceState and named "mem_reentrancy_guard".
+
The AioContext can be obtained from the IOThread using
iothread_get_aio_context() or for the main loop using qemu_get_aio_context().
Code that takes an AioContext argument works both in IOThreads or the main
diff --git a/include/block/aio.h b/include/block/aio.h
index e267d918fd..89bbc536f9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -23,6 +23,8 @@
#include "qemu/thread.h"
#include "qemu/timer.h"
#include "block/graph-lock.h"
+#include "hw/qdev-core.h"
+
typedef struct BlockAIOCB BlockAIOCB;
typedef void BlockCompletionFunc(void *opaque, int ret);
@@ -323,9 +325,11 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
* is opaque and must be allocated prior to its use.
*
* @name: A human-readable identifier for debugging purposes.
+ * @reentrancy_guard: A guard set when entering a cb to prevent
+ * device-reentrancy issues
*/
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
- const char *name);
+ const char *name, MemReentrancyGuard *reentrancy_guard);
/**
* aio_bh_new: Allocate a new bottom half structure
@@ -334,7 +338,17 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
* string.
*/
#define aio_bh_new(ctx, cb, opaque) \
- aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)))
+ aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), NULL)
+
+/**
+ * aio_bh_new_guarded: Allocate a new bottom half structure with a
+ * reentrancy_guard
+ *
+ * A convenience wrapper for aio_bh_new_full() that uses the cb as the name
+ * string.
+ */
+#define aio_bh_new_guarded(ctx, cb, opaque, guard) \
+ aio_bh_new_full((ctx), (cb), (opaque), (stringify(cb)), guard)
/**
* aio_notify: Force processing of pending events.
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index b3e54e00bc..68e70e61aa 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -387,9 +387,12 @@ void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
/* internal interfaces */
+#define qemu_bh_new_guarded(cb, opaque, guard) \
+ qemu_bh_new_full((cb), (opaque), (stringify(cb)), guard)
#define qemu_bh_new(cb, opaque) \
- qemu_bh_new_full((cb), (opaque), (stringify(cb)))
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name);
+ qemu_bh_new_full((cb), (opaque), (stringify(cb)), NULL)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard);
void qemu_bh_schedule_idle(QEMUBH *bh);
enum {
diff --git a/tests/unit/ptimer-test-stubs.c b/tests/unit/ptimer-test-stubs.c
index f2bfcede93..8c9407c560 100644
--- a/tests/unit/ptimer-test-stubs.c
+++ b/tests/unit/ptimer-test-stubs.c
@@ -107,7 +107,8 @@ int64_t qemu_clock_deadline_ns_all(QEMUClockType type, int attr_mask)
return deadline;
}
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name,
+ MemReentrancyGuard *reentrancy_guard)
{
QEMUBH *bh = g_new(QEMUBH, 1);
diff --git a/util/async.c b/util/async.c
index 21016a1ac7..a9b528c370 100644
--- a/util/async.c
+++ b/util/async.c
@@ -65,6 +65,7 @@ struct QEMUBH {
void *opaque;
QSLIST_ENTRY(QEMUBH) next;
unsigned flags;
+ MemReentrancyGuard *reentrancy_guard;
};
/* Called concurrently from any thread */
@@ -137,7 +138,7 @@ void aio_bh_schedule_oneshot_full(AioContext *ctx, QEMUBHFunc *cb,
}
QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
- const char *name)
+ const char *name, MemReentrancyGuard *reentrancy_guard)
{
QEMUBH *bh;
bh = g_new(QEMUBH, 1);
@@ -146,13 +147,28 @@ QEMUBH *aio_bh_new_full(AioContext *ctx, QEMUBHFunc *cb, void *opaque,
.cb = cb,
.opaque = opaque,
.name = name,
+ .reentrancy_guard = reentrancy_guard,
};
return bh;
}
void aio_bh_call(QEMUBH *bh)
{
+ bool last_engaged_in_io = false;
+
+ if (bh->reentrancy_guard) {
+ last_engaged_in_io = bh->reentrancy_guard->engaged_in_io;
+ if (bh->reentrancy_guard->engaged_in_io) {
+ trace_reentrant_aio(bh->ctx, bh->name);
+ }
+ bh->reentrancy_guard->engaged_in_io = true;
+ }
+
bh->cb(bh->opaque);
+
+ if (bh->reentrancy_guard) {
+ bh->reentrancy_guard->engaged_in_io = last_engaged_in_io;
+ }
}
/* Multiple occurrences of aio_bh_poll cannot be called concurrently. */
diff --git a/util/main-loop.c b/util/main-loop.c
index e180c85145..4d76261010 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -605,9 +605,10 @@ void main_loop_wait(int nonblocking)
/* Functions to operate on the main QEMU AioContext. */
-QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name)
+QEMUBH *qemu_bh_new_full(QEMUBHFunc *cb, void *opaque, const char *name, MemReentrancyGuard *reentrancy_guard)
{
- return aio_bh_new_full(qemu_aio_context, cb, opaque, name);
+ return aio_bh_new_full(qemu_aio_context, cb, opaque, name,
+ reentrancy_guard);
}
/*
diff --git a/util/trace-events b/util/trace-events
index 16f78d8fe5..3f7e766683 100644
--- a/util/trace-events
+++ b/util/trace-events
@@ -11,6 +11,7 @@ poll_remove(void *ctx, void *node, int fd) "ctx %p node %p fd %d"
# async.c
aio_co_schedule(void *ctx, void *co) "ctx %p co %p"
aio_co_schedule_bh_cb(void *ctx, void *co) "ctx %p co %p"
+reentrant_aio(void *ctx, const char *name) "ctx %p name %s"
# thread-pool.c
thread_pool_submit(void *pool, void *req, void *opaque) "pool %p req %p opaque %p"
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v10 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 1/8] " Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 2/8] async: Add an optional reentrancy guard to the BH API Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded Alexander Bulekov
` (6 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
Advise authors to use the _guarded versions of the APIs, instead.
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
scripts/checkpatch.pl | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d768171dcf..eeaec436eb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2865,6 +2865,14 @@ sub process {
if ($line =~ /\bsignal\s*\(/ && !($line =~ /SIG_(?:IGN|DFL)/)) {
ERROR("use sigaction to establish signal handlers; signal is not portable\n" . $herecurr);
}
+# recommend qemu_bh_new_guarded instead of qemu_bh_new
+ if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\bqemu_bh_new\s*\(/) {
+ ERROR("use qemu_bh_new_guarded() instead of qemu_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
+# recommend aio_bh_new_guarded instead of aio_bh_new
+ if ($realfile =~ /.*\/hw\/.*/ && $line =~ /\baio_bh_new\s*\(/) {
+ ERROR("use aio_bh_new_guarded() instead of aio_bh_new() to avoid reentrancy problems\n" . $herecurr);
+ }
# check for module_init(), use category-specific init macros explicitly please
if ($line =~ /^module_init\s*\(/) {
ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (2 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 3/8] checkpatch: add qemu_bh_new/aio_bh_new checks Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 5/8] lsi53c895a: disable reentrancy detection for script RAM Alexander Bulekov
` (5 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Paul Durrant, Stefano Stabellini, Anthony Perard, Kevin Wolf,
Hanna Reitz, Amit Shah, Marc-André Lureau, John Snow,
Peter Maydell, Mark Cave-Ayland, Keith Busch, Klaus Jensen,
Fam Zheng, Dmitry Fleytman, Gonglei (Arei),
open list:X86 Xen CPUs, open list:virtio-blk,
open list:i.MX31 (kzm), open list:New World (mac99)
This protects devices from bh->mmio reentrancy issues.
Thanks: Thomas Huth <thuth@redhat.com> for diagnosing OS X test failure.
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
hw/9pfs/xen-9p-backend.c | 5 ++++-
hw/block/dataplane/virtio-blk.c | 3 ++-
hw/block/dataplane/xen-block.c | 5 +++--
hw/char/virtio-serial-bus.c | 3 ++-
hw/display/qxl.c | 9 ++++++---
hw/display/virtio-gpu.c | 6 ++++--
hw/ide/ahci.c | 3 ++-
hw/ide/ahci_internal.h | 1 +
hw/ide/core.c | 4 +++-
hw/misc/imx_rngc.c | 6 ++++--
hw/misc/macio/mac_dbdma.c | 2 +-
hw/net/virtio-net.c | 3 ++-
hw/nvme/ctrl.c | 6 ++++--
hw/scsi/mptsas.c | 3 ++-
hw/scsi/scsi-bus.c | 3 ++-
hw/scsi/vmw_pvscsi.c | 3 ++-
hw/usb/dev-uas.c | 3 ++-
hw/usb/hcd-dwc2.c | 3 ++-
hw/usb/hcd-ehci.c | 3 ++-
hw/usb/hcd-uhci.c | 2 +-
hw/usb/host-libusb.c | 6 ++++--
hw/usb/redirect.c | 6 ++++--
hw/usb/xen-usb.c | 3 ++-
hw/virtio/virtio-balloon.c | 5 +++--
hw/virtio/virtio-crypto.c | 3 ++-
25 files changed, 66 insertions(+), 33 deletions(-)
diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c
index 74f3a05f88..0e266c552b 100644
--- a/hw/9pfs/xen-9p-backend.c
+++ b/hw/9pfs/xen-9p-backend.c
@@ -61,6 +61,7 @@ typedef struct Xen9pfsDev {
int num_rings;
Xen9pfsRing *rings;
+ MemReentrancyGuard mem_reentrancy_guard;
} Xen9pfsDev;
static void xen_9pfs_disconnect(struct XenLegacyDevice *xendev);
@@ -443,7 +444,9 @@ static int xen_9pfs_connect(struct XenLegacyDevice *xendev)
xen_9pdev->rings[i].ring.out = xen_9pdev->rings[i].data +
XEN_FLEX_RING_SIZE(ring_order);
- xen_9pdev->rings[i].bh = qemu_bh_new(xen_9pfs_bh, &xen_9pdev->rings[i]);
+ xen_9pdev->rings[i].bh = qemu_bh_new_guarded(xen_9pfs_bh,
+ &xen_9pdev->rings[i],
+ &xen_9pdev->mem_reentrancy_guard);
xen_9pdev->rings[i].out_cons = 0;
xen_9pdev->rings[i].out_size = 0;
xen_9pdev->rings[i].inprogress = false;
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b28d81737e..a6202997ee 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -127,7 +127,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
} else {
s->ctx = qemu_get_aio_context();
}
- s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
+ s->bh = aio_bh_new_guarded(s->ctx, notify_guest_bh, s,
+ &DEVICE(vdev)->mem_reentrancy_guard);
s->batch_notify_vqs = bitmap_new(conf->num_queues);
*dataplane = s;
diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index 734da42ea7..d8bc39d359 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -633,8 +633,9 @@ XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev,
} else {
dataplane->ctx = qemu_get_aio_context();
}
- dataplane->bh = aio_bh_new(dataplane->ctx, xen_block_dataplane_bh,
- dataplane);
+ dataplane->bh = aio_bh_new_guarded(dataplane->ctx, xen_block_dataplane_bh,
+ dataplane,
+ &DEVICE(xendev)->mem_reentrancy_guard);
return dataplane;
}
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 7d4601cb5d..dd619f0731 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -985,7 +985,8 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
return;
}
- port->bh = qemu_bh_new(flush_queued_data_bh, port);
+ port->bh = qemu_bh_new_guarded(flush_queued_data_bh, port,
+ &dev->mem_reentrancy_guard);
port->elem = NULL;
}
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 80ce1e9a93..f1c0eb7dfc 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2201,11 +2201,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
- qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
+ qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
qxl_reset_state(qxl);
- qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl);
- qxl->ssd.cursor_bh = qemu_bh_new(qemu_spice_cursor_refresh_bh, &qxl->ssd);
+ qxl->update_area_bh = qemu_bh_new_guarded(qxl_render_update_area_bh, qxl,
+ &DEVICE(qxl)->mem_reentrancy_guard);
+ qxl->ssd.cursor_bh = qemu_bh_new_guarded(qemu_spice_cursor_refresh_bh, &qxl->ssd,
+ &DEVICE(qxl)->mem_reentrancy_guard);
}
static void qxl_realize_primary(PCIDevice *dev, Error **errp)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5e15c79b94..66ac9b6cc5 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1339,8 +1339,10 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
g->ctrl_vq = virtio_get_queue(vdev, 0);
g->cursor_vq = virtio_get_queue(vdev, 1);
- g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
- g->cursor_bh = qemu_bh_new(virtio_gpu_cursor_bh, g);
+ g->ctrl_bh = qemu_bh_new_guarded(virtio_gpu_ctrl_bh, g,
+ &qdev->mem_reentrancy_guard);
+ g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
+ &qdev->mem_reentrancy_guard);
QTAILQ_INIT(&g->reslist);
QTAILQ_INIT(&g->cmdq);
QTAILQ_INIT(&g->fenceq);
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 55902e1df7..4e76d6b191 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1509,7 +1509,8 @@ static void ahci_cmd_done(const IDEDMA *dma)
ahci_write_fis_d2h(ad);
if (ad->port_regs.cmd_issue && !ad->check_bh) {
- ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
+ ad->check_bh = qemu_bh_new_guarded(ahci_check_cmd_bh, ad,
+ &ad->mem_reentrancy_guard);
qemu_bh_schedule(ad->check_bh);
}
}
diff --git a/hw/ide/ahci_internal.h b/hw/ide/ahci_internal.h
index 303fcd7235..2480455372 100644
--- a/hw/ide/ahci_internal.h
+++ b/hw/ide/ahci_internal.h
@@ -321,6 +321,7 @@ struct AHCIDevice {
bool init_d2h_sent;
AHCICmdHdr *cur_cmd;
NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
+ MemReentrancyGuard mem_reentrancy_guard;
};
struct AHCIPCIState {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 45d14a25e9..de48ff9f86 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -513,6 +513,7 @@ BlockAIOCB *ide_issue_trim(
BlockCompletionFunc *cb, void *cb_opaque, void *opaque)
{
IDEState *s = opaque;
+ IDEDevice *dev = s->unit ? s->bus->slave : s->bus->master;
TrimAIOCB *iocb;
/* Paired with a decrement in ide_trim_bh_cb() */
@@ -520,7 +521,8 @@ BlockAIOCB *ide_issue_trim(
iocb = blk_aio_get(&trim_aiocb_info, s->blk, cb, cb_opaque);
iocb->s = s;
- iocb->bh = qemu_bh_new(ide_trim_bh_cb, iocb);
+ iocb->bh = qemu_bh_new_guarded(ide_trim_bh_cb, iocb,
+ &DEVICE(dev)->mem_reentrancy_guard);
iocb->ret = 0;
iocb->qiov = qiov;
iocb->i = -1;
diff --git a/hw/misc/imx_rngc.c b/hw/misc/imx_rngc.c
index 632c03779c..082c6980ad 100644
--- a/hw/misc/imx_rngc.c
+++ b/hw/misc/imx_rngc.c
@@ -228,8 +228,10 @@ static void imx_rngc_realize(DeviceState *dev, Error **errp)
sysbus_init_mmio(sbd, &s->iomem);
sysbus_init_irq(sbd, &s->irq);
- s->self_test_bh = qemu_bh_new(imx_rngc_self_test, s);
- s->seed_bh = qemu_bh_new(imx_rngc_seed, s);
+ s->self_test_bh = qemu_bh_new_guarded(imx_rngc_self_test, s,
+ &dev->mem_reentrancy_guard);
+ s->seed_bh = qemu_bh_new_guarded(imx_rngc_seed, s,
+ &dev->mem_reentrancy_guard);
}
static void imx_rngc_reset(DeviceState *dev)
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 43bb1f56ba..80a789f32b 100644
--- a/hw/misc/macio/mac_dbdma.c
+++ b/hw/misc/macio/mac_dbdma.c
@@ -914,7 +914,7 @@ static void mac_dbdma_realize(DeviceState *dev, Error **errp)
{
DBDMAState *s = MAC_DBDMA(dev);
- s->bh = qemu_bh_new(DBDMA_run_bh, s);
+ s->bh = qemu_bh_new_guarded(DBDMA_run_bh, s, &dev->mem_reentrancy_guard);
}
static void mac_dbdma_class_init(ObjectClass *oc, void *data)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 53e1c32643..447f669921 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2917,7 +2917,8 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
n->vqs[index].tx_vq =
virtio_add_queue(vdev, n->net_conf.tx_queue_size,
virtio_net_handle_tx_bh);
- n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
+ n->vqs[index].tx_bh = qemu_bh_new_guarded(virtio_net_tx_bh, &n->vqs[index],
+ &DEVICE(vdev)->mem_reentrancy_guard);
}
n->vqs[index].tx_waiting = 0;
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f59dfe1cbe..fd917fcda1 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -4607,7 +4607,8 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
QTAILQ_INSERT_TAIL(&(sq->req_list), &sq->io_req[i], entry);
}
- sq->bh = qemu_bh_new(nvme_process_sq, sq);
+ sq->bh = qemu_bh_new_guarded(nvme_process_sq, sq,
+ &DEVICE(sq->ctrl)->mem_reentrancy_guard);
if (n->dbbuf_enabled) {
sq->db_addr = n->dbbuf_dbs + (sqid << 3);
@@ -5253,7 +5254,8 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
}
}
n->cq[cqid] = cq;
- cq->bh = qemu_bh_new(nvme_post_cqes, cq);
+ cq->bh = qemu_bh_new_guarded(nvme_post_cqes, cq,
+ &DEVICE(cq->ctrl)->mem_reentrancy_guard);
}
static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index c485da792c..3de288b454 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1322,7 +1322,8 @@ static void mptsas_scsi_realize(PCIDevice *dev, Error **errp)
}
s->max_devices = MPTSAS_NUM_PORTS;
- s->request_bh = qemu_bh_new(mptsas_fetch_requests, s);
+ s->request_bh = qemu_bh_new_guarded(mptsas_fetch_requests, s,
+ &DEVICE(dev)->mem_reentrancy_guard);
scsi_bus_init(&s->bus, sizeof(s->bus), &dev->qdev, &mptsas_scsi_info);
}
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index c97176110c..3c20b47ad0 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -193,7 +193,8 @@ static void scsi_dma_restart_cb(void *opaque, bool running, RunState state)
AioContext *ctx = blk_get_aio_context(s->conf.blk);
/* The reference is dropped in scsi_dma_restart_bh.*/
object_ref(OBJECT(s));
- s->bh = aio_bh_new(ctx, scsi_dma_restart_bh, s);
+ s->bh = aio_bh_new_guarded(ctx, scsi_dma_restart_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
qemu_bh_schedule(s->bh);
}
}
diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index fa76696855..4de34536e9 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1184,7 +1184,8 @@ pvscsi_realizefn(PCIDevice *pci_dev, Error **errp)
pcie_endpoint_cap_init(pci_dev, PVSCSI_EXP_EP_OFFSET);
}
- s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
+ s->completion_worker = qemu_bh_new_guarded(pvscsi_process_completion_queue, s,
+ &DEVICE(pci_dev)->mem_reentrancy_guard);
scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(pci_dev), &pvscsi_scsi_info);
/* override default SCSI bus hotplug-handler, with pvscsi's one */
diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c
index 88f99c05d5..f013ded91e 100644
--- a/hw/usb/dev-uas.c
+++ b/hw/usb/dev-uas.c
@@ -937,7 +937,8 @@ static void usb_uas_realize(USBDevice *dev, Error **errp)
QTAILQ_INIT(&uas->results);
QTAILQ_INIT(&uas->requests);
- uas->status_bh = qemu_bh_new(usb_uas_send_status_bh, uas);
+ uas->status_bh = qemu_bh_new_guarded(usb_uas_send_status_bh, uas,
+ &d->mem_reentrancy_guard);
dev->flags |= (1 << USB_DEV_FLAG_IS_SCSI_STORAGE);
scsi_bus_init(&uas->bus, sizeof(uas->bus), DEVICE(dev), &usb_uas_scsi_info);
diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
index 8755e9cbb0..a0c4e782b2 100644
--- a/hw/usb/hcd-dwc2.c
+++ b/hw/usb/hcd-dwc2.c
@@ -1364,7 +1364,8 @@ static void dwc2_realize(DeviceState *dev, Error **errp)
s->fi = USB_FRMINTVL - 1;
s->eof_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_frame_boundary, s);
s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, dwc2_work_timer, s);
- s->async_bh = qemu_bh_new(dwc2_work_bh, s);
+ s->async_bh = qemu_bh_new_guarded(dwc2_work_bh, s,
+ &dev->mem_reentrancy_guard);
sysbus_init_irq(sbd, &s->irq);
}
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d4da8dcb8d..c930c60921 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2533,7 +2533,8 @@ void usb_ehci_realize(EHCIState *s, DeviceState *dev, Error **errp)
}
s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ehci_work_timer, s);
- s->async_bh = qemu_bh_new(ehci_work_bh, s);
+ s->async_bh = qemu_bh_new_guarded(ehci_work_bh, s,
+ &dev->mem_reentrancy_guard);
s->device = dev;
s->vmstate = qemu_add_vm_change_state_handler(usb_ehci_vm_state_change, s);
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 8ac1175ad2..77baaa7a6b 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1190,7 +1190,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL);
}
}
- s->bh = qemu_bh_new(uhci_bh, s);
+ s->bh = qemu_bh_new_guarded(uhci_bh, s, &DEVICE(dev)->mem_reentrancy_guard);
s->frame_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, uhci_frame_timer, s);
s->num_ports_vmstate = NB_PORTS;
QTAILQ_INIT(&s->queues);
diff --git a/hw/usb/host-libusb.c b/hw/usb/host-libusb.c
index 176868d345..f500db85ab 100644
--- a/hw/usb/host-libusb.c
+++ b/hw/usb/host-libusb.c
@@ -1141,7 +1141,8 @@ static void usb_host_nodev_bh(void *opaque)
static void usb_host_nodev(USBHostDevice *s)
{
if (!s->bh_nodev) {
- s->bh_nodev = qemu_bh_new(usb_host_nodev_bh, s);
+ s->bh_nodev = qemu_bh_new_guarded(usb_host_nodev_bh, s,
+ &DEVICE(s)->mem_reentrancy_guard);
}
qemu_bh_schedule(s->bh_nodev);
}
@@ -1739,7 +1740,8 @@ static int usb_host_post_load(void *opaque, int version_id)
USBHostDevice *dev = opaque;
if (!dev->bh_postld) {
- dev->bh_postld = qemu_bh_new(usb_host_post_load_bh, dev);
+ dev->bh_postld = qemu_bh_new_guarded(usb_host_post_load_bh, dev,
+ &DEVICE(dev)->mem_reentrancy_guard);
}
qemu_bh_schedule(dev->bh_postld);
dev->bh_postld_pending = true;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index fd7df599bc..39fbaaab16 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1441,8 +1441,10 @@ static void usbredir_realize(USBDevice *udev, Error **errp)
}
}
- dev->chardev_close_bh = qemu_bh_new(usbredir_chardev_close_bh, dev);
- dev->device_reject_bh = qemu_bh_new(usbredir_device_reject_bh, dev);
+ dev->chardev_close_bh = qemu_bh_new_guarded(usbredir_chardev_close_bh, dev,
+ &DEVICE(dev)->mem_reentrancy_guard);
+ dev->device_reject_bh = qemu_bh_new_guarded(usbredir_device_reject_bh, dev,
+ &DEVICE(dev)->mem_reentrancy_guard);
dev->attach_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, usbredir_do_attach, dev);
packet_id_queue_init(&dev->cancelled, dev, "cancelled");
diff --git a/hw/usb/xen-usb.c b/hw/usb/xen-usb.c
index 66cb3f7c24..38ee660a30 100644
--- a/hw/usb/xen-usb.c
+++ b/hw/usb/xen-usb.c
@@ -1032,7 +1032,8 @@ static void usbback_alloc(struct XenLegacyDevice *xendev)
QTAILQ_INIT(&usbif->req_free_q);
QSIMPLEQ_INIT(&usbif->hotplug_q);
- usbif->bh = qemu_bh_new(usbback_bh, usbif);
+ usbif->bh = qemu_bh_new_guarded(usbback_bh, usbif,
+ &DEVICE(xendev)->mem_reentrancy_guard);
}
static int usbback_free(struct XenLegacyDevice *xendev)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index fd06fcfb3f..d004cf29d2 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -886,8 +886,9 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
precopy_add_notifier(&s->free_page_hint_notify);
object_ref(OBJECT(s->iothread));
- s->free_page_bh = aio_bh_new(iothread_get_aio_context(s->iothread),
- virtio_ballloon_get_free_page_hints, s);
+ s->free_page_bh = aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
+ virtio_ballloon_get_free_page_hints, s,
+ &dev->mem_reentrancy_guard);
}
if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 802e1b9659..2fe804510f 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1074,7 +1074,8 @@ static void virtio_crypto_device_realize(DeviceState *dev, Error **errp)
vcrypto->vqs[i].dataq =
virtio_add_queue(vdev, 1024, virtio_crypto_handle_dataq_bh);
vcrypto->vqs[i].dataq_bh =
- qemu_bh_new(virtio_crypto_dataq_bh, &vcrypto->vqs[i]);
+ qemu_bh_new_guarded(virtio_crypto_dataq_bh, &vcrypto->vqs[i],
+ &dev->mem_reentrancy_guard);
vcrypto->vqs[i].vcrypto = vcrypto;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v10 5/8] lsi53c895a: disable reentrancy detection for script RAM
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (3 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 4/8] hw: replace most qemu_bh_new calls with qemu_bh_new_guarded Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 6/8] bcm2835_property: disable reentrancy detection for iomem Alexander Bulekov
` (4 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Fiona Ebner, Fam Zheng
As the code is designed to use the memory APIs to access the script ram,
disable reentrancy checks for the pseudo-RAM ram_io MemoryRegion.
In the future, ram_io may be converted from an IO to a proper RAM MemoryRegion.
Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
---
hw/scsi/lsi53c895a.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index af93557a9a..db27872963 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2302,6 +2302,12 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
memory_region_init_io(&s->io_io, OBJECT(s), &lsi_io_ops, s,
"lsi-io", 256);
+ /*
+ * Since we use the address-space API to interact with ram_io, disable the
+ * re-entrancy guard.
+ */
+ s->ram_io.disable_reentrancy_guard = true;
+
address_space_init(&s->pci_io_as, pci_address_space_io(dev), "lsi-pci-io");
qdev_init_gpio_out(d, &s->ext_irq, 1);
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v10 6/8] bcm2835_property: disable reentrancy detection for iomem
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (4 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 5/8] lsi53c895a: disable reentrancy detection for script RAM Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-04-27 21:10 ` [PATCH v10 7/8] raven: " Alexander Bulekov
` (3 subsequent siblings)
9 siblings, 0 replies; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Peter Maydell, open list:Raspberry Pi
As the code is designed for re-entrant calls from bcm2835_property to
bcm2835_mbox and back into bcm2835_property, mark iomem as
reentrancy-safe.
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
hw/misc/bcm2835_property.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
index 890ae7bae5..de056ea2df 100644
--- a/hw/misc/bcm2835_property.c
+++ b/hw/misc/bcm2835_property.c
@@ -382,6 +382,13 @@ static void bcm2835_property_init(Object *obj)
memory_region_init_io(&s->iomem, OBJECT(s), &bcm2835_property_ops, s,
TYPE_BCM2835_PROPERTY, 0x10);
+
+ /*
+ * bcm2835_property_ops call into bcm2835_mbox, which in-turn reads from
+ * iomem. As such, mark iomem as re-entracy safe.
+ */
+ s->iomem.disable_reentrancy_guard = true;
+
sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
sysbus_init_irq(SYS_BUS_DEVICE(s), &s->mbox_irq);
}
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v10 7/8] raven: disable reentrancy detection for iomem
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (5 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 6/8] bcm2835_property: disable reentrancy detection for iomem Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-05-04 12:03 ` Darren Kenny
2023-04-27 21:10 ` [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi Alexander Bulekov
` (2 subsequent siblings)
9 siblings, 1 reply; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev,
Hervé Poussineau, open list:PReP
As the code is designed for re-entrant calls from raven_io_ops to
pci-conf, mark raven_io_ops as reentrancy-safe.
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
hw/pci-host/raven.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 072ffe3c5e..9a11ac4b2b 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -294,6 +294,13 @@ static void raven_pcihost_initfn(Object *obj)
memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
+ /*
+ * Raven's raven_io_ops use the address-space API to access pci-conf-idx
+ * (which is also owned by the raven device). As such, mark the
+ * pci_io_non_contiguous as re-entrancy safe.
+ */
+ s->pci_io_non_contiguous.disable_reentrancy_guard = true;
+
/* CPU address space */
memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
&s->pci_io);
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v10 7/8] raven: disable reentrancy detection for iomem
2023-04-27 21:10 ` [PATCH v10 7/8] raven: " Alexander Bulekov
@ 2023-05-04 12:03 ` Darren Kenny
0 siblings, 0 replies; 33+ messages in thread
From: Darren Kenny @ 2023-05-04 12:03 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
Marcel Apfelbaum, Daniel P . Berrangé, Eduardo Habkost,
Jon Maloy, Siqi Chen, Michael Tokarev, Hervé Poussineau,
open list:PReP
On Thursday, 2023-04-27 at 17:10:12 -04, Alexander Bulekov wrote:
> As the code is designed for re-entrant calls from raven_io_ops to
> pci-conf, mark raven_io_ops as reentrancy-safe.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> ---
> hw/pci-host/raven.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 072ffe3c5e..9a11ac4b2b 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -294,6 +294,13 @@ static void raven_pcihost_initfn(Object *obj)
> memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
> address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
>
> + /*
> + * Raven's raven_io_ops use the address-space API to access pci-conf-idx
> + * (which is also owned by the raven device). As such, mark the
> + * pci_io_non_contiguous as re-entrancy safe.
> + */
> + s->pci_io_non_contiguous.disable_reentrancy_guard = true;
> +
> /* CPU address space */
> memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
> &s->pci_io);
> --
> 2.39.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (6 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 7/8] raven: " Alexander Bulekov
@ 2023-04-27 21:10 ` Alexander Bulekov
2023-05-18 20:22 ` Michael S. Tsirkin
2023-05-04 14:08 ` [PATCH v10 0/8] memory: prevent dma-reentracy issues Michael Tokarev
2024-11-08 19:56 ` Alexander Bulekov
9 siblings, 1 reply; 33+ messages in thread
From: Alexander Bulekov @ 2023-04-27 21:10 UTC (permalink / raw)
To: qemu-devel
Cc: Alexander Bulekov, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Michael S . Tsirkin, Marcel Apfelbaum, Daniel P . Berrangé,
Eduardo Habkost, Jon Maloy, Siqi Chen, Michael Tokarev
As the code is designed for re-entrant calls to apic-msi, mark apic-msi
as reentrancy-safe.
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
---
hw/intc/apic.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 20b5a94073..ac3d47d231 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -885,6 +885,13 @@ static void apic_realize(DeviceState *dev, Error **errp)
memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
APIC_SPACE_SIZE);
+ /*
+ * apic-msi's apic_mem_write can call into ioapic_eoi_broadcast, which can
+ * write back to apic-msi. As such mark the apic-msi region re-entrancy
+ * safe.
+ */
+ s->io_memory.disable_reentrancy_guard = true;
+
s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
local_apics[s->id] = s;
--
2.39.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi
2023-04-27 21:10 ` [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi Alexander Bulekov
@ 2023-05-18 20:22 ` Michael S. Tsirkin
2023-05-18 20:33 ` Michael Tokarev
0 siblings, 1 reply; 33+ messages in thread
From: Michael S. Tsirkin @ 2023-05-18 20:22 UTC (permalink / raw)
To: Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Marcel Apfelbaum, Daniel P . Berrangé, Eduardo Habkost,
Jon Maloy, Siqi Chen, Michael Tokarev
On Thu, Apr 27, 2023 at 05:10:13PM -0400, Alexander Bulekov wrote:
> As the code is designed for re-entrant calls to apic-msi, mark apic-msi
> as reentrancy-safe.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
feel free to merge with rest of patchset - who's going to
merge it btw?
> ---
> hw/intc/apic.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 20b5a94073..ac3d47d231 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -885,6 +885,13 @@ static void apic_realize(DeviceState *dev, Error **errp)
> memory_region_init_io(&s->io_memory, OBJECT(s), &apic_io_ops, s, "apic-msi",
> APIC_SPACE_SIZE);
>
> + /*
> + * apic-msi's apic_mem_write can call into ioapic_eoi_broadcast, which can
> + * write back to apic-msi. As such mark the apic-msi region re-entrancy
> + * safe.
> + */
> + s->io_memory.disable_reentrancy_guard = true;
> +
> s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, apic_timer, s);
> local_apics[s->id] = s;
>
> --
> 2.39.0
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi
2023-05-18 20:22 ` Michael S. Tsirkin
@ 2023-05-18 20:33 ` Michael Tokarev
0 siblings, 0 replies; 33+ messages in thread
From: Michael Tokarev @ 2023-05-18 20:33 UTC (permalink / raw)
To: Michael S. Tsirkin, Alexander Bulekov
Cc: qemu-devel, Stefan Hajnoczi, Philippe Mathieu-Daudé,
Mauro Matteo Cascella, Peter Xu, Jason Wang, David Hildenbrand,
Gerd Hoffmann, Thomas Huth, Laurent Vivier, Bandan Das,
Edgar E . Iglesias, Darren Kenny, Bin Meng, Paolo Bonzini,
Marcel Apfelbaum, Daniel P. Berrangé, Eduardo Habkost,
Jon Maloy, Siqi Chen
18.05.2023 23:22, Michael S. Tsirkin пишет:
> On Thu, Apr 27, 2023 at 05:10:13PM -0400, Alexander Bulekov wrote:
>> As the code is designed for re-entrant calls to apic-msi, mark apic-msi
>> as reentrancy-safe.
>>
>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
>> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
>
> feel free to merge with rest of patchset - who's going to
> merge it btw?
>
https://gitlab.com/qemu-project/qemu/-/commit/50795ee051a342c681a9b45671c552fbd6274db8
Author: Alexander Bulekov <alxndr@bu.edu>
AuthorDate: Thu Apr 27 17:10:13 2023 -0400
Commit: Thomas Huth <thuth@redhat.com>
CommitDate: Fri Apr 28 11:31:54 2023 +0200
FWIW
/mjt
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v10 0/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (7 preceding siblings ...)
2023-04-27 21:10 ` [PATCH v10 8/8] apic: disable reentrancy detection for apic-msi Alexander Bulekov
@ 2023-05-04 14:08 ` Michael Tokarev
2024-11-08 19:56 ` Alexander Bulekov
9 siblings, 0 replies; 33+ messages in thread
From: Michael Tokarev @ 2023-05-04 14:08 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
28.04.2023 00:10, Alexander Bulekov wrote:
..
> These patches aim to solve two types of DMA-reentrancy issues:
>
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
Should this patchset (with any follow-up fixes) go to 8.0-stable?
Thanks,
/mjt
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 0/8] memory: prevent dma-reentracy issues
2023-04-27 21:10 [PATCH v10 0/8] memory: prevent dma-reentracy issues Alexander Bulekov
` (8 preceding siblings ...)
2023-05-04 14:08 ` [PATCH v10 0/8] memory: prevent dma-reentracy issues Michael Tokarev
@ 2024-11-08 19:56 ` Alexander Bulekov
2024-11-09 10:14 ` Akihiko Odaki
9 siblings, 1 reply; 33+ messages in thread
From: Alexander Bulekov @ 2024-11-08 19:56 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Philippe Mathieu-Daud??, Mauro Matteo Cascella,
Peter Xu, Jason Wang, David Hildenbrand, Gerd Hoffmann,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
Marcel Apfelbaum, Daniel P . Berrang??, Eduardo Habkost,
Jon Maloy, Siqi Chen, Michael Tokarev, Akihiko Odaki
On 230427 1710, Alexander Bulekov wrote:
<snip>
> These patches aim to solve two types of DMA-reentrancy issues:
>
> 1.) mmio -> dma -> mmio case
> To solve this, we track whether the device is engaged in io by
> checking/setting a reentrancy-guard within APIs used for MMIO access.
>
> 2.) bh -> dma write -> mmio case
> This case is trickier, since we dont have a generic way to associate a
> bh with the underlying Device/DeviceState. Thus, this version allows a
> device to associate a reentrancy-guard with a bh, when creating it.
> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
<snip>
Later on there was also a guard added by Akihiko Odaki for
network-backends.
7d0fefdf81f: net: Provide MemReentrancyGuard * to qemu_new_nic()
Recently a talk came out about re-entrancy bugs in qemu
(unfortuantely I could not find a non-video version of the slides):
https://www.youtube.com/watch?v=wL3LK9Dp4os
The talk gives an overview of these bugs and also demonstrates how they
can be used for VM escapes. It lists the following vectors:
1. MMIO
2. Bottom-Halves
3. Netqueues
4. IOEventfd
5. Timers
The first three should be covered by the current defenses. However,
afaik IOEventfds and Timers are still unprotected. The talk demonstates
an attack via IOEventFd, but not one with timers. In any case, it might
make sense to revist the guard to ensure that we are covering all types
of code that perform DMA.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v10 0/8] memory: prevent dma-reentracy issues
2024-11-08 19:56 ` Alexander Bulekov
@ 2024-11-09 10:14 ` Akihiko Odaki
0 siblings, 0 replies; 33+ messages in thread
From: Akihiko Odaki @ 2024-11-09 10:14 UTC (permalink / raw)
To: Alexander Bulekov, qemu-devel
Cc: Stefan Hajnoczi, Philippe Mathieu-Daud??, Mauro Matteo Cascella,
Peter Xu, Jason Wang, David Hildenbrand, Gerd Hoffmann,
Thomas Huth, Laurent Vivier, Bandan Das, Edgar E . Iglesias,
Darren Kenny, Bin Meng, Paolo Bonzini, Michael S . Tsirkin,
Marcel Apfelbaum, Daniel P . Berrang??, Eduardo Habkost,
Jon Maloy, Siqi Chen, Michael Tokarev
On 2024/11/09 4:56, Alexander Bulekov wrote:
> On 230427 1710, Alexander Bulekov wrote:
>
> <snip>
>> These patches aim to solve two types of DMA-reentrancy issues:
>>
>> 1.) mmio -> dma -> mmio case
>> To solve this, we track whether the device is engaged in io by
>> checking/setting a reentrancy-guard within APIs used for MMIO access.
>>
>> 2.) bh -> dma write -> mmio case
>> This case is trickier, since we dont have a generic way to associate a
>> bh with the underlying Device/DeviceState. Thus, this version allows a
>> device to associate a reentrancy-guard with a bh, when creating it.
>> (Instead of calling qemu_bh_new, you call qemu_bh_new_guarded)
> <snip>
>
> Later on there was also a guard added by Akihiko Odaki for
> network-backends.
> 7d0fefdf81f: net: Provide MemReentrancyGuard * to qemu_new_nic()
>
> Recently a talk came out about re-entrancy bugs in qemu
> (unfortuantely I could not find a non-video version of the slides):
> https://www.youtube.com/watch?v=wL3LK9Dp4os
>
> The talk gives an overview of these bugs and also demonstrates how they
> can be used for VM escapes. It lists the following vectors:
> 1. MMIO
> 2. Bottom-Halves
> 3. Netqueues
> 4. IOEventfd
> 5. Timers
>
> The first three should be covered by the current defenses. However,
> afaik IOEventfds and Timers are still unprotected. The talk demonstates
> an attack via IOEventFd, but not one with timers. In any case, it might
> make sense to revist the guard to ensure that we are covering all types
> of code that perform DMA.
Hi,
Thank you for reporting this.
Was this problem reported at qemu-security or is the presentation the first?
^ permalink raw reply [flat|nested] 33+ messages in thread