[PATCH rc 0/3] iommufd: Fix three bugs found by Syzkaller

[PATCH rc 0/3] iommufd: Fix three bugs found by Syzkaller

[Nicolin Chen]

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(-)

-- 
2.43.0

[PATCH rc 1/3] iommufd: Fix iopt_access_list_id overwrite bug

[Nicolin Chen]

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);
-- 
2.43.0

[PATCH rc 2/3] iommufd/selftest: Fix mock_dev_num bug

[Nicolin Chen]

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;
 
-- 
2.43.0

[PATCH rc 3/3] iommufd: Fix protection fault in iommufd_test_syz_conv_iova

[Nicolin Chen]

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)
-- 
2.43.0

RE: [PATCH rc 0/3] iommufd: Fix three bugs found by Syzkaller

[Tian, Kevin]

> 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>

Re: [PATCH rc 0/3] iommufd: Fix three bugs found by Syzkaller

[Jason Gunthorpe]

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

Re: [PATCH rc 0/3] iommufd: Fix three bugs found by Syzkaller

[Jason Gunthorpe]

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