RE: [PATCH v2] iommu/iommufd: Fix IOPF group ownership UAF

[Tian, Kevin]

> From: Peiyang He <peiyang_he@xxxxxxxxxxxxxxxx>
> Sent: Saturday, July 18, 2026 12:33 PM
> 
> iopf_group_alloc() links each last-page IOPF group into the generic IOPF
> pending list before invoking the domain fault handler.
> iommufd_fault_iopf_handler() also queued an accepted group in the
> IOMMUFD deliver list without removing it from the generic pending list.
> 
> When detach or HWPT replacement drops the device's IOPF reference count
> to zero, an IOMMU driver may call iopf_queue_remove_device(). That
> function responds to and frees groups through the generic pending list
> without removing the same groups from IOMMUFD's deliver list or response
> xarray. A later read, response, or cleanup can then access the freed
> group and cause a UAF.
> 
> Fix this by dequeuing an accepted group from the generic pending list
> before IOMMUFD queues it for userspace response, and by making
> iopf_group_response() free the group as well after sending the response.
> The abort_group cleanup in iommu_report_device_fault() keeps
> using the locked helper, since abort_group is stack-allocated
> and must not be freed by iopf_group_response().
> 
> Closes: https://lore.kernel.org/all/B4F28798E2E784CA+d29f723c-b2b5-4b67-
> 8d1c-4f7b9b0b27cb@xxxxxxxxxxxxxxxx/
> Fixes: 34765cbc679c ("iommufd: Associate fault object with
> iommufd_hw_pgtable")
> Cc: stable@xxxxxxxxxxxxxxx
> Tested-by: Peiyang He <peiyang_he@xxxxxxxxxxxxxxxx>
> Signed-off-by: Peiyang He <peiyang_he@xxxxxxxxxxxxxxxx>
> Assisted-by: Codex:gpt-5.6-sol

this is basically ok but let's split it into two.

the first patch just includes addition of iopf_group_dequeue() and
removal of the check on list_empty() in iopf_group_response(). that's
simple and correct for backporting to stable.

the 2nd patch does the remaining cleanup:

  __iopf_group_response_locked(): locked version of sending response
  __iopf_group_response(): acquire lock and send response
  iopf_group_response(): send response plus freeing group. still as the
  exported symbol

then...

> @@ -267,11 +282,14 @@ int iommu_report_device_fault(struct device *dev,
> struct iopf_fault *evt)
>  err_abort:
>  	dev_warn_ratelimited(dev, "iopf with pasid %d aborted\n",
>  			     fault->prm.pasid);
> -	iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);

...just replace iopf_group_response() with __iopf_group_response(). No
change to the following lines:

> -	if (group == &abort_group)
> +	if (group == &abort_group) {
> +		mutex_lock(&group->fault_param->lock);
> +		iopf_group_response_locked(group,
> IOMMU_PAGE_RESP_FAILURE);
> +		mutex_unlock(&group->fault_param->lock);
>  		__iopf_free_group(group);
> -	else
> -		iopf_free_group(group);
> +	} else {
> +		iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> +	}

[...]

>  void iopf_group_response(struct iopf_group *group,
>  			 enum iommu_page_response_code status)
>  {
>  	struct iommu_fault_param *fault_param = group->fault_param;
> -	struct iopf_fault *iopf = &group->last_fault;
> -	struct device *dev = group->fault_param->dev;
> -	const struct iommu_ops *ops = dev_iommu_ops(dev);
> -	struct iommu_page_response resp = {
> -		.pasid = iopf->fault.prm.pasid,
> -		.grpid = iopf->fault.prm.grpid,
> -		.code = status,
> -	};
> 
> -	/* Only send response if there is a fault report pending */
>  	mutex_lock(&fault_param->lock);
> -	if (!list_empty(&group->pending_node)) {
> -		ops->page_response(dev, &group->last_fault, &resp);
> -		list_del_init(&group->pending_node);
> -	}
> +	iopf_group_response_locked(group, status);
>  	mutex_unlock(&fault_param->lock);
> +	iopf_free_group(group);
>  }

iopf_group_response()
{
	__ iopf_group_response();
	iopf_free_group(group);
}

> @@ -469,15 +495,7 @@ void iopf_queue_remove_device(struct iopf_queue
> *queue, struct device *dev)
>  		kfree(partial_iopf);
> 
>  	list_for_each_entry_safe(group, temp, &fault_param->faults,
> pending_node) {
> -		struct iopf_fault *iopf = &group->last_fault;
> -		struct iommu_page_response resp = {
> -			.pasid = iopf->fault.prm.pasid,
> -			.grpid = iopf->fault.prm.grpid,
> -			.code = IOMMU_PAGE_RESP_INVALID
> -		};
> -
> -		ops->page_response(dev, iopf, &resp);
> -		list_del_init(&group->pending_node);
> +		iopf_group_response_locked(group,
> IOMMU_PAGE_RESP_INVALID);
>  		iopf_free_group(group);
>  	}

	__iopf_group_response_locked();
	iopf_free_group(group);