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

[Yi Liu <yi.l.liu@intel.com>]

On 10/30/26 02:45, Jason Gunthorpe wrote:
> On Thu, Oct 29, 2026 at 07:00:18PM +0800, Yi Liu wrote:
>> On 10/29/26 15:25, Tian, Kevin wrote:
>>>> From: Nicolin Chen <nicolinc@nvidia.com>
>>>> Sent: Wednesday, September 30, 2026 5:52 AM
>>>>
>>>> iommu_replace_group_handle() looks like it hides the detaching handle ahead
>>>> of the driver callback. But in fact, xa_reserve() calls xa_cmpxchg(), which
>>>> stores only when the current entry matches @old=NULL. For a replace, there
>>>> must be an entry sitting there by definition (i.e. @old cannot be NULL), so
>>>> the compare fails, XA_ZERO_ENTRY is never written, and the call decays into
>>>> a plain read.
>>>>
>>>> Therefore, the old handle stays published during
>>>> __iommu_group_set_domain()
>>>> and only gets replaced after the call. This creates a window, during which
>>>> an asynchronous fault path might hit UAF:
>>>>
>>>>     core, holding group->mutex          fault path, no group->mutex
>>>>     ==========================          ===========================
>>>>
>>>>     xa_reserve()                    ,-- old handle stays published
>>>>       no-op, the slot is occupied   |
>>>>     __iommu_group_set_domain()      |
>>>>       driver attach/detach ops      |
>>>>           iopf_queue_flush_dev()    |
>>>>                                     |   iommu_attach_handle_get()
>>>>                                     |     reads old_handle
>>>>     xa_store(new entry)             `-- window closes
>>>>     mutex_unlock()
>>>>     kfree(old_handle)  [caller]
>>>>                                     UAF: old_handle->domain->iopf_handler()
>>>>
>>>> Replace the xa_reserve() with xa_store(XA_ZERO_ENTRY) to unpublish the old
>>>> handle.
>>>>
>>>> Fixes: 8519e689834a ("iommu: Extend domain attach group with handle
>>>> support")
>>>> Cc: stable@vger.kernel.org
>>>> Assisted-by: LLM
>>>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>>>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>
>>> btw I just realized that neither old code nor this fix can actually support
>>> replacement with lossless fault handling, but I don't recall whether it
>>> was the original goal (then destroyed by a following change) or intended
>>> to be a future enhancement...
>>
>> There was a related discussion[1]. With this patch, a page request in
>> the replace window gets IOMMU_PAGE_RESP_INVALID instead of being
>> serviced - spec only says re-issuing the same PRG won't succeed unless
>> the mapping changes, it doesn't forbid retry. So I think this is just a
>> more conservative instance of the failure case Jason already called OK
>> in [1], not a new hazard.
>>
>> [1]
>> https://lore.kernel.org/linux-iommu/20241105151042.GC458827@nvidia.com/
> 
> I called using either the old or new translation OK, using neither
> seems suboptimal. So returning IOMMU_PAGE_RESP_INVALID doesn't sound
> ideal..

I see. maybe we can have a interim state for the replace window. This
state responds PageRequests with success for the device. The device
shall retry DMA and re-trigger PageRequests again. Once the replacement
is done, PageRequests should be forwarded to the correct domain and get
resolved.

Regards,
Yi Liu