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

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Friday, October 30, 2026 2:46 AM
> 
> 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@xxxxxxxxxx>
> > > > 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@xxxxxxxxxxxxxxx
> > > > Assisted-by: LLM
> > > > Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > >
> > > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx/
> 
> I called using either the old or new translation OK, using neither
> seems suboptimal. So returning IOMMU_PAGE_RESP_INVALID doesn't sound
> ideal..
> 

This probably need a RCU so the old domain can continue to be
referenced by in-fly faults in that window...