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

[Yi Liu]

On 11/1/26 13:53, Baolu Lu wrote:
> On 9/30/2026 5:51 AM, Nicolin Chen wrote:
> > 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@xxxxxxxxxxxxxxx
> > Assisted-by: LLM
> > Signed-off-by: Nicolin Chen<nicolinc@xxxxxxxxxx>
> > ---
> >   drivers/iommu/iommu.c | 23 +++++++++++++----------
> >   1 file changed, 13 insertions(+), 10 deletions(-)
>
> This doesn't fix the same-domain replace issue, which can still result
> in a use-after-free bug. I agree that this needs a separate lifetime
> mechanism as stated in the cover letter. But is it possible to prevent
> the same-domain replace case from going through this interface?

fwiw, iommufd already guards this at the device-replace layer
(iommufd_device_do_replace() short-circuits when hwpt == old_hwpt) [1],
so IOMMUFD callers can't currently trigger this. But that's a 
caller-side convention, not something 
iommu_replace_group_handle()/iommu_replace_device_pasid() enforce 
themselves - so it's still
reachable if a future/other caller doesn't do the same check. Worth
deciding whether to push the guard down into the core API, or document
it as a caller contract.

[1] 
https://github.com/torvalds/linux/blob/6628e7d4c6b25c8d18f1345e3a20d21829aae7e4/drivers/iommu/iommufd/device.c#L888

Regards,
Yi Liu