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
