From: "Matthew R. Ochs" <[email protected]>
With CMDQV enabled, resetting a guest after it enables VINTF invokes
the VINTF page0 unmap path. The resulting crash is intermittent and
has been observed on the RCU reclaim thread as:
reboot: Restarting system
double free or corruption (!prev)
...
#5 address_space_dispatch_free
#6 flatview_destroy
#7 call_rcu_thread
FlatViews retain raw MemoryRegion pointers and release their references
asynchronously through RCU. The VINTF page0 unmap path removes the
subregion and immediately unparents and frees it. An old FlatView can
then access the freed region during teardown, resulting in a
use-after-free and heap corruption.
Embed the VINTF page0 MemoryRegion in Tegra241CMDQV and tie its lifetime
to the VINTF page0 mmap. Initialize and add the region disabled after
successful vIOMMU allocation, and remove and unparent it before releasing
the mmap during allocation unwind. Guest VINTF enable and disable then
only toggle the region with memory_region_set_enabled(). New FlatViews
omit the disabled region, while old views continue to reference valid
storage.
Keeping the region initialized across VINTF disable and reset is safe
because the vIOMMU association and its VINTF page0 mmap remain stable
once the guest CMDQ has been initialized. QEMU blocks hot-unplug of the
device that established the association, so later hot-adds reuse it
instead of associating the initialized guest CMDQ with a different host
SMMUv3. The CMDQV free_viommu path is therefore only invoked while
unwinding initial allocation, when the region is still disabled and has
never entered a FlatView.
Fixes: 5965b81ce283 ("hw/arm/tegra241-cmdqv: Use mmap'd host VINTF page0 for
virtual VINTF page0")
Suggested-by: Shameer Kolothum <[email protected]>
Signed-off-by: Matthew R. Ochs <[email protected]>
Message-id: [email protected]
Reviewed-by: Peter Maydell <[email protected]>
Signed-off-by: Peter Maydell <[email protected]>
---
hw/arm/tegra241-cmdqv.c | 54 +++++++++++++++++++++--------------------
hw/arm/tegra241-cmdqv.h | 2 +-
2 files changed, 29 insertions(+), 27 deletions(-)
diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
index 273633e6293..57041408a7f 100644
--- a/hw/arm/tegra241-cmdqv.c
+++ b/hw/arm/tegra241-cmdqv.c
@@ -131,36 +131,12 @@ static void
tegra241_cmdqv_reset_vcmdq_cache(Tegra241CMDQV *cmdqv, int index)
static void tegra241_cmdqv_guest_unmap_vintf_page0(Tegra241CMDQV *cmdqv)
{
- if (!cmdqv->mr_vintf_page0) {
- return;
- }
-
- memory_region_del_subregion(&cmdqv->mmio_cmdqv, cmdqv->mr_vintf_page0);
- object_unparent(OBJECT(cmdqv->mr_vintf_page0));
- g_free(cmdqv->mr_vintf_page0);
- cmdqv->mr_vintf_page0 = NULL;
+ memory_region_set_enabled(&cmdqv->mr_vintf_page0, false);
}
static void tegra241_cmdqv_guest_map_vintf_page0(Tegra241CMDQV *cmdqv)
{
- char *name;
-
- if (cmdqv->mr_vintf_page0) {
- return;
- }
-
- name = g_strdup_printf("%s vintf-page0",
- memory_region_name(&cmdqv->mmio_cmdqv));
- cmdqv->mr_vintf_page0 = g_malloc0(sizeof(*cmdqv->mr_vintf_page0));
- memory_region_init_ram_device_ptr(cmdqv->mr_vintf_page0,
- memory_region_owner(&cmdqv->mmio_cmdqv),
- name, VINTF_PAGE_SIZE,
- cmdqv->vintf_page0);
- memory_region_set_skip_iommu_map(cmdqv->mr_vintf_page0, true);
- memory_region_add_subregion_overlap(&cmdqv->mmio_cmdqv,
- CMDQV_VINTF_PAGE0_BASE,
- cmdqv->mr_vintf_page0, 1);
- g_free(name);
+ memory_region_set_enabled(&cmdqv->mr_vintf_page0, true);
}
static void tegra241_cmdqv_free_vcmdq(Tegra241CMDQV *cmdqv, int index)
@@ -899,6 +875,9 @@ static void tegra241_cmdqv_free_viommu(SMMUv3State *s)
cmdqv->veventq = NULL;
}
if (cmdqv->vintf_page0) {
+ memory_region_del_subregion(&cmdqv->mmio_cmdqv,
+ &cmdqv->mr_vintf_page0);
+ object_unparent(OBJECT(&cmdqv->mr_vintf_page0));
munmap(cmdqv->vintf_page0, VINTF_PAGE_SIZE);
cmdqv->vintf_page0 = NULL;
}
@@ -910,6 +889,7 @@ tegra241_cmdqv_alloc_viommu(SMMUv3State *s,
HostIOMMUDeviceIOMMUFD *idev,
uint32_t *out_viommu_id, Error **errp)
{
Tegra241CMDQV *cmdqv = s->s_accel->cmdqv;
+ char *name;
uint32_t viommu_id, veventq_id, veventq_fd;
IOMMUFDVeventq *veventq;
int flags;
@@ -955,6 +935,28 @@ tegra241_cmdqv_alloc_viommu(SMMUv3State *s,
HostIOMMUDeviceIOMMUFD *idev,
/* Set up event handler for veventq fd */
qemu_set_fd_handler(veventq_fd, tegra241_cmdqv_event_read, NULL, cmdqv);
+
+ /*
+ * Tie the MemoryRegion lifetime to the VINTF page0 mmap. Initialize it
+ * disabled. If initial vIOMMU setup later unwinds, the region has never
+ * entered a FlatView and can be removed and unparented before releasing
+ * the mmap. After successful setup, keep it parented across guest disable
+ * and reset, and only toggle its enabled state so old FlatViews continue
+ * to reference valid storage.
+ */
+ name = g_strdup_printf("%s vintf-page0",
+ memory_region_name(&cmdqv->mmio_cmdqv));
+ memory_region_init_ram_device_ptr(&cmdqv->mr_vintf_page0,
+ memory_region_owner(&cmdqv->mmio_cmdqv),
+ name, VINTF_PAGE_SIZE,
+ cmdqv->vintf_page0);
+ memory_region_set_skip_iommu_map(&cmdqv->mr_vintf_page0, true);
+ memory_region_set_enabled(&cmdqv->mr_vintf_page0, false);
+ memory_region_add_subregion_overlap(&cmdqv->mmio_cmdqv,
+ CMDQV_VINTF_PAGE0_BASE,
+ &cmdqv->mr_vintf_page0, 1);
+ g_free(name);
+
*out_viommu_id = viommu_id;
return true;
diff --git a/hw/arm/tegra241-cmdqv.h b/hw/arm/tegra241-cmdqv.h
index de4c1e53358..bba985099e3 100644
--- a/hw/arm/tegra241-cmdqv.h
+++ b/hw/arm/tegra241-cmdqv.h
@@ -49,7 +49,7 @@ typedef struct Tegra241CMDQV {
IOMMUFDVeventq *veventq;
IOMMUFDHWqueue *vcmdq[TEGRA241_CMDQV_MAX_CMDQ];
void *vintf_page0;
- MemoryRegion *mr_vintf_page0;
+ MemoryRegion mr_vintf_page0;
/* CMDQ-V Config page register cache */
uint32_t config;
--
2.43.0
