Re: [PATCH v1 3/4] iommu: Unpublish the old attach handle before the group replace callback

[Baolu Lu]

On 11/2/2026 1:54 PM, Nicolin Chen wrote:
> On Mon, Nov 02, 2026 at 11:30:02AM +0800, Yi Liu wrote:
> > On 11/1/26 13:53, Baolu Lu wrote:
> > > This doesn't fix the same-domain replace issue, which can still result
> > > in a use-after-free bug. I agree that this needs a separate lifetime
> > > mechanism as stated in the cover letter. But is it possible to prevent
> > > the same-domain replace case from going through this interface?
> > fwiw, iommufd already guards this at the device-replace layer
> > (iommufd_device_do_replace() short-circuits when hwpt == old_hwpt) [1],
> > so IOMMUFD callers can't currently trigger this. But that's a caller-side
> > convention, not something
> > iommu_replace_group_handle()/iommu_replace_device_pasid() enforce themselves
> > - so it's still
> > reachable if a future/other caller doesn't do the same check. Worth
> > deciding whether to push the guard down into the core API, or document
> > it as a caller contract.
> I can add a fifth patch. Please see if it looks good to you.
>
> Subject: [PATCH] iommu: Reject a same-domain replace in the attach handle APIs
>
> Both iommu_replace_group_handle() and iommu_replace_device_pasid() skip the
> driver callback when the new domain is the one currently attached. In that
> case, nothing flushes the pending faults that may still be holding the old
> handle. Then, the caller frees the old handle once the replace returns, so
> such a fault would hit a use-after-free.
>
> No caller does this today, since iommufd_device_do_replace() returns early
> when the new hwpt is the old one. However, it is only a caller convention,
> which neither API enforces.
>
> Reject a same-domain replace in both APIs with a WARN_ON, and document it
> in their kernel-docs.
>
> Suggested-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> Signed-off-by: Nicolin Chen<nicolinc@xxxxxxxxxx>
> ---
>   drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 3fea48d3538ad..a9bc4ca5dbb4e 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3716,6 +3716,10 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
>    * Caller should always provide a new handle to avoid race with the paths
>    * that have lockless reference to handle if it intends to pass a valid handle.
>    *
> + * The @domain must not be the domain currently attached to the @pasid. Such a
> + * same-domain replace would skip the driver callback flushing pending faults.
> + * Then, those faults could use the old handle after the caller frees it.
> + *
>    * Return 0 on success, or an error.
>    */
>   int iommu_replace_device_pasid(struct iommu_domain *domain,
> @@ -3785,15 +3789,17 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
>   	}
>   
>   	curr_domain = pasid_array_entry_to_domain(curr);
> -	ret = 0;
> -
> -	if (curr_domain != domain) {
> -		ret = __iommu_set_group_pasid(domain, group,
> -					      pasid, curr_domain);
> -		if (ret)
> -			entry = curr; /* restore the old handle */
> +	/* Same as iommu_replace_group_handle(), reject a same-domain replace */
> +	if (WARN_ON(curr_domain == domain)) {
> +		entry = curr; /* restore the old handle */
> +		ret = -EINVAL;
> +		goto out_store;
>   	}
>   
> +	ret = __iommu_set_group_pasid(domain, group, pasid, curr_domain);
> +	if (ret)
> +		entry = curr; /* restore the old handle */
> +
>   out_store:
>   	/*
>   	 * The xa_store() above reserved the memory, and the group->mutex
> @@ -3985,6 +3991,10 @@ EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL");
>    *
>    * Caller should always provide a new handle to avoid race with the paths
>    * that have lockless reference to handle.
> + *
> + * The @new_domain must not be the domain currently attached to the group. Such
> + * a same-domain replace would skip the driver callback flushing pending faults.
> + * Then, those faults could use the old handle after the caller frees it.
>    */
>   int iommu_replace_group_handle(struct iommu_group *group,
>   			       struct iommu_domain *new_domain,
> @@ -3997,6 +4007,16 @@ int iommu_replace_group_handle(struct iommu_group *group,
>   		return -EINVAL;
>   
>   	mutex_lock(&group->mutex);
> +	/*
> +	 * A same-domain replace would skip the driver callback flushing pending
> +	 * faults. Then, those faults could use the old handle after the caller
> +	 * frees it.
> +	 */
> +	if (WARN_ON(group->domain == new_domain)) {
> +		ret = -EINVAL;
> +		goto err_unlock;
> +	}
> +
>   	entry = iommu_make_pasid_array_entry(new_domain, handle);
>   	/*
>   	 * iommu_attach_handle_get() runs without the group mutex, so unpublish

I am fine with this change.

Thanks,
baolu