Re: [PATCH 3/4] vfio/container: Fix potential SIGSEGV when recover from unmap-all-vaddr failure

[Steven Sistare]

On 6/24/2025 12:54 PM, Cédric Le Goater wrote:

On 6/23/25 12:22, Zhenzhong Duan wrote:

cpr.saved_dma_map isn't initialized in source qemu which lead to vioc->dma_map
assigned a NULL value, this will trigger SIGSEGV.

I don't understand the scenario. Could you please explain more ?

Thank you Zhenzhong, I see the bug.

CPR overrides then restores dma_map in both outgoing and incoming QEMU, for
different reasons. But it only sets saved_dma_map in the target.

So, a more future-proof fix IMO is to always set saved_dma_map:

@@ -182,9 +182,9 @@ bool vfio_legacy_cpr_register_container(VFIOContainer *conta
     vmstate_register(NULL, -1, &vfio_container_vmstate, container);

     /* During incoming CPR, divert calls to dma_map. */
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+    container->cpr.saved_dma_map = vioc->dma_map;
     if (cpr_is_incoming()) {
-        VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
-        container->cpr.saved_dma_map = vioc->dma_map;
         vioc->dma_map = vfio_legacy_cpr_dma_map;
     }

- Steve

Fix it by save and restore vioc->dma_map locally.

Steve, this is cpr territory. Is it still an issue with the rest
of the patches applied ?

Thanks,

C.

Fixes: eba1f657cbb1 ("vfio/container: recover from unmap-all-vaddr failure")
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
  include/hw/vfio/vfio-cpr.h | 8 +++++---
  hw/vfio/cpr-legacy.c       | 3 ++-
  2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index 8bf85b9f4e..aef542e93c 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -16,14 +16,16 @@ struct VFIOContainer;
  struct VFIOContainerBase;
  struct VFIOGroup;
+typedef int (*DMA_MAP_FUNC)(const struct VFIOContainerBase *bcontainer,
+                            hwaddr iova, ram_addr_t size, void *vaddr,
+                            bool readonly, MemoryRegion *mr);
+
  typedef struct VFIOContainerCPR {
      Error *blocker;
      bool vaddr_unmapped;
      NotifierWithReturn transfer_notifier;
      MemoryListener remap_listener;
-    int (*saved_dma_map)(const struct VFIOContainerBase *bcontainer,
-                         hwaddr iova, ram_addr_t size,
-                         void *vaddr, bool readonly, MemoryRegion *mr);
+    DMA_MAP_FUNC saved_dma_map;
  } VFIOContainerCPR;
  typedef struct VFIODeviceCPR {
diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
index a84c3247b7..100a8db74d 100644
--- a/hw/vfio/cpr-legacy.c
+++ b/hw/vfio/cpr-legacy.c
@@ -148,6 +148,7 @@ static int vfio_cpr_fail_notifier(NotifierWithReturn 
*notifier,
           */
          VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+        DMA_MAP_FUNC saved_dma_map = vioc->dma_map;
          vioc->dma_map = vfio_legacy_cpr_dma_map;
          container->cpr.remap_listener = (MemoryListener) {
@@ -158,7 +159,7 @@ static int vfio_cpr_fail_notifier(NotifierWithReturn 
*notifier,
                                   bcontainer->space->as);
          memory_listener_unregister(&container->cpr.remap_listener);
          container->cpr.vaddr_unmapped = false;
-        vioc->dma_map = container->cpr.saved_dma_map;
+        vioc->dma_map = saved_dma_map;
      }
      return 0;
  }