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

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

[Rajat Asthana]

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 <rajatasthana4@gmail.com>
---
 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

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

[Christoph Hellwig]

This should be fixed properly by:

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

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

[Rajat Asthana]

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

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

[Christoph Hellwig]

On Tue, Jul 06, 2021 at 06:31:11AM +0530, Rajat Asthana wrote:
> 
> 
> 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?

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.

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

[Rajat Asthana]

> 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