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

[Peiyang He]

On 2026/7/17 16:00, Tian, Kevin wrote:
>> 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.
> 

Hi Kevin,

v2 patch has addressed all these issues and passed my PoC. Thanks for your feedback!

Best,
Peiyang