iommufd: Fix three bugs found by Syzkaller
Subsystems (3)
cover iommufd: Fix three bugs found by Syzkaller
Jason has been running Syzkaller that found three bugs.
Fix them properly.
Nicolin Chen (3):
iommufd: Fix iopt_access_list_id overwrite bug iommufd/selftest: Fix mock_dev_num bug
iommufd: Fix protection fault in iommufd_test_syz_conv_iova
drivers/iommu/iommufd/io_pagetable.c | 9 ++++--- drivers/iommu/iommufd/selftest.c | 40 +++++++++++++++++++++------- 2 files changed, 36 insertions(+), 13 deletions(-)
Signature · 1 line
2.43.0
PATCH 1/3 iommufd: Fix iopt_access_list_id overwrite bug
Jason has been running Syzkaller that reported the following WARN_ON:
WARNING: CPU: 1 PID: 4738 at drivers/iommu/iommufd/io_pagetable.c:1360
that is a bug of mismatched access pointers.
Call Trace: iommufd_access_change_ioas+0x2fe/0x4e0 iommufd_access_destroy_object+0x50/0xb0 iommufd_object_remove+0x2a3/0x490 iommufd_object_destroy_user iommufd_access_destroy+0x71/0xb0 iommufd_test_staccess_release+0x89/0xd0 __fput+0x272/0xb50 __fput_sync+0x4b/0x60 __do_sys_close __se_sys_close __x64_sys_close+0x8b/0x110 do_syscall_x64
The mismatch between the access pointer in the list and the passed-in pointer is resulted from an overwrite at access->iopt_access_list_id, in iopt_add_access(), called from iommufd_access_change_ioas(), when xa_alloc() succeeds while iopt_calculate_iova_alignment() fails.
Add a new_id in iopt_add_access() and only update iopt_access_list_id when returning successfully.
--- drivers/iommu/iommufd/io_pagetable.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/iommu/iommufd/io_pagetable.c +6 -3
@@ -1330,20 +1330,23 @@ int iopt_disable_large_pages(struct io_pagetable *iopt)int iopt_add_access (struct io_pagetable *iopt,struct iommufd_access *access) { +u32 new_id;int rc;down_write (&iopt->domains_rwsem);down_write (&iopt->iova_rwsem); - rc =xa_alloc (&iopt->access_list, &access->iopt_access_list_id, access, - xa_limit_16b,GFP_KERNEL_ACCOUNT ); + rc =xa_alloc (&iopt->access_list, &new_id, access, xa_limit_16b, +GFP_KERNEL_ACCOUNT ); +if (rc) goto out_unlock; rc =iopt_calculate_iova_alignment (iopt);if (rc) { -xa_erase (&iopt->access_list, access->iopt_access_list_id); +xa_erase (&iopt->access_list, new_id); goto out_unlock; } + access->iopt_access_list_id = new_id; out_unlock:up_write (&iopt->iova_rwsem);
Signature · 1 line
2.43.0
PATCH 2/3 iommufd/selftest: Fix mock_dev_num bug
In reply to Nicolin ChenJason has been running Syzkaller that reported the following bug:
sysfs: cannot create duplicate filename '/devices/iommufd_mock4'
Call Trace: sysfs_warn_dup+0x71/0x90 sysfs_create_dir_ns+0x1ee/0x260 ? sysfs_create_mount_point+0x80/0x80 ? spin_bug+0x1d0/0x1d0 ? do_raw_spin_unlock+0x54/0x220 kobject_add_internal+0x221/0x970 kobject_add+0x11c/0x1e0 ? lockdep_hardirqs_on_prepare+0x273/0x3e0 ? kset_create_and_add+0x160/0x160 ? kobject_put+0x5d/0x390 ? bus_get_dev_root+0x4a/0x60 ? kobject_put+0x5d/0x390 device_add+0x1d5/0x1550 ? __fw_devlink_link_to_consumers.isra.0+0x1f0/0x1f0 ? __init_waitqueue_head+0xcb/0x150 iommufd_test+0x462/0x3b60 ? lock_release+0x1fe/0x640 ? __might_fault+0x117/0x170 ? reacquire_held_locks+0x4b0/0x4b0 ? iommufd_selftest_destroy+0xd0/0xd0 ? __might_fault+0xbe/0x170 iommufd_fops_ioctl+0x256/0x350 ? iommufd_option+0x180/0x180 ? __lock_acquire+0x1755/0x45f0 __x64_sys_ioctl+0xa13/0x1640
The bug is triggered when Syzkaller created multiple mock devices but didn't destroy them in the same sequence, messing up the mock_dev_num counter. So, fix it with a mock_dev_ida.
--- drivers/iommu/iommufd/selftest.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/iommu/iommufd/selftest.c +9 -4
@@ -36,7 +36,7 @@ static struct mock_bus_type iommufd_mock_bus_type = { }, }; -static atomic_t mock_dev_num; +static DEFINE_IDA (mock_dev_ida);enum {MOCK_DIRTY_TRACK =1 ,
@@ -123,6 +123,7 @@ enum selftest_obj_type {struct mock_dev {struct device dev;unsigned long flags; +int id; };struct selftest_obj {
@@ -631,7 +632,7 @@ static void mock_dev_release(struct device *dev) { struct mock_dev *mdev = container_of(dev, struct mock_dev, dev); -atomic_dec (&mock_dev_num); +ida_free (&mock_dev_ida, mdev->id);kfree (mdev); }
@@ -653,8 +654,12 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags) mdev->dev.release = mock_dev_release; mdev->dev.bus = &iommufd_mock_bus_type.bus; - rc =dev_set_name (&mdev->dev,"iommufd_mock%u" , -atomic_inc_return (&mock_dev_num)); + rc =ida_alloc (&mock_dev_ida,GFP_KERNEL ); +if (rc <0 ) + goto err_put; + mdev->id = rc; + + rc =dev_set_name (&mdev->dev,"iommufd_mock%u" , mdev->id);if (rc) goto err_put;
Signature · 1 line
2.43.0
PATCH 3/3 iommufd: Fix protection fault in iommufd_test_syz_conv_iova
In reply to Nicolin ChenJason has been running Syzkaller that reported the following bug:
general protection fault, probably for non-canonical address 0xdffffc0000000038: 0000 [#1] SMP KASAN KASAN: null-ptr-deref in range [0x00000000000001c0-0x00000000000001c7] Call Trace: lock_acquire lock_acquire+0x1ce/0x4f0 down_read+0x93/0x4a0 iommufd_test_syz_conv_iova+0x56/0x1f0 iommufd_test_access_rw.isra.0+0x2ec/0x390 iommufd_test+0x1058/0x1e30 iommufd_fops_ioctl+0x381/0x510 vfs_ioctl __do_sys_ioctl __se_sys_ioctl __x64_sys_ioctl+0x170/0x1e0 do_syscall_x64 do_syscall_64+0x71/0x140
This is because the new iommufd_access_change_ioas() sets access->ioas to NULL during its process, so the lock might be gone in a concurrent racing context.
Fix this by doing the same access->ioas sanity as iommufd_access_rw and iommufd_access_pin_pages functions do.
--- drivers/iommu/iommufd/selftest.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
drivers/iommu/iommufd/selftest.c +21 -6
@@ -63,8 +63,8 @@ enum {* In syzkaller mode the 64 bit IOVA is converted into an nth area and offset * value. This has a much smaller randomization space and syzkaller can hit it. */ -static unsigned long iommufd_test_syz_conv_iova (struct io_pagetable *iopt, -u64 *iova) +static unsigned long __iommufd_test_syz_conv_iova (struct io_pagetable *iopt, +u64 *iova) {struct syz_layout {__u32 nth_area;
@@ -88,6 +88,21 @@ static unsigned long iommufd_test_syz_conv_iova(struct io_pagetable *iopt,return 0 ; } +static unsigned long iommufd_test_syz_conv_iova (struct iommufd_access *access, +u64 *iova) +{ +unsigned long ret; + +mutex_lock (&access->ioas_lock); +if (!access->ioas) { +mutex_unlock (&access->ioas_lock); +return 0 ; + } + ret =__iommufd_test_syz_conv_iova (&access->ioas->iopt, iova); +mutex_unlock (&access->ioas_lock); +return ret; +} +void iommufd_test_syz_conv_iova_id (struct iommufd_ucmd *ucmd,unsigned int ioas_id,u64 *iova,u32 *flags) {
@@ -100,7 +115,7 @@ void iommufd_test_syz_conv_iova_id(struct iommufd_ucmd *ucmd, ioas =iommufd_get_ioas (ucmd->ictx, ioas_id);if (IS_ERR (ioas))return ; - *iova =iommufd_test_syz_conv_iova (&ioas->iopt, iova); + *iova =__iommufd_test_syz_conv_iova (&ioas->iopt, iova);iommufd_put_object (ucmd->ictx, &ioas->obj); }
@@ -1161,7 +1176,7 @@ static int iommufd_test_access_pages(struct iommufd_ucmd *ucmd, }if (flags &MOCK_FLAGS_ACCESS_SYZ ) - iova =iommufd_test_syz_conv_iova (&staccess->access->ioas->iopt, + iova =iommufd_test_syz_conv_iova (staccess->access, &cmd->access_pages.iova); npages = (ALIGN(iova + length, PAGE_SIZE) -
@@ -1263,8 +1278,8 @@ static int iommufd_test_access_rw(struct iommufd_ucmd *ucmd, }if (flags &MOCK_FLAGS_ACCESS_SYZ ) - iova =iommufd_test_syz_conv_iova (&staccess->access->ioas->iopt, - &cmd->access_rw.iova); + iova =iommufd_test_syz_conv_iova (staccess->access, + &cmd->access_rw.iova); rc =iommufd_access_rw (staccess->access, iova, tmp, length, flags);if (rc)
Signature · 1 line
2.43.0
Quoted 4 lines
From: Nicolin Chen <[email protected]>
Sent: Friday, February 23, 2024 5:24 AMJason has been running Syzkaller that found three bugs.
could you remove "Jason has been running" from all three patches? Just say that Syzkaller found a bug.
Quoted 12 lines
Fix them properly.
Nicolin Chen (3):
iommufd: Fixiopt_access_list_idoverwrite bug iommufd/selftest: Fixmock_dev_numbug
iommufd: Fix protection fault iniommufd_test_syz_conv_iovadrivers/iommu/iommufd/io_pagetable.c | 9 ++++--- drivers/iommu/iommufd/selftest.c | 40 +++++++++++++++++++++------- 2 files changed, 36 insertions(+), 13 deletions(-)
otherwise all look good to me:
Jason Gunthorpe
maintainerQuoted 7 lines · Kevin
Quoted 4 lines
From: Nicolin Chen <[email protected]>
Sent: Friday, February 23, 2024 5:24 AMJason has been running Syzkaller that found three bugs.
could you remove "Jason has been running" from all three patches? Just say that Syzkaller found a bug.
I will copy edit the commit messages
Thanks,
Jason
Quoted 4 lines · Nicolin Chen
Jason has been running Syzkaller that found three bugs.
Fix them properly.
Quoted 8 lines
Nicolin Chen (3):
iommufd: Fixiopt_access_list_idoverwrite bug iommufd/selftest: Fixmock_dev_numbug
iommufd: Fix protection fault iniommufd_test_syz_conv_iovadrivers/iommu/iommufd/io_pagetable.c | 9 ++++--- drivers/iommu/iommufd/selftest.c | 40 +++++++++++++++++++++------- 2 files changed, 36 insertions(+), 13 deletions(-)
Applied to for-rc
Thanks,
Jason