[PATCH v1 0/4] iommu: Fix attach handle publication ordering

[PATCH v1 0/4] iommu: Fix attach handle publication ordering

[Nicolin Chen]

Jason pointed out an issue in the attach handle replacing path:
https://lore.kernel.org/linux-iommu/20260923233920.GJ2545495@nvidia.com/

When an attachment is detached or replaced, the IOMMU core can still leave
the outgoing attach handle published while calling into the driver. A fault
report looks up the handle without the group mutex, so it can find the old
handle after the driver's fault flush and just before the caller frees it.

These four patches unpublish the outgoing handle before a driver callback
for both group and PASID detach and replace operations. Both replace paths
store XA_ZERO_ENTRY to reserve the slot and restore the old entry on error.

This blocks new lookups, but does not retire a fault that already obtained
the old handle. The driver must still synchronize those readers before the
handle is freed. A same-domain replacement skips the driver callback, so it
still needs a separate lifetime mechanism or fence.

This is on GitHub:
https://github.com/nicolinc/iommufd/commits/fix_iommu_attach_handle-v1

Nicolin Chen (4):
  iommu: Unpublish the attach handle before the group detach callback
  iommu: Unpublish the attach handle before the PASID detach callback
  iommu: Unpublish the old attach handle before the group replace
    callback
  iommu: Unpublish the old attach handle before the PASID replace
    callback

 drivers/iommu/iommu.c | 54 ++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 19 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.43.0

[PATCH v1 1/4] iommu: Unpublish the attach handle before the group detach callback

[Nicolin Chen]

The group->pasid_array entry is erased only after the detach callback has
returned, so the outgoing attach handle stays published for the whole of
that call. iommu_attach_handle_get() reads it under the xa_lock alone, not
under the group mutex, so a fault raised in the meantime still finds that
handle and the domain being detached, both of which the caller frees the
moment that detach returns.

Commit 5e9f822c9c68 ("iommu: Swap the order of setting group->pasid_array
and calling attach op of iommu drivers") has fixed the attach path, while
the detach path never got the symmetric change.

Erase the entry ahead of the callback instead. A missing entry reads back
as NULL from xa_load(), so that iommu_attach_handle_get() returns -ENOENT
and the fault gets rejected rather than delivered against a domain that is
on its way out.

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>
---
 drivers/iommu/iommu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd1bca7ede9af..a062a84686c29 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3951,8 +3951,12 @@ void iommu_detach_group_handle(struct iommu_domain *domain,
 			       struct iommu_group *group)
 {
 	mutex_lock(&group->mutex);
-	__iommu_group_set_core_domain(group);
+	/*
+	 * Unpublish the handle first, so it would not resolve to the detaching
+	 * domain once the driver is detaching it.
+	 */
 	xa_erase(&group->pasid_array, IOMMU_NO_PASID);
+	__iommu_group_set_core_domain(group);
 	mutex_unlock(&group->mutex);
 }
 EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL");
-- 
2.43.0

[PATCH v1 2/4] iommu: Unpublish the attach handle before the PASID detach callback

[Nicolin Chen]

iommu_detach_device_pasid() erases the group->pasid_array entry only once
__iommu_remove_group_pasid() has returned, so the detaching handle stays
visible to iommu_attach_handle_get() for the whole of that callback, and
the caller frees it as soon as the detach returns.

Erase the entry ahead of the callback, for the same reason the group path
does: a fault raised in the meantime must not resolve to a domain that is
on its way out.

Fixes: 16603704559c ("iommu: Add attach/detach_dev_pasid iommu interfaces")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a062a84686c29..19b880d23314b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3818,8 +3818,12 @@ void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
 	struct iommu_group *group = dev->iommu_group;
 
 	mutex_lock(&group->mutex);
-	__iommu_remove_group_pasid(group, pasid, domain);
+	/*
+	 * Unpublish the handle first, so it would not resolve to the detaching
+	 * domain once the driver is detaching it.
+	 */
 	xa_erase(&group->pasid_array, pasid);
+	__iommu_remove_group_pasid(group, pasid, domain);
 	mutex_unlock(&group->mutex);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device_pasid);
-- 
2.43.0

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

[Nicolin Chen]

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>
---
 drivers/iommu/iommu.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 19b880d23314b..52e59f38cec18 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3985,7 +3985,7 @@ int iommu_replace_group_handle(struct iommu_group *group,
 			       struct iommu_domain *new_domain,
 			       struct iommu_attach_handle *handle)
 {
-	void *curr, *entry;
+	void *curr, *entry, *old;
 	int ret;
 
 	if (!new_domain || !handle)
@@ -3993,22 +3993,25 @@ int iommu_replace_group_handle(struct iommu_group *group,
 
 	mutex_lock(&group->mutex);
 	entry = iommu_make_pasid_array_entry(new_domain, handle);
-	ret = xa_reserve(&group->pasid_array, IOMMU_NO_PASID, GFP_KERNEL);
-	if (ret)
+	/*
+	 * iommu_attach_handle_get() runs without the group mutex, so unpublish
+	 * the old handle before the driver callback: a fault must not resolve
+	 * to either domain while the switch is in progress. This reserves the
+	 * slot too, so the store below cannot fail.
+	 */
+	old = xa_store(&group->pasid_array, IOMMU_NO_PASID, XA_ZERO_ENTRY,
+		       GFP_KERNEL);
+	if (xa_is_err(old)) {
+		ret = xa_err(old);
 		goto err_unlock;
+	}
 
 	ret = __iommu_group_set_domain(group, new_domain);
 	if (ret)
-		goto err_release;
+		entry = old; /* Restore the old handle */
 
 	curr = xa_store(&group->pasid_array, IOMMU_NO_PASID, entry, GFP_KERNEL);
 	WARN_ON(xa_is_err(curr));
-
-	mutex_unlock(&group->mutex);
-
-	return 0;
-err_release:
-	xa_release(&group->pasid_array, IOMMU_NO_PASID);
 err_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.43.0

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

[Nicolin Chen]

Similar to iommu_replace_group_handle(), iommu_replace_device_pasid() has
the same inert reserve as the group path and the UAF window:

  core, holding group->mutex          fault path, no group->mutex
  ==========================          ===========================

  xa_cmpxchg()                    ,-- old handle stays published
    no-op, the slot is occupied   |
  __iommu_set_group_pasid()       |
    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()

Store XA_ZERO_ENTRY outright, just as the group path now does.

Fixes: 8a9e1e773f60 ("iommu: Introduce a replace API for device pasid")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommu.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 52e59f38cec18..2ff81e94a7b7b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3751,8 +3751,13 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
 	}
 
 	entry = iommu_make_pasid_array_entry(domain, handle);
-	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL,
-			  XA_ZERO_ENTRY, GFP_KERNEL);
+	/*
+	 * iommu_attach_handle_get() runs without the group mutex, so unpublish
+	 * the old handle before the driver callback: a fault must not resolve
+	 * to either domain while the switch is in progress. This reserves the
+	 * slot too, so the store below cannot fail.
+	 */
+	curr = xa_store(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
 	if (xa_is_err(curr)) {
 		ret = xa_err(curr);
 		goto out_unlock;
@@ -3776,7 +3781,7 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
 	if (curr == entry) {
 		WARN_ON(1);
 		ret = -EINVAL;
-		goto out_unlock;
+		goto out_store;
 	}
 
 	curr_domain = pasid_array_entry_to_domain(curr);
@@ -3786,16 +3791,16 @@ int iommu_replace_device_pasid(struct iommu_domain *domain,
 		ret = __iommu_set_group_pasid(domain, group,
 					      pasid, curr_domain);
 		if (ret)
-			goto out_unlock;
+			entry = curr; /* restore the old handle */
 	}
 
+out_store:
 	/*
-	 * The above xa_cmpxchg() reserved the memory, and the
-	 * group->mutex is held, this cannot fail.
+	 * The xa_store() above reserved the memory, and the group->mutex
+	 * is held, this cannot fail.
 	 */
 	WARN_ON(xa_is_err(xa_store(&group->pasid_array,
 				   pasid, entry, GFP_KERNEL)));
-
 out_unlock:
 	mutex_unlock(&group->mutex);
 	return ret;
-- 
2.43.0