From: Jason Gunthorpe jgg@nvidia.com
[ Upstream commit 99f98a7c0d6985d5507c8130a981972e4b7b3bdc ]
syzkaller found a race where IOMMUFD_DESTROY increments the refcount:
obj = iommufd_get_object(ucmd->ictx, cmd->id, IOMMUFD_OBJ_ANY); if (IS_ERR(obj)) return PTR_ERR(obj); iommufd_ref_to_users(obj); /* See iommufd_ref_to_users() */ if (!iommufd_object_destroy_user(ucmd->ictx, obj))
As part of the sequence to join the two existing primitives together.
Allowing the refcount the be elevated without holding the destroy_rwsem violates the assumption that all temporary refcount elevations are protected by destroy_rwsem. Racing IOMMUFD_DESTROY with iommufd_object_destroy_user() will cause spurious failures:
WARNING: CPU: 0 PID: 3076 at drivers/iommu/iommufd/device.c:477 iommufd_access_destroy+0x18/0x20 drivers/iommu/iommufd/device.c:478 Modules linked in: CPU: 0 PID: 3076 Comm: syz-executor.0 Not tainted 6.3.0-rc1-syzkaller #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/03/2023 RIP: 0010:iommufd_access_destroy+0x18/0x20 drivers/iommu/iommufd/device.c:477 Code: e8 3d 4e 00 00 84 c0 74 01 c3 0f 0b c3 0f 1f 44 00 00 f3 0f 1e fa 48 89 fe 48 8b bf a8 00 00 00 e8 1d 4e 00 00 84 c0 74 01 c3 <0f> 0b c3 0f 1f 44 00 00 41 57 41 56 41 55 4c 8d ae d0 00 00 00 41 RSP: 0018:ffffc90003067e08 EFLAGS: 00010246 RAX: 0000000000000000 RBX: ffff888109ea0300 RCX: 0000000000000000 RDX: 0000000000000001 RSI: 0000000000000000 RDI: 00000000ffffffff RBP: 0000000000000004 R08: 0000000000000000 R09: ffff88810bbb3500 R10: ffff88810bbb3e48 R11: 0000000000000000 R12: ffffc90003067e88 R13: ffffc90003067ea8 R14: ffff888101249800 R15: 00000000fffffffe FS: 00007ff7254fe6c0(0000) GS:ffff888237c00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000555557262da8 CR3: 000000010a6fd000 CR4: 0000000000350ef0 Call Trace: <TASK> iommufd_test_create_access drivers/iommu/iommufd/selftest.c:596 [inline] iommufd_test+0x71c/0xcf0 drivers/iommu/iommufd/selftest.c:813 iommufd_fops_ioctl+0x10f/0x1b0 drivers/iommu/iommufd/main.c:337 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline] __se_sys_ioctl fs/ioctl.c:856 [inline] __x64_sys_ioctl+0x84/0xc0 fs/ioctl.c:856 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x38/0x80 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd
The solution is to not increment the refcount on the IOMMUFD_DESTROY path at all. Instead use the xa_lock to serialize everything. The refcount check == 1 and xa_erase can be done under a single critical region. This avoids the need for any refcount incrementing.
It has the downside that if userspace races destroy with other operations it will get an EBUSY instead of waiting, but this is kind of racing is already dangerous.
Fixes: 2ff4bed7fee7 ("iommufd: File descriptor, context, kconfig and makefiles") Link: https://lore.kernel.org/r/2-v1-85aacb2af554+bc-iommufd_syz3_jgg@nvidia.com Reviewed-by: Kevin Tian kevin.tian@intel.com Reported-by: syzbot+7574ebfe589049630608@syzkaller.appspotmail.com Signed-off-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Sasha Levin sashal@kernel.org --- drivers/iommu/iommufd/device.c | 12 +--- drivers/iommu/iommufd/iommufd_private.h | 15 ++++- drivers/iommu/iommufd/main.c | 78 +++++++++++++++++++------ 3 files changed, 75 insertions(+), 30 deletions(-)
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c index 29d05663d4d17..ed2937a4e196f 100644 --- a/drivers/iommu/iommufd/device.c +++ b/drivers/iommu/iommufd/device.c @@ -109,10 +109,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_device_bind, IOMMUFD); */ void iommufd_device_unbind(struct iommufd_device *idev) { - bool was_destroyed; - - was_destroyed = iommufd_object_destroy_user(idev->ictx, &idev->obj); - WARN_ON(!was_destroyed); + iommufd_object_destroy_user(idev->ictx, &idev->obj); } EXPORT_SYMBOL_NS_GPL(iommufd_device_unbind, IOMMUFD);
@@ -382,7 +379,7 @@ void iommufd_device_detach(struct iommufd_device *idev) mutex_unlock(&hwpt->devices_lock);
if (hwpt->auto_domain) - iommufd_object_destroy_user(idev->ictx, &hwpt->obj); + iommufd_object_deref_user(idev->ictx, &hwpt->obj); else refcount_dec(&hwpt->obj.users);
@@ -456,10 +453,7 @@ EXPORT_SYMBOL_NS_GPL(iommufd_access_create, IOMMUFD); */ void iommufd_access_destroy(struct iommufd_access *access) { - bool was_destroyed; - - was_destroyed = iommufd_object_destroy_user(access->ictx, &access->obj); - WARN_ON(!was_destroyed); + iommufd_object_destroy_user(access->ictx, &access->obj); } EXPORT_SYMBOL_NS_GPL(iommufd_access_destroy, IOMMUFD);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index b38e67d1988bd..f9790983699ce 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -176,8 +176,19 @@ void iommufd_object_abort_and_destroy(struct iommufd_ctx *ictx, struct iommufd_object *obj); void iommufd_object_finalize(struct iommufd_ctx *ictx, struct iommufd_object *obj); -bool iommufd_object_destroy_user(struct iommufd_ctx *ictx, - struct iommufd_object *obj); +void __iommufd_object_destroy_user(struct iommufd_ctx *ictx, + struct iommufd_object *obj, bool allow_fail); +static inline void iommufd_object_destroy_user(struct iommufd_ctx *ictx, + struct iommufd_object *obj) +{ + __iommufd_object_destroy_user(ictx, obj, false); +} +static inline void iommufd_object_deref_user(struct iommufd_ctx *ictx, + struct iommufd_object *obj) +{ + __iommufd_object_destroy_user(ictx, obj, true); +} + struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, size_t size, enum iommufd_object_type type); diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 3fbe636c3d8a6..4cf5f73f27084 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -116,14 +116,56 @@ struct iommufd_object *iommufd_get_object(struct iommufd_ctx *ictx, u32 id, return obj; }
+/* + * Remove the given object id from the xarray if the only reference to the + * object is held by the xarray. The caller must call ops destroy(). + */ +static struct iommufd_object *iommufd_object_remove(struct iommufd_ctx *ictx, + u32 id, bool extra_put) +{ + struct iommufd_object *obj; + XA_STATE(xas, &ictx->objects, id); + + xa_lock(&ictx->objects); + obj = xas_load(&xas); + if (xa_is_zero(obj) || !obj) { + obj = ERR_PTR(-ENOENT); + goto out_xa; + } + + /* + * If the caller is holding a ref on obj we put it here under the + * spinlock. + */ + if (extra_put) + refcount_dec(&obj->users); + + if (!refcount_dec_if_one(&obj->users)) { + obj = ERR_PTR(-EBUSY); + goto out_xa; + } + + xas_store(&xas, NULL); + if (ictx->vfio_ioas == container_of(obj, struct iommufd_ioas, obj)) + ictx->vfio_ioas = NULL; + +out_xa: + xa_unlock(&ictx->objects); + + /* The returned object reference count is zero */ + return obj; +} + /* * The caller holds a users refcount and wants to destroy the object. Returns * true if the object was destroyed. In all cases the caller no longer has a * reference on obj. */ -bool iommufd_object_destroy_user(struct iommufd_ctx *ictx, - struct iommufd_object *obj) +void __iommufd_object_destroy_user(struct iommufd_ctx *ictx, + struct iommufd_object *obj, bool allow_fail) { + struct iommufd_object *ret; + /* * The purpose of the destroy_rwsem is to ensure deterministic * destruction of objects used by external drivers and destroyed by this @@ -131,22 +173,22 @@ bool iommufd_object_destroy_user(struct iommufd_ctx *ictx, * side of this, such as during ioctl execution. */ down_write(&obj->destroy_rwsem); - xa_lock(&ictx->objects); - refcount_dec(&obj->users); - if (!refcount_dec_if_one(&obj->users)) { - xa_unlock(&ictx->objects); - up_write(&obj->destroy_rwsem); - return false; - } - __xa_erase(&ictx->objects, obj->id); - if (ictx->vfio_ioas && &ictx->vfio_ioas->obj == obj) - ictx->vfio_ioas = NULL; - xa_unlock(&ictx->objects); + ret = iommufd_object_remove(ictx, obj->id, true); up_write(&obj->destroy_rwsem);
+ if (allow_fail && IS_ERR(ret)) + return; + + /* + * If there is a bug and we couldn't destroy the object then we did put + * back the caller's refcount and will eventually try to free it again + * during close. + */ + if (WARN_ON(IS_ERR(ret))) + return; + iommufd_object_ops[obj->type].destroy(obj); kfree(obj); - return true; }
static int iommufd_destroy(struct iommufd_ucmd *ucmd) @@ -154,13 +196,11 @@ static int iommufd_destroy(struct iommufd_ucmd *ucmd) struct iommu_destroy *cmd = ucmd->cmd; struct iommufd_object *obj;
- obj = iommufd_get_object(ucmd->ictx, cmd->id, IOMMUFD_OBJ_ANY); + obj = iommufd_object_remove(ucmd->ictx, cmd->id, false); if (IS_ERR(obj)) return PTR_ERR(obj); - iommufd_ref_to_users(obj); - /* See iommufd_ref_to_users() */ - if (!iommufd_object_destroy_user(ucmd->ictx, obj)) - return -EBUSY; + iommufd_object_ops[obj->type].destroy(obj); + kfree(obj); return 0; }