iommu: Fix attach handle publication ordering
v2 cover iommu: Fix attach handle publication ordering
Jason pointed out an issue in the attach handle replacing path:
https://lore.kernel.org/linux-iommu/[email protected]/
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.
Patches 1-4 unpublish the outgoing handle before calling into the driver, 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.
Patch 5 rejects a same-domain replace in both of the replace APIs. Such a replace skips the driver callback. So, nothing would ever flush the pending faults before the caller frees the old handle.
This is on GitHub:
https://github.com/nicolinc/iommufd/commits/fix_iommu_attach_handle-v2
Changelog
v2
* Rebase on v7.4-rc1
* Drop "Assisted-by: LLM" tags
* Add Reviewed-by from Kevin, Yi and Baolu
* New patch: reject a same-domain replace in the attach handle APIs
v1
https://lore.kernel.org/linux-iommu/[email protected]/
Nicolin Chen (5):
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
iommu: Reject a same-domain replace in the attach handle APIs
drivers/iommu/iommu.c | 86 ++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 25 deletions(-)
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
Signature · 1 line
2.43.0
PATCH v2 1/5 iommu: Unpublish the attach handle before the group detach callback
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.
--- drivers/iommu/iommu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/iommu/iommu.c +5 -1
@@ -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" );
Signature · 1 line
2.43.0
PATCH v2 2/5 iommu: Unpublish the attach handle before the PASID detach callback
In reply to Nicolin Cheniommu_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.
--- drivers/iommu/iommu.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/iommu/iommu.c +5 -1
@@ -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);
Signature · 1 line
2.43.0
PATCH v2 3/5 iommu: Unpublish the old attach handle before the group replace callback
In reply to Nicolin Cheniommu_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.
--- drivers/iommu/iommu.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)
drivers/iommu/iommu.c +13 -10
@@ -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;
Signature · 1 line
2.43.0
PATCH v2 4/5 iommu: Unpublish the old attach handle before the PASID replace callback
In reply to Nicolin ChenSimilar 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.
--- drivers/iommu/iommu.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
drivers/iommu/iommu.c +12 -7
@@ -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;
Signature · 1 line
2.43.0
PATCH v2 5/5 iommu: Reject a same-domain replace in the attach handle APIs
In reply to Nicolin ChenBoth iommu_replace_group_handle() and iommu_replace_device_pasid() skip the driver callback when the new domain is the one currently attached. In that case, nothing flushes the pending faults that may still be holding the old handle. Then, the caller frees the old handle once the replace returns, so such a fault would hit a use-after-free.
No caller does this today, since iommufd_device_do_replace() returns early when the new hwpt is the old one. However, it is only a caller convention, which neither API enforces.
Reject a same-domain replace in both APIs with a WARN_ON, and document it in their kernel-docs.
--- drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
drivers/iommu/iommu.c +27 -7
@@ -3716,6 +3716,10 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);* Caller should always provide a new handle to avoid race with the paths * that have lockless reference to handle if it intends to pass a valid handle. * +* The @domain must not be the domain currently attached to the @pasid. Such a +* same-domain replace would skip the driver callback flushing pending faults. +* Then, those faults could use the old handle after the caller frees it. +* * Return 0 on success, or an error. */ int iommu_replace_device_pasid(struct iommu_domain *domain,
@@ -3785,15 +3789,17 @@ int iommu_replace_device_pasid(struct iommu_domain *domain, } curr_domain =pasid_array_entry_to_domain (curr); - ret =0 ; - -if (curr_domain != domain) { - ret =__iommu_set_group_pasid (domain, group, - pasid, curr_domain); -if (ret) - entry = curr;/* restore the old handle */ +/* Same as iommu_replace_group_handle(), reject a same-domain replace */ +if (WARN_ON (curr_domain == domain)) { + entry = curr;/* restore the old handle */ + ret = -EINVAL ; + goto out_store; } + ret =__iommu_set_group_pasid (domain, group, pasid, curr_domain); +if (ret) + entry = curr;/* restore the old handle */ + out_store:/* * The xa_store() above reserved the memory, and the group->mutex
@@ -3985,6 +3991,10 @@ EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL");* * Caller should always provide a new handle to avoid race with the paths * that have lockless reference to handle. +* +* The @new_domain must not be the domain currently attached to the group. Such +* a same-domain replace would skip the driver callback flushing pending faults. +* Then, those faults could use the old handle after the caller frees it. */ int iommu_replace_group_handle(struct iommu_group *group, struct iommu_domain *new_domain,
@@ -3997,6 +4007,16 @@ int iommu_replace_group_handle(struct iommu_group *group,return -EINVAL ;mutex_lock (&group->mutex); +/* +* A same-domain replace would skip the driver callback flushing pending +* faults. Then, those faults could use the old handle after the caller +* frees it. +*/ +if (WARN_ON (group->domain == new_domain)) { + ret = -EINVAL ; + goto err_unlock; + } + entry =iommu_make_pasid_array_entry (new_domain, handle);/* * iommu_attach_handle_get() runs without the group mutex, so unpublish
Signature · 1 line
2.43.0
Re: [PATCH v2 5/5] iommu: Reject a same-domain replace in the attach handle APIs
Quoted 24 lines · Nicolin Chen
Both
iommu_replace_group_handle()andiommu_replace_device_pasid()skip the driver callback when the new domain is the one currently attached. In that case, nothing flushes the pending faults that may still be holding the old handle. Then, the caller frees the old handle once the replace returns, so such a fault would hit a use-after-free.No caller does this today, since
iommufd_device_do_replace()returns early when the new hwpt is the old one. However, it is only a caller convention, which neither API enforces.Reject a same-domain replace in both APIs with a
WARN_ON, and document it in their kernel-docs.Suggested-by Lu Baolu <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/iommu.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)drivers/iommu/iommu.c +0
-0
drivers/iommu/iommu.c EXPORT_SYMBOL_GPL(iommu_attach_device_pasid); · line 3722
Quoted 7 lines
drivers/iommu/iommu.c +4
-0@@ -3716,6 +3716,10 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);* Caller should always provide a new handle to avoid race with the paths * that have lockless reference to handle if it intends to pass a valid handle. * +* The @domain must not be the domain currently attached to the @pasid. Such a +* same-domain replace would skip the driver callback flushing pending faults. +* Then, those faults could use the old handle after the caller frees it. +*
Not a native speaker, but this reads a bit ambiguous to me:
I parsed the last sentence as describing intended/acceptable behavior rather than the hazard being guarded against - "Then, ... could use" reads like a neutral continuation rather than a warning. Might be worth making the use-after-free explicit, e.g.:
A same-domain replace would skip the driver callback that flushes pending faults - this is a use-after-free hazard: a fault still holding the old handle could dereference it after the caller frees it.
Quoted 29 lines
drivers/iommu/iommu.c +9
-7* Return 0 on success, or an error. */ int iommu_replace_device_pasid(struct iommu_domain *domain,@@ -3785,15 +3789,17 @@ int iommu_replace_device_pasid(struct iommu_domain *domain, } curr_domain =pasid_array_entry_to_domain (curr); - ret =0 ; - -if (curr_domain != domain) { - ret =__iommu_set_group_pasid (domain, group, - pasid, curr_domain); -if (ret) - entry = curr;/* restore the old handle */ +/* Same as iommu_replace_group_handle(), reject a same-domain replace */ +if (WARN_ON (curr_domain == domain)) { + entry = curr;/* restore the old handle */ + ret = -EINVAL ; + goto out_store; } + ret =__iommu_set_group_pasid (domain, group, pasid, curr_domain); +if (ret) + entry = curr;/* restore the old handle */ + out_store:/* * The xa_store() above reserved the memory, and the group->mutex
drivers/iommu/iommu.c EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL"); · line 3997
Quoted 7 lines
drivers/iommu/iommu.c +4
-0@@ -3985,6 +3991,10 @@ EXPORT_SYMBOL_NS_GPL(iommu_detach_group_handle, "IOMMUFD_INTERNAL");* * Caller should always provide a new handle to avoid race with the paths * that have lockless reference to handle. +* +* The @new_domain must not be the domain currently attached to the group. Such +* a same-domain replace would skip the driver callback flushing pending faults. +* Then, those faults could use the old handle after the caller frees it.
ditto. Otherwise looks good.
drivers/iommu/iommu.c int iommu_replace_group_handle(struct iommu_group *group, · line 4013
Quoted 11 lines
drivers/iommu/iommu.c +4
-0*/ int iommu_replace_group_handle(struct iommu_group *group, struct iommu_domain *new_domain,@@ -3997,6 +4007,16 @@ int iommu_replace_group_handle(struct iommu_group *group,return -EINVAL ;mutex_lock (&group->mutex); +/* +* A same-domain replace would skip the driver callback flushing pending +* faults. Then, those faults could use the old handle after the caller +* frees it.
ditto.
Otherwise looks good.
Quoted 9 lines
drivers/iommu/iommu.c +6
-0+*/ +if (WARN_ON (group->domain == new_domain)) { + ret = -EINVAL ; + goto err_unlock; + } + entry =iommu_make_pasid_array_entry (new_domain, handle);/* * iommu_attach_handle_get() runs without the group mutex, so unpublish
Re: [PATCH v2 5/5] iommu: Reject a same-domain replace in the attach handle APIs
Quoted 13 lines · Yi Liu
drivers/iommu/iommu.c EXPORT_SYMBOL_GPL(iommu_attach_device_pasid); · line 3722
Quoted 8 lines · Nicolin Chen
drivers/iommu/iommu.c +4
-0@@ -3716,6 +3716,10 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);* Caller should always provide a new handle to avoid race with the paths * that have lockless reference to handle if it intends to pass a valid handle. * +* The @domain must not be the domain currently attached to the @pasid. Such a +* same-domain replace would skip the driver callback flushing pending faults. +* Then, those faults could use the old handle after the caller frees it. +* Not a native speaker, but this reads a bit ambiguous to me:
Quoted 8 lines
I parsed the last sentence as describing intended/acceptable behavior rather than the hazard being guarded against - "Then, ... could use" reads like a neutral continuation rather than a warning. Might be worth making the use-after-free explicit, e.g.:
A same-domain replace would skip the driver callback that flushes pending faults - this is a use-after-free hazard: a fault still holding the old handle could dereference it after the caller frees it.
OK. I will respin a v3.
Nicolin