Skip to content

Commit 4e034bf

Browse files
committed
iommufd: Fix race during abort for file descriptors
fput() doesn't actually call file_operations release() synchronously, it puts the file on a work queue and it will be released eventually. This is normally fine, except for iommufd the file and the iommufd_object are tied to gether. The file has the object as it's private_data and holds a users refcount, while the object is expected to remain alive as long as the file is. When the allocation of a new object aborts before installing the file it will fput() the file and then go on to immediately kfree() the obj. This causes a UAF once the workqueue completes the fput() and tries to decrement the users refcount. Fix this by putting the core code in charge of the file lifetime, and call __fput_sync() during abort to ensure that release() is called before kfree. __fput_sync() is a bit too tricky to open code in all the object implementations. Instead the objects tell the core code where the file pointer is and the core will take care of the life cycle. If the object is successfully allocated then the file will hold a users refcount and the iommufd_object cannot be destroyed. It is worth noting that close(); ioctl(IOMMU_DESTROY); doesn't have an issue because close() is already using a synchronous version of fput(). The UAF looks like this: BUG: KASAN: slab-use-after-free in iommufd_eventq_fops_release+0x45/0xc0 drivers/iommu/iommufd/eventq.c:376 Write of size 4 at addr ffff888059c97804 by task syz.0.46/6164 CPU: 0 UID: 0 PID: 6164 Comm: syz.0.46 Not tainted syzkaller #0 PREEMPT(full) Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/18/2025 Call Trace: <TASK> __dump_stack lib/dump_stack.c:94 [inline] dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120 print_address_description mm/kasan/report.c:378 [inline] print_report+0xcd/0x630 mm/kasan/report.c:482 kasan_report+0xe0/0x110 mm/kasan/report.c:595 check_region_inline mm/kasan/generic.c:183 [inline] kasan_check_range+0x100/0x1b0 mm/kasan/generic.c:189 instrument_atomic_read_write include/linux/instrumented.h:96 [inline] atomic_fetch_sub_release include/linux/atomic/atomic-instrumented.h:400 [inline] __refcount_dec include/linux/refcount.h:455 [inline] refcount_dec include/linux/refcount.h:476 [inline] iommufd_eventq_fops_release+0x45/0xc0 drivers/iommu/iommufd/eventq.c:376 __fput+0x402/0xb70 fs/file_table.c:468 task_work_run+0x14d/0x240 kernel/task_work.c:227 resume_user_mode_work include/linux/resume_user_mode.h:50 [inline] exit_to_user_mode_loop+0xeb/0x110 kernel/entry/common.c:43 exit_to_user_mode_prepare include/linux/irq-entry-common.h:225 [inline] syscall_exit_to_user_mode_work include/linux/entry-common.h:175 [inline] syscall_exit_to_user_mode include/linux/entry-common.h:210 [inline] do_syscall_64+0x41c/0x4c0 arch/x86/entry/syscall_64.c:100 entry_SYSCALL_64_after_hwframe+0x77/0x7f Link: https://patch.msgid.link/r/[email protected] Cc: [email protected] Fixes: 07838f7 ("iommufd: Add iommufd fault object") Reviewed-by: Nicolin Chen <[email protected]> Reviewed-by: Nirmoy Das <[email protected]> Reviewed-by: Kevin Tian <[email protected]> Tested-by: Nicolin Chen <[email protected]> Reported-by: [email protected] Closes: https://lore.kernel.org/r/[email protected] Signed-off-by: Jason Gunthorpe <[email protected]>
1 parent 7a425ec commit 4e034bf

File tree

2 files changed

+34
-10
lines changed

2 files changed

+34
-10
lines changed

drivers/iommu/iommufd/eventq.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -393,12 +393,12 @@ static int iommufd_eventq_init(struct iommufd_eventq *eventq, char *name,
393393
const struct file_operations *fops)
394394
{
395395
struct file *filep;
396-
int fdno;
397396

398397
spin_lock_init(&eventq->lock);
399398
INIT_LIST_HEAD(&eventq->deliver);
400399
init_waitqueue_head(&eventq->wait_queue);
401400

401+
/* The filep is fput() by the core code during failure */
402402
filep = anon_inode_getfile(name, fops, eventq, O_RDWR);
403403
if (IS_ERR(filep))
404404
return PTR_ERR(filep);
@@ -408,10 +408,7 @@ static int iommufd_eventq_init(struct iommufd_eventq *eventq, char *name,
408408
eventq->filep = filep;
409409
refcount_inc(&eventq->obj.users);
410410

411-
fdno = get_unused_fd_flags(O_CLOEXEC);
412-
if (fdno < 0)
413-
fput(filep);
414-
return fdno;
411+
return get_unused_fd_flags(O_CLOEXEC);
415412
}
416413

417414
static const struct file_operations iommufd_fault_fops =
@@ -452,7 +449,6 @@ int iommufd_fault_alloc(struct iommufd_ucmd *ucmd)
452449
return 0;
453450
out_put_fdno:
454451
put_unused_fd(fdno);
455-
fput(fault->common.filep);
456452
return rc;
457453
}
458454

@@ -536,7 +532,6 @@ int iommufd_veventq_alloc(struct iommufd_ucmd *ucmd)
536532

537533
out_put_fdno:
538534
put_unused_fd(fdno);
539-
fput(veventq->common.filep);
540535
out_abort:
541536
iommufd_object_abort_and_destroy(ucmd->ictx, &veventq->common.obj);
542537
out_unlock_veventqs:

drivers/iommu/iommufd/main.c

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "iommufd_test.h"
2424

2525
struct iommufd_object_ops {
26+
size_t file_offset;
2627
void (*pre_destroy)(struct iommufd_object *obj);
2728
void (*destroy)(struct iommufd_object *obj);
2829
void (*abort)(struct iommufd_object *obj);
@@ -131,10 +132,30 @@ void iommufd_object_abort(struct iommufd_ctx *ictx, struct iommufd_object *obj)
131132
void iommufd_object_abort_and_destroy(struct iommufd_ctx *ictx,
132133
struct iommufd_object *obj)
133134
{
134-
if (iommufd_object_ops[obj->type].abort)
135-
iommufd_object_ops[obj->type].abort(obj);
135+
const struct iommufd_object_ops *ops = &iommufd_object_ops[obj->type];
136+
137+
if (ops->file_offset) {
138+
struct file **filep = ((void *)obj) + ops->file_offset;
139+
140+
/*
141+
* A file should hold a users refcount while the file is open
142+
* and put it back in its release. The file should hold a
143+
* pointer to obj in their private data. Normal fput() is
144+
* deferred to a workqueue and can get out of order with the
145+
* following kfree(obj). Using the sync version ensures the
146+
* release happens immediately. During abort we require the file
147+
* refcount is one at this point - meaning the object alloc
148+
* function cannot do anything to allow another thread to take a
149+
* refcount prior to a guaranteed success.
150+
*/
151+
if (*filep)
152+
__fput_sync(*filep);
153+
}
154+
155+
if (ops->abort)
156+
ops->abort(obj);
136157
else
137-
iommufd_object_ops[obj->type].destroy(obj);
158+
ops->destroy(obj);
138159
iommufd_object_abort(ictx, obj);
139160
}
140161

@@ -659,6 +680,12 @@ void iommufd_ctx_put(struct iommufd_ctx *ictx)
659680
}
660681
EXPORT_SYMBOL_NS_GPL(iommufd_ctx_put, "IOMMUFD");
661682

683+
#define IOMMUFD_FILE_OFFSET(_struct, _filep, _obj) \
684+
.file_offset = (offsetof(_struct, _filep) + \
685+
BUILD_BUG_ON_ZERO(!__same_type( \
686+
struct file *, ((_struct *)NULL)->_filep)) + \
687+
BUILD_BUG_ON_ZERO(offsetof(_struct, _obj)))
688+
662689
static const struct iommufd_object_ops iommufd_object_ops[] = {
663690
[IOMMUFD_OBJ_ACCESS] = {
664691
.destroy = iommufd_access_destroy_object,
@@ -669,6 +696,7 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
669696
},
670697
[IOMMUFD_OBJ_FAULT] = {
671698
.destroy = iommufd_fault_destroy,
699+
IOMMUFD_FILE_OFFSET(struct iommufd_fault, common.filep, common.obj),
672700
},
673701
[IOMMUFD_OBJ_HW_QUEUE] = {
674702
.destroy = iommufd_hw_queue_destroy,
@@ -691,6 +719,7 @@ static const struct iommufd_object_ops iommufd_object_ops[] = {
691719
[IOMMUFD_OBJ_VEVENTQ] = {
692720
.destroy = iommufd_veventq_destroy,
693721
.abort = iommufd_veventq_abort,
722+
IOMMUFD_FILE_OFFSET(struct iommufd_veventq, common.filep, common.obj),
694723
},
695724
[IOMMUFD_OBJ_VIOMMU] = {
696725
.destroy = iommufd_viommu_destroy,

0 commit comments

Comments
 (0)