Re: [PATCH] hw/virtio: reject inverted virtio-iommu IOVA ranges

On Wed, Jul 29, 2026 at 02:51:53PM +0800, Jia Jia wrote:
> Guest MAP and UNMAP requests can set virt_end below virt_start. Since
> virt_end is inclusive, this is not a valid interval. MAP nevertheless
> stores it in domain->mappings, but interval_cmp() assumes low <= high.
> For an inverted key, interval_cmp(key, key) returns -1. A covering UNMAP
> can therefore find the key but fail to remove it and repeat forever while
> holding s->mutex.
> 
> Reject inverted request ranges with VIRTIO_IOMMU_S_INVAL and make the
> notifier range decomposition skip invalid ranges. Keep the existing
> notifier-before-remove ordering, but return VIRTIO_IOMMU_S_DEVERR if
> g_tree_remove() fails.
> 
> State produced by an older QEMU can still carry such a key over
> migration. Reconstruct endpoints first so failed loads can be cleaned up by
> the normal reset and unrealize paths. Validate every mapping before switching
> address spaces or scheduling command processing, and reject invalid migration
> state.
> 
> Fixes: fe2cacae2438 ("virtio-iommu: Implement map/unmap")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4104
> Signed-off-by: Jia Jia <[email protected]>

I'm fine with rejecting this to make debugging such issues easier, but
worrying about migrating from qemu with this is IMHO not worth it.


> ---
>  hw/virtio/virtio-iommu.c | 77 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 533bd5073f..b7145c8277 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -210,7 +210,13 @@ static void virtio_iommu_notify_map_unmap(IOMMUMemoryRegion *mr,
>                                            IOMMUTLBEvent *event,
>                                            hwaddr virt_start, hwaddr virt_end)
>  {
> -    uint64_t delta = virt_end - virt_start;
> +    uint64_t delta;
> +
> +    if (virt_end < virt_start) {
> +        return;
> +    }
> +
> +    delta = virt_end - virt_start;
>  
>      event->entry.iova = virt_start;
>      event->entry.addr_mask = delta;
> @@ -807,6 +813,10 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
>          return VIRTIO_IOMMU_S_INVAL;
>      }
>  
> +    if (virt_end < virt_start) {
> +        return VIRTIO_IOMMU_S_INVAL;
> +    }
> +
>      domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
>      if (!domain) {
>          return VIRTIO_IOMMU_S_NOENT;
> @@ -857,6 +867,10 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>  
>      trace_virtio_iommu_unmap(domain_id, virt_start, virt_end);
>  
> +    if (virt_end < virt_start) {
> +        return VIRTIO_IOMMU_S_INVAL;
> +    }
> +
>      domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
>      if (!domain) {
>          return VIRTIO_IOMMU_S_NOENT;
> @@ -879,7 +893,10 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
>                  virtio_iommu_notify_unmap(ep->iommu_mr, current_low,
>                                            current_high);
>              }
> -            g_tree_remove(domain->mappings, iter_key);
> +            if (!g_tree_remove(domain->mappings, iter_key)) {
> +                ret = VIRTIO_IOMMU_S_DEVERR;
> +                break;
> +            }
>              trace_virtio_iommu_unmap_done(domain_id, current_low, current_high);
>          } else {
>              ret = VIRTIO_IOMMU_S_RANGE;
> @@ -1639,12 +1656,62 @@ static gboolean reconstruct_endpoints(gpointer key, gpointer value,
>      return false; /* continue the domain traversal */
>  }
>  
> -static int iommu_post_load(void *opaque, int version_id)
> +typedef struct VirtIOIOMMUMappingValidation {
> +    bool valid;
> +    uint32_t domain_id;
> +    uint64_t low;
> +    uint64_t high;
> +} VirtIOIOMMUMappingValidation;
> +
> +static gboolean virtio_iommu_validate_mapping(gpointer key, gpointer value,
> +                                              gpointer data)
> +{
> +    VirtIOIOMMUInterval *interval = key;
> +    VirtIOIOMMUMappingValidation *validation = data;
> +
> +    if (interval->high < interval->low) {
> +        validation->valid = false;
> +        validation->low = interval->low;
> +        validation->high = interval->high;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static gboolean virtio_iommu_validate_domain_mappings(gpointer key,
> +                                                      gpointer value,
> +                                                      gpointer data)
> +{
> +    VirtIOIOMMUDomain *domain = value;
> +    VirtIOIOMMUMappingValidation *validation = data;
> +
> +    validation->domain_id = domain->id;
> +    g_tree_foreach(domain->mappings, virtio_iommu_validate_mapping,
> +                   validation);
> +    return !validation->valid;
> +}
> +
> +static bool iommu_post_load_errp(void *opaque, int version_id, Error **errp)
>  {
>      VirtIOIOMMU *s = opaque;
> +    VirtIOIOMMUMappingValidation validation = {
> +        .valid = true,
> +    };
>  
> +    /* Rebuild ownership before validation so failed loads can be cleaned up. */
>      g_tree_foreach(s->domains, reconstruct_endpoints, s);
>  
> +    g_tree_foreach(s->domains, virtio_iommu_validate_domain_mappings,
> +                   &validation);
> +    if (!validation.valid) {
> +        error_setg(errp,
> +                   "virtio-iommu: invalid migrated mapping in domain %u: "
> +                   "[0x%" PRIx64 ", 0x%" PRIx64 "]",
> +                   validation.domain_id, validation.low, validation.high);
> +        return false;
> +    }
> +
>      /*
>       * Memory regions are dynamically turned on/off depending on
>       * 'config.bypass' and attached domain type if there is. After
> @@ -1657,14 +1724,14 @@ static int iommu_post_load(void *opaque, int version_id)
>          timer_mod(s->cmd_timer,
>                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL_RT) + 1);
>      }
> -    return 0;
> +    return true;
>  }
>  
>  static const VMStateDescription vmstate_virtio_iommu_device = {
>      .name = "virtio-iommu-device",
>      .minimum_version_id = 2,
>      .version_id = 2,
> -    .post_load = iommu_post_load,
> +    .post_load_errp = iommu_post_load_errp,
>      .fields = (const VMStateField[]) {
>          VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 2,
>                                     &vmstate_domain, VirtIOIOMMUDomain),
> -- 
> 2.34.1