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

From: Tian, Kevin

Date: Fri Jul 17 2026 - 04:00:55 EST


> From: Peiyang He <peiyang_he@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, July 15, 2026 12:24 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 separating response state from generic ownership.
> Add iopf_group::response_pending to track whether a device response
> is still owed, independently of generic pending-list membership,
> and update that state under iommu_fault_param::lock in a helper.
> Before publishing an accepted group to the IOMMUFD deliver list, call
> iopf_group_take_ownership() to remove it from the generic pending list.
> IOMMUFD then becomes the sole owner responsible for responding to and
> freeing the group.

this is ok but could be simplified.

There is no use case in which we allow a group staying in the pending
list after its response has been sent. Actually every iopf_group_response()
is followed by iopf_free_group().

so let's just remove the list_empty() check in iopf_group_response().

better - move iopf_free_group() into iopf_group_response() (with a special
handling in err unwinding of iommu_report_device_fault()).

> +/**
> + * iopf_group_take_ownership - Take ownership of a group of page faults
> + * @group: the group of faults whose ownership is transferred to the fault
> handler
> + *
> + * Remove the group from the generic IOPF pending list. The fault handler is
> + * responsible for responding to and freeing the group after this returns.
> + */
> +void iopf_group_take_ownership(struct iopf_group *group)

iopf_group_dequeue()

> @@ -582,7 +582,7 @@ static int iommufd_hwpt_replace_device(struct
> iommufd_device *idev,
> if (rc)
> goto out_free_handle;
>
> - iommufd_auto_response_faults(hwpt, old_handle);
> + iommufd_auto_response_faults(old, old_handle);

this belongs to another patch already queued by Jason.