Re: [PATCH v2 5/5] iommu: Reject a same-domain replace in the attach handle APIs

[Yi Liu]

On 11/2/26 15:04, Nicolin Chen wrote:
> 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.
> + *


Not a native speaker, but this reads a bit ambiguous to me:

I parsed the last sentence as describing intended/acceptable behavior
rather than the hazard being guarded against - "Then, ... could use"
reads like a neutral continuation rather than a warning. Might be worth
making the use-after-free explicit, e.g.:

A same-domain replace would skip the driver callback that flushes
pending faults - this is a use-after-free hazard: a fault still holding
the old handle could dereference it 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.

ditto. Otherwise looks good.

>    */
>   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.

ditto.

Otherwise looks good.

Reviewed-by: Yi Liu <yi.l.liu@xxxxxxxxx>

> +	 */
> +	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