NVIDIA: SAUCE: hw/arm/tegra241-cmdqv: Keep VINTF page0 region alive

[nvmochs]

Summary

Fix a use-after-free in the Tegra241 CMDQV VINTF page0 memory region
during guest reset or VINTF disable.

Related: NVBug 6676450

Root cause

The VINTF page0 unmap path removed, unparented, and immediately freed its
dynamically allocated MemoryRegion.

QEMU FlatViews retain raw MemoryRegion pointers and release their
references asynchronously through RCU. An old FlatView could therefore
access the VINTF page0 region after it had been freed, causing heap
corruption or a QEMU crash during address-space cleanup.

The observed crash path included:

object_unref
address_space_dispatch_free
flatview_destroy
call_rcu_thread

This was introduced by upstream commit:

5965b81 ("hw/arm/tegra241-cmdqv: Use mmap'd host VINTF page0 for virtual VINTF page0")

Fix

Embed the VINTF page0 MemoryRegion in Tegra241CMDQV and initialize
and add it only once.

When the guest disables VINTF or resets, disable the region with
memory_region_set_enabled(). Re-enable the same region when VINTF is
enabled again.

New FlatViews omit the disabled region, while old FlatViews continue to
reference valid storage until their RCU cleanup completes. This also
eliminates per-reset allocation and freeing of the region.

Reproducer

Start an Arm virt guest with one passed-through device behind an
accelerated SMMUv3 configured with cmdqv=on, and add an HMP monitor:

-monitor unix:/tmp/qmon.sock,server,nowait

Freed-memory poisoning makes the failure reliable without an ASan build:

GLIBC_TUNABLES=glibc.malloc.tcache_count=0
MALLOC_PERTURB_=165
MALLOC_CHECK_=3
qemu-system-aarch64

After info mtree shows the VINTF page0 region, reset through the
monitor:

printf 'system_reset\n' |
timeout 5 nc -N -U /tmp/qmon.sock

The unpatched binary crashed on the first reset with SIGSEGV. The core
showed object_unref() receiving the poisoned pointer
0xa5a5a5a5a5a5a5a5 from deferred address-space cleanup.

Validation

• Unpatched, one CMDQV instance: SIGSEGV on the first reset
• Patched, one CMDQV instance: 100/100 resets completed successfully
• scripts/checkpatch.pl: 0 errors and 0 warnings
• Patch applies cleanly to current upstream/master

[nirmoy]

BaseOS Kernel Review

Tip

✅ Review passed

No issues found across the reviewed commits.

Findings: none

🔍 Review artifacts

• 🟢 Boro review: no findings

Review metadata

• Reviewed head: f84a70f86ac9
• Overall status: passed

This comment is maintained by BaseOS Reviewer and updated when the GitHub watcher publishes a newer review.

[MitchellAugustin]

This all looks correct to me and consistent with the upstream patch, thanks!

Acked-by: Mitchell Augustin <mitchell.augustin@canonical.com>

[NathanChenNVIDIA]

From Cursor:

The old code destroyed the vintf_page0 region and built a new one with the current vintf_page0 in tegra241_cmdqv_guest_map_vintf_page0(). In this patch, tegra241_cmdqv_guest_map_vintf_page0() => memory_region_init_ram_device_ptr(..., cmdqv->vintf_page0) runs once, and then sets mr_vintf_page0_initialized to true, never updating the RAM pointer. If QEMU later munmaps that page and mmaps it again at a different address (device unplug/replug, viommu free then alloc), the guest region still points at the old address. Reset-only testing does not catch this, because reset leaves the original mmap in place.

Can vintf_page0 be remapped after the first memory_region_init_ram_device_ptr? If not, it might be worth mentioning in the commit message.

Separately, tegra241_cmdqv_free_viommu() munmaps while the guest region can still be live. Does the region need to be disabled before that munmap?

[k-s-0]

+1 to @NathanChenNVIDIA's concern, my AI review caught the same. LGTM otherwise.

[nvmochs]

From Cursor:

The old code destroyed the vintf_page0 region and built a new one with the current vintf_page0 in tegra241_cmdqv_guest_map_vintf_page0(). In this patch, tegra241_cmdqv_guest_map_vintf_page0() => memory_region_init_ram_device_ptr(..., cmdqv->vintf_page0) runs once, and then sets mr_vintf_page0_initialized to true, never updating the RAM pointer. If QEMU later munmaps that page and mmaps it again at a different address (device unplug/replug, viommu free then alloc), the guest region still points at the old address. Reset-only testing does not catch this, because reset leaves the original mmap in place.

Can vintf_page0 be remapped after the first memory_region_init_ram_device_ptr? If not, it might be worth mentioning in the commit message.

Separately, tegra241_cmdqv_free_viommu() munmaps while the guest region can still be live. Does the region need to be disabled before that munmap?

@NathanChenNVIDIA

Thanks for raising this. I traced the allocation and teardown paths.

cmdqv_ops->free_viommu() currently has only one call site: the failure-unwind path in smmuv3_accel_alloc_viommu(). At that point device setup has not completed and the guest cannot have initialized the VINTF page0 MemoryRegion. A failed allocation may subsequently be retried with a different mmap address, but mr_vintf_page0_initialized is still false in that case.

After successful initialization, the device that established the CMDQV vIOMMU association is protected by an unplug blocker. Reset and VINTF disable do not free or remap the mmap; they only disable the guest-visible region. Therefore, under the currently supported lifecycle, vintf_page0 cannot change after memory_region_init_ram_device_ptr() has run.

For the same reason, tegra241_cmdqv_free_viommu() cannot currently be called while the guest region is live, so it does not need to disable the region before munmap(). A simple disable followed immediately by munmap() would not be sufficient for a future runtime teardown path anyway, because old FlatViews could retain the backing pointer until their RCU cleanup completes.

I agree that this lifetime assumption is not obvious from the CMDQV code. I propose documenting it in the commit message and enforcing the current free_viommu() contract with:

     if (!viommu) {
        return;
     }

     /*
      * This callback is only used to unwind vIOMMU allocation before
      * the guest VINTF page0 MemoryRegion can be initialized.
      */
     g_assert(!cmdqv->mr_vintf_page0_initialized);

This would also catch any future caller that attempts to free or remap the backing after the MemoryRegion has been initialized. FYI, I have tested this assert by forcing the error path in smmuv3_accel_alloc_viommu().

[shamiali2008]

From Cursor:
The old code destroyed the vintf_page0 region and built a new one with the current vintf_page0 in tegra241_cmdqv_guest_map_vintf_page0(). In this patch, tegra241_cmdqv_guest_map_vintf_page0() => memory_region_init_ram_device_ptr(..., cmdqv->vintf_page0) runs once, and then sets mr_vintf_page0_initialized to true, never updating the RAM pointer. If QEMU later munmaps that page and mmaps it again at a different address (device unplug/replug, viommu free then alloc), the guest region still points at the old address. Reset-only testing does not catch this, because reset leaves the original mmap in place.
Can vintf_page0 be remapped after the first memory_region_init_ram_device_ptr? If not, it might be worth mentioning in the commit message.
Separately, tegra241_cmdqv_free_viommu() munmaps while the guest region can still be live. Does the region need to be disabled before that munmap?

@NathanChenNVIDIA

Thanks for raising this. I traced the allocation and teardown paths.

cmdqv_ops->free_viommu() currently has only one call site: the failure-unwind path in smmuv3_accel_alloc_viommu(). At that point device setup has not completed and the guest cannot have initialized the VINTF page0 MemoryRegion. A failed allocation may subsequently be retried with a different mmap address, but mr_vintf_page0_initialized is still false in that case.

After successful initialization, the device that established the CMDQV vIOMMU association is protected by an unplug blocker. Reset and VINTF disable do not free or remap the mmap; they only disable the guest-visible region. Therefore, under the currently supported lifecycle, vintf_page0 cannot change after memory_region_init_ram_device_ptr() has run.

For the same reason, tegra241_cmdqv_free_viommu() cannot currently be called while the guest region is live, so it does not need to disable the region before munmap(). A simple disable followed immediately by munmap() would not be sufficient for a future runtime teardown path anyway, because old FlatViews could retain the backing pointer until their RCU cleanup completes.

I agree that this lifetime assumption is not obvious from the CMDQV code. I propose documenting it in the commit message and enforcing the current free_viommu() contract with:

     if (!viommu) {
        return;
     }

     /*
      * This callback is only used to unwind vIOMMU allocation before
      * the guest VINTF page0 MemoryRegion can be initialized.
      */
     g_assert(!cmdqv->mr_vintf_page0_initialized);

This would also catch any future caller that attempts to free or remap the backing after the MemoryRegion has been initialized. FYI, I have tested this assert by forcing the error path in smmuv3_accel_alloc_viommu().

Yes, once CMDQ is initialized, we explicitly block free_viommu and alloc again on hot add paths. This is to prevent associating the already initialized Guest CMDQ with a different host SMMUv3. And I don't think we gain much in adding that g_assert there.

[shamiali2008]

LGTM:
Acked-by: Shameer Kolothum skolothumtho@nvidia.com

[k-s-0]

LGTM considering the analysis of the vintf_page0 lifetime / teardown concern

Acked-by: Keifer Snedeker <keifer.snedeker@canonical.com>

[nvmochs]

FYI, I pushed an updated commit that adds the following paragraph to the end of the commit message for clarity:

    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, before guest CMDQ initialization.

[mxc42]

LGTM

Acked-by: Morgan Hunter <morgan.hunter@canonical.com

[NathanChenNVIDIA]

LGTM, thanks for looking into the concerns above!

Acked-by: Nathan Chen <nathanc@nvidia.com>

[nvmochs]

Thanks all for the reviews.

Merged, closing PR.

2d16045a1153 (HEAD -> nvidia_stable-11.0, nvidia/nvidia_stable-11.0) NVIDIA: SAUCE: hw/arm/tegra241-cmdqv: Keep VINTF page0 region alive