[PATCH] block: Avoid accessing an already freed kobject in delete_partition

When several processes, which interacts with block devices, are created
and killed continuously, a case happens when the kobject freed by a
process is accessed by another process in delete_partition.

This causes a use-after-free bug and is being reported by syzbot here:
https://syzkaller.appspot.com/bug?extid=7d6c5587ec9cff5be65c

Add a condition to check if the kobject for a device exists, if it
does, then go ahead and refer the kobject, else avoid referencing it.

In this scenario, when more than one processes are interacting with the
block device, one process might have deleted the device from the system
and the other process might try to delete an already deleted device,
which will result in several null pointer dereference errors. So, check
if device has a sysfs entry before calling functions to remove it from
sysfs. We should also check if the knode belongs to any list before
calling the function to remove it from the lists.

Reported-and-tested-by: syzbot+7d6c5587ec9cff5be65c@syzkaller.appspotmail.com
Signed-off-by: Rajat Asthana <redacted>
---
 block/partitions/core.c |  6 +++++-
 drivers/base/core.c     | 21 ++++++++++++++-------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 347c56a51d87..3fc0364c1c35 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -291,7 +291,11 @@ static void delete_partition(struct block_device *part)
 	__invalidate_device(part, true);
 
 	xa_erase(&part->bd_disk->part_tbl, part->bd_partno);
-	kobject_put(part->bd_holder_dir);
+
+	if (!part->bd_holder_dir) {
+		kobject_put(part->bd_holder_dir);
+	}
+
 	device_del(&part->bd_device);
 
 	/*

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a61003ea2c1..d4f43fd0d0ae 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c

@@ -3486,16 +3486,20 @@ void device_del(struct device *dev)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
 
-	dpm_sysfs_remove(dev);
-	if (parent)
+	if (dev->kobj.sd)
+		dpm_sysfs_remove(dev);
+	if (parent && klist_node_attached(&dev->p->knode_parent))
 		klist_del(&dev->p->knode_parent);
 	if (MAJOR(dev->devt)) {
 		devtmpfs_delete_node(dev);
 		device_remove_sys_dev_entry(dev);
-		device_remove_file(dev, &dev_attr_dev);
+
+		if (dev->kobj.sd)
+			device_remove_file(dev, &dev_attr_dev);
 	}
 	if (dev->class) {
-		device_remove_class_symlinks(dev);
+		if (dev->kobj.sd)
+			device_remove_class_symlinks(dev);
 
 		mutex_lock(&dev->class->p->mutex);
 		/* notify any interfaces that the device is now gone */

@@ -3504,11 +3508,14 @@ void device_del(struct device *dev)
 			if (class_intf->remove_dev)
 				class_intf->remove_dev(dev, class_intf);
 		/* remove the device from the class list */
-		klist_del(&dev->p->knode_class);
+		if (klist_node_attached(&dev->p->knode_class))
+			klist_del(&dev->p->knode_class);
 		mutex_unlock(&dev->class->p->mutex);
 	}
-	device_remove_file(dev, &dev_attr_uevent);
-	device_remove_attrs(dev);
+	if (dev->kobj.sd) {
+		device_remove_file(dev, &dev_attr_uevent);
+		device_remove_attrs(dev);
+	}
 	bus_remove_device(dev);
 	device_pm_remove(dev);
 	driver_deferred_probe_del(dev);

-- 
2.32.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

This should be fixed properly by:

"block: check disk exist before trying to add partition"
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

On 03/07/21 10:56 am, Christoph Hellwig wrote:

This should be fixed properly by:

"block: check disk exist before trying to add partition"

Hi Christoph, thanks a lot for suggesting this fix. I have been
working on implementing this and have tried the following:
- I checked if the the kobject of device structure embedded in
   gendisk structure is not NULL, to add the partition.
   This didn't work.

- Then I checked the if kobject of the block_device struct (part0)
   embedded in the gendisk struct is not NULL, to add the partition.
   This also didn't work.

- Then I checked the i_state of the bd_inode field of block_device
   struct embedded in the gendisk struct. I checked if the I_FREEING or
   I_WILL_FREE fields are not set. The reason behind doing this was
   to confirm that we only create partition on the disks which are not
   being freed.

Am I going in the right direction? Can you point me to the correct
direction if I am not?

thanks
-- Rajat
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

On Tue, Jul 06, 2021 at 06:31:11AM +0530, Rajat Asthana wrote:

On 03/07/21 10:56 am, Christoph Hellwig wrote:

quoted

This should be fixed properly by:

"block: check disk exist before trying to add partition"

Hi Christoph, thanks a lot for suggesting this fix. I have been
working on implementing this and have tried the following:
- I checked if the the kobject of device structure embedded in
  gendisk structure is not NULL, to add the partition.
  This didn't work.

- Then I checked the if kobject of the block_device struct (part0)
  embedded in the gendisk struct is not NULL, to add the partition.
  This also didn't work.

- Then I checked the i_state of the bd_inode field of block_device
  struct embedded in the gendisk struct. I checked if the I_FREEING or
  I_WILL_FREE fields are not set. The reason behind doing this was
  to confirm that we only create partition on the disks which are not
  being freed.

Am I going in the right direction? Can you point me to the correct
direction if I am not?

No.  There should be no need to check anything, but the code needs
to ensure that the block device is alive.  I think the above mentioned
patch (now in Jens' tree) does that, so please try it.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

No.  There should be no need to check anything, but the code needs
to ensure that the block device is alive.  I think the above mentioned
patch (now in Jens' tree) does that, so please try it.

Ah! yes, found the patch. I should have checked earlier.

thanks
-- Rajat



_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees