Re: [PATCH v5 11/14] iommu/amd: Introduce gDomID-to-hDomID Mapping and handle parent domain invalidation

[Suthikulpanit, Suravee]

On 11/14/2025 3:36 AM, Nicolin Chen wrote:
> > @@ -92,7 +94,60 @@ amd_iommu_alloc_domain_nested(struct iommufd_viommu *viommu, u32 flags,
> > .....
> > +	/*
> > +	 * Normally, when a guest has multiple pass-through devices,
> > +	 * the IOMMU driver setup DTEs with the same stage-2 table and
> > +	 * use the same host domain ID (hDomId). In case of nested translation,
> > +	 * if the guest setup different stage-1 tables with same PASID,
> > +	 * IOMMU would use the same TLB tag. This will results in TLB
> > +	 * aliasing issue.
> > +	 *
> > +	 * The guest is assigning gDomIDs based on its own algorithm for managing
> > +	 * cache tags of (DomID, PASID). Within a single viommu, the nest parent domain
> > +	 * (w/ S2 table) is used by all DTEs. But we need to consistently map the gDomID
> > +	 * to a single hDomID. This is done using an xarray in the vIOMMU to
> > +	 * keep track of the gDomID mapping. When the S2 is changed, the INVALIDATE_IOMMU_PAGES
> > +	 * command must be issued for each hDomID in the xarray.
> > +	 */
> > +	curr = xa_cmpxchg(&aviommu->gdomid_array,
> > +			  ndom->gdom_id, NULL, gdom_info, GFP_ATOMIC);
> > +	if (curr) {
> > +		if (xa_err(curr)) {
> > +			ret = -EINVAL;
> > +			goto out_err_gdom_info;
> > +		} else {
> > +			/* The gDomID already exist */
> > +			pr_debug("%s: Found gdom_id=%#x, hdom_id=%#x\n",
> > +				 __func__, ndom->gdom_id, curr->hdom_id);
> > +			refcount_inc(&curr->users);
> > +			ndom->gdom_info = curr;
> This looks racy..
>
> When a gDomID is shared between two nested domains, a concurrent
> nested_domain_free() could enter before refcount_inc(), and call
> refcount_dec_and_test() or even free the curr and ndom.
>
> Then, this refcount_inc() will blow up, or curr/ndom will UAF.
>
> Actually, I don't see where amd_iommu_alloc_domain_nested() gets
> used in this series.. I assume AMD will use the iommufd's vIOMMU
> infrastructure directly which doesn't mutex across nested domain
> allocation/free calls.
>
> So, the entire thing here should hold xa_lock(), use xas_load()
> for the existing curr and use xas_store() to store gdom_info if
> !curr, and xa_unlock() after gdom_info is fully initialized.
>
> > +			kfree(gdom_info);
> > +			return &ndom->domain;
> > +		}
> > +	}
> > +
> > +	/* The gDomID does not exist. We allocate new hdom_id */
> > +	gdom_info->hdom_id = amd_iommu_pdom_id_alloc();
> > +	if (gdom_info->hdom_id <= 0) {
> > +		xa_cmpxchg(&aviommu->gdomid_array,
> > +			   ndom->gdom_id, gdom_info, NULL, GFP_ATOMIC);
> > +		ret = -ENOSPC;
> > +		goto out_err_gdom_info;
> > +	}
> > +
> > +	refcount_set(&gdom_info->users, 1);
> Similar risk here. gdom_info is stored to the xarray before this
> line. A concurrent amd_iommu_alloc_domain_nested() could get the
> stored gdom_info and blow up at refcount_inc().
>
> Make sure the entire thing is locked and safe.
>
> Nicolin

Thanks for pointing this out. I am fixing this in V6 w/ Jason's 
suggestion to use xa_lock/unlock w/ __xa_cmpxchg.

Thanks,
Suravee