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(-)
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.
Fixes: 9227da7816dd ("iommufd: Add iommufd_access_change_ioas(_id) helpers") Cc: stable@vger.kernel.org Reported-by: Jason Gunthorpe jgg@nvidia.com Suggested-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/io_pagetable.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c index 504ac1b01b2d..05fd9d3abf1b 100644 --- a/drivers/iommu/iommufd/io_pagetable.c +++ b/drivers/iommu/iommufd/io_pagetable.c @@ -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);
Jason 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.
Fixes: 23a1b46f15d5 ("iommufd/selftest: Make the mock iommu driver into a real driver") Cc: stable@vger.kernel.org Reported-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/selftest.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 8abf9747773e..2bfe77bd351d 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -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;
Jason 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.
Fixes: 9227da7816dd ("iommufd: Add iommufd_access_change_ioas(_id) helpers") Cc: stable@vger.kernel.org Reported-by: Jason Gunthorpe jgg@nvidia.com Signed-off-by: Nicolin Chen nicolinc@nvidia.com --- drivers/iommu/iommufd/selftest.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 2bfe77bd351d..d59e199a8705 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -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)
From: Nicolin Chen nicolinc@nvidia.com Sent: Friday, February 23, 2024 5:24 AM
Jason 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.
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(-)
otherwise all look good to me:
Reviewed-by: Kevin Tian kevin.tian@intel.com
On Tue, Feb 27, 2024 at 03:34:11AM +0000, Tian, Kevin wrote:
From: Nicolin Chen nicolinc@nvidia.com Sent: Friday, February 23, 2024 5:24 AM
Jason 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
On Thu, Feb 22, 2024 at 01:23:44PM -0800, Nicolin Chen wrote:
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(-)
Applied to for-rc
Thanks, Jason
linux-stable-mirror@lists.linaro.org
-
Jason Gunthorpe -
Nicolin Chen -
Tian, Kevin