[PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[Baokun Li]

Hulk Robot reported a KASAN report about use-after-free:
 ==================================================================
 BUG: KASAN: use-after-free in __list_del_entry_valid+0x13d/0x160
 Read of size 8 at addr ffff888035e37d98 by task ubiattach/1385
 [...]
 Call Trace:
  klist_dec_and_del+0xa7/0x4a0
  klist_put+0xc7/0x1a0
  device_del+0x4d4/0xed0
  cdev_device_del+0x1a/0x80
  ubi_attach_mtd_dev+0x2951/0x34b0 [ubi]
  ctrl_cdev_ioctl+0x286/0x2f0 [ubi]

 Allocated by task 1414:
  device_add+0x60a/0x18b0
  cdev_device_add+0x103/0x170
  ubi_create_volume+0x1118/0x1a10 [ubi]
  ubi_cdev_ioctl+0xb7f/0x1ba0 [ubi]

 Freed by task 1385:
  cdev_device_del+0x1a/0x80
  ubi_remove_volume+0x438/0x6c0 [ubi]
  ubi_cdev_ioctl+0xbf4/0x1ba0 [ubi]
 [...]
 ==================================================================

The lock held by ctrl_cdev_ioctl is ubi_devices_mutex, but the lock held
by ubi_cdev_ioctl is ubi->device_mutex. Therefore, the two locks can be
concurrent.

ctrl_cdev_ioctl contains two operations: ubi_attach and ubi_detach.
ubi_detach is bug-free because it uses reference counting to prevent
concurrency. However, uif_init and uif_close in ubi_attach may race with
ubi_cdev_ioctl.

uif_init will race with ubi_cdev_ioctl as in the following stack.
           cpu1                   cpu2                  cpu3
_______________________|________________________|______________________
ctrl_cdev_ioctl
 ubi_attach_mtd_dev
  uif_init
                           ubi_cdev_ioctl
                            ubi_create_volume
                             cdev_device_add
   ubi_add_volume
   // sysfs exist
   kill_volumes
                                                    ubi_cdev_ioctl
                                                     ubi_remove_volume
                                                      cdev_device_del
                                                       // first free
    ubi_free_volume
     cdev_del
     // double free
   cdev_device_del

And uif_close will race with ubi_cdev_ioctl as in the following stack.
           cpu1                   cpu2                  cpu3
_______________________|________________________|______________________
ctrl_cdev_ioctl
 ubi_attach_mtd_dev
  uif_init
                           ubi_cdev_ioctl
                            ubi_create_volume
                             cdev_device_add
  ubi_debugfs_init_dev
  //error goto out_uif;
  uif_close
   kill_volumes
                                                    ubi_cdev_ioctl
                                                     ubi_remove_volume
                                                      cdev_device_del
                                                       // first free
    ubi_free_volume
    // double free

The cause of this problem is that commit 714fb87e8bc0 make device
"available" before it becomes accessible via sysfs. Therefore, we
roll back the modification. We will fix the race condition between
ubi device creation and udev by removing ubi_get_device in
vol_attribute_show and dev_attribute_show.This avoids accessing
uninitialized ubi_devices[ubi_num].

ubi_get_device is used to prevent devices from being deleted during
sysfs execution. However, now kernfs ensures that devices will not
be deleted before all reference counting are released.
The key process is shown in the following stack.

device_del
  device_remove_attrs
    device_remove_groups
      sysfs_remove_groups
        sysfs_remove_group
          remove_files
            kernfs_remove_by_name
              kernfs_remove_by_name_ns
                __kernfs_remove
                  kernfs_drain

Fixes: 714fb87e8bc0 ("ubi: Fix race condition between ubi device creation and udev")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V1->V2:
	Add race in uif_close
V2->V3:
	Solution modified becase ubi_add_volume might sleepping in spin_lock.

 drivers/mtd/ubi/build.c | 9 +--------
 drivers/mtd/ubi/vmt.c   | 8 +-------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index a7e3eb9befb6..a32050fecabf 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -351,9 +351,6 @@ static ssize_t dev_attribute_show(struct device *dev,
 	 * we still can use 'ubi->ubi_num'.
 	 */
 	ubi = container_of(dev, struct ubi_device, dev);
-	ubi = ubi_get_device(ubi->ubi_num);
-	if (!ubi)
-		return -ENODEV;
 
 	if (attr == &dev_eraseblock_size)
 		ret = sprintf(buf, "%d\n", ubi->leb_size);
@@ -382,7 +379,6 @@ static ssize_t dev_attribute_show(struct device *dev,
 	else
 		ret = -EINVAL;
 
-	ubi_put_device(ubi);
 	return ret;
 }
 
@@ -979,9 +975,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 			goto out_detach;
 	}
 
-	/* Make device "available" before it becomes accessible via sysfs */
-	ubi_devices[ubi_num] = ubi;
-
 	err = uif_init(ubi);
 	if (err)
 		goto out_detach;
@@ -1026,6 +1019,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 	wake_up_process(ubi->bgt_thread);
 	spin_unlock(&ubi->wl_lock);
 
+	ubi_devices[ubi_num] = ubi;
 	ubi_notify_all(ubi, UBI_VOLUME_ADDED, NULL);
 	return ubi_num;
 
@@ -1034,7 +1028,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
 out_uif:
 	uif_close(ubi);
 out_detach:
-	ubi_devices[ubi_num] = NULL;
 	ubi_wl_close(ubi);
 	ubi_free_all_volumes(ubi);
 	vfree(ubi->vtbl);
diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 139ee132bfbc..1bc7b3a05604 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -56,16 +56,11 @@ static ssize_t vol_attribute_show(struct device *dev,
 {
 	int ret;
 	struct ubi_volume *vol = container_of(dev, struct ubi_volume, dev);
-	struct ubi_device *ubi;
-
-	ubi = ubi_get_device(vol->ubi->ubi_num);
-	if (!ubi)
-		return -ENODEV;
+	struct ubi_device *ubi = vol->ubi;
 
 	spin_lock(&ubi->volumes_lock);
 	if (!ubi->volumes[vol->vol_id]) {
 		spin_unlock(&ubi->volumes_lock);
-		ubi_put_device(ubi);
 		return -ENODEV;
 	}
 	/* Take a reference to prevent volume removal */
@@ -103,7 +98,6 @@ static ssize_t vol_attribute_show(struct device *dev,
 	vol->ref_count -= 1;
 	ubi_assert(vol->ref_count >= 0);
 	spin_unlock(&ubi->volumes_lock);
-	ubi_put_device(ubi);
 	return ret;
 }
 
-- 
2.31.1

Re: [PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[libaokun (A)]

在 2021/11/5 17:30, Baokun Li 写道:

ping

> Hulk Robot reported a KASAN report about use-after-free:
>   ==================================================================
>   BUG: KASAN: use-after-free in __list_del_entry_valid+0x13d/0x160
>   Read of size 8 at addr ffff888035e37d98 by task ubiattach/1385
>   [...]
>   Call Trace:
>    klist_dec_and_del+0xa7/0x4a0
>    klist_put+0xc7/0x1a0
>    device_del+0x4d4/0xed0
>    cdev_device_del+0x1a/0x80
>    ubi_attach_mtd_dev+0x2951/0x34b0 [ubi]
>    ctrl_cdev_ioctl+0x286/0x2f0 [ubi]
>
>   Allocated by task 1414:
>    device_add+0x60a/0x18b0
>    cdev_device_add+0x103/0x170
>    ubi_create_volume+0x1118/0x1a10 [ubi]
>    ubi_cdev_ioctl+0xb7f/0x1ba0 [ubi]
>
>   Freed by task 1385:
>    cdev_device_del+0x1a/0x80
>    ubi_remove_volume+0x438/0x6c0 [ubi]
>    ubi_cdev_ioctl+0xbf4/0x1ba0 [ubi]
>   [...]
>   ==================================================================
>
> The lock held by ctrl_cdev_ioctl is ubi_devices_mutex, but the lock held
> by ubi_cdev_ioctl is ubi->device_mutex. Therefore, the two locks can be
> concurrent.
>
> ctrl_cdev_ioctl contains two operations: ubi_attach and ubi_detach.
> ubi_detach is bug-free because it uses reference counting to prevent
> concurrency. However, uif_init and uif_close in ubi_attach may race with
> ubi_cdev_ioctl.
>
> uif_init will race with ubi_cdev_ioctl as in the following stack.
>             cpu1                   cpu2                  cpu3
> _______________________|________________________|______________________
> ctrl_cdev_ioctl
>   ubi_attach_mtd_dev
>    uif_init
>                             ubi_cdev_ioctl
>                              ubi_create_volume
>                               cdev_device_add
>     ubi_add_volume
>     // sysfs exist
>     kill_volumes
>                                                      ubi_cdev_ioctl
>                                                       ubi_remove_volume
>                                                        cdev_device_del
>                                                         // first free
>      ubi_free_volume
>       cdev_del
>       // double free
>     cdev_device_del
>
> And uif_close will race with ubi_cdev_ioctl as in the following stack.
>             cpu1                   cpu2                  cpu3
> _______________________|________________________|______________________
> ctrl_cdev_ioctl
>   ubi_attach_mtd_dev
>    uif_init
>                             ubi_cdev_ioctl
>                              ubi_create_volume
>                               cdev_device_add
>    ubi_debugfs_init_dev
>    //error goto out_uif;
>    uif_close
>     kill_volumes
>                                                      ubi_cdev_ioctl
>                                                       ubi_remove_volume
>                                                        cdev_device_del
>                                                         // first free
>      ubi_free_volume
>      // double free
>
> The cause of this problem is that commit 714fb87e8bc0 make device
> "available" before it becomes accessible via sysfs. Therefore, we
> roll back the modification. We will fix the race condition between
> ubi device creation and udev by removing ubi_get_device in
> vol_attribute_show and dev_attribute_show.This avoids accessing
> uninitialized ubi_devices[ubi_num].
>
> ubi_get_device is used to prevent devices from being deleted during
> sysfs execution. However, now kernfs ensures that devices will not
> be deleted before all reference counting are released.
> The key process is shown in the following stack.
>
> device_del
>    device_remove_attrs
>      device_remove_groups
>        sysfs_remove_groups
>          sysfs_remove_group
>            remove_files
>              kernfs_remove_by_name
>                kernfs_remove_by_name_ns
>                  __kernfs_remove
>                    kernfs_drain
>
> Fixes: 714fb87e8bc0 ("ubi: Fix race condition between ubi device creation and udev")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> V1->V2:
> 	Add race in uif_close
> V2->V3:
> 	Solution modified becase ubi_add_volume might sleepping in spin_lock.
>
>   drivers/mtd/ubi/build.c | 9 +--------
>   drivers/mtd/ubi/vmt.c   | 8 +-------
>   2 files changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index a7e3eb9befb6..a32050fecabf 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -351,9 +351,6 @@ static ssize_t dev_attribute_show(struct device *dev,
>   	 * we still can use 'ubi->ubi_num'.
>   	 */
>   	ubi = container_of(dev, struct ubi_device, dev);
> -	ubi = ubi_get_device(ubi->ubi_num);
> -	if (!ubi)
> -		return -ENODEV;
>   
>   	if (attr == &dev_eraseblock_size)
>   		ret = sprintf(buf, "%d\n", ubi->leb_size);
> @@ -382,7 +379,6 @@ static ssize_t dev_attribute_show(struct device *dev,
>   	else
>   		ret = -EINVAL;
>   
> -	ubi_put_device(ubi);
>   	return ret;
>   }
>   
> @@ -979,9 +975,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>   			goto out_detach;
>   	}
>   
> -	/* Make device "available" before it becomes accessible via sysfs */
> -	ubi_devices[ubi_num] = ubi;
> -
>   	err = uif_init(ubi);
>   	if (err)
>   		goto out_detach;
> @@ -1026,6 +1019,7 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>   	wake_up_process(ubi->bgt_thread);
>   	spin_unlock(&ubi->wl_lock);
>   
> +	ubi_devices[ubi_num] = ubi;
>   	ubi_notify_all(ubi, UBI_VOLUME_ADDED, NULL);
>   	return ubi_num;
>   
> @@ -1034,7 +1028,6 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num,
>   out_uif:
>   	uif_close(ubi);
>   out_detach:
> -	ubi_devices[ubi_num] = NULL;
>   	ubi_wl_close(ubi);
>   	ubi_free_all_volumes(ubi);
>   	vfree(ubi->vtbl);
> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
> index 139ee132bfbc..1bc7b3a05604 100644
> --- a/drivers/mtd/ubi/vmt.c
> +++ b/drivers/mtd/ubi/vmt.c
> @@ -56,16 +56,11 @@ static ssize_t vol_attribute_show(struct device *dev,
>   {
>   	int ret;
>   	struct ubi_volume *vol = container_of(dev, struct ubi_volume, dev);
> -	struct ubi_device *ubi;
> -
> -	ubi = ubi_get_device(vol->ubi->ubi_num);
> -	if (!ubi)
> -		return -ENODEV;
> +	struct ubi_device *ubi = vol->ubi;
>   
>   	spin_lock(&ubi->volumes_lock);
>   	if (!ubi->volumes[vol->vol_id]) {
>   		spin_unlock(&ubi->volumes_lock);
> -		ubi_put_device(ubi);
>   		return -ENODEV;
>   	}
>   	/* Take a reference to prevent volume removal */
> @@ -103,7 +98,6 @@ static ssize_t vol_attribute_show(struct device *dev,
>   	vol->ref_count -= 1;
>   	ubi_assert(vol->ref_count >= 0);
>   	spin_unlock(&ubi->volumes_lock);
> -	ubi_put_device(ubi);
>   	return ret;
>   }
>   

Re: [PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "libaokun (A)" <libaokun1@huawei.com>
> 在 2021/11/5 17:30, Baokun Li 写道:
> 
> ping

Thanks for your patience.
 
>> Hulk Robot reported a KASAN report about use-after-free:

[...]

>> The cause of this problem is that commit 714fb87e8bc0 make device
>> "available" before it becomes accessible via sysfs. Therefore, we
>> roll back the modification. We will fix the race condition between
>> ubi device creation and udev by removing ubi_get_device in
>> vol_attribute_show and dev_attribute_show.This avoids accessing
>> uninitialized ubi_devices[ubi_num].
>>
>> ubi_get_device is used to prevent devices from being deleted during
>> sysfs execution. However, now kernfs ensures that devices will not
>> be deleted before all reference counting are released.
>> The key process is shown in the following stack.

ubi_get_device() in dev_attribute_show() is used to detect whether
the ubi device got detached while the sysfs file is open.

Hmm. I thought for sysfs this is not the case since sysfs does not implement
a release() method. So kernfs_drain_open_files() will return early.
But there is a good chance that I don't got all kernfs/sysfs details.

Thanks,
//richard

Re: [PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[Zhihao Cheng]

在 2021/12/24 5:06, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----


>>> ubi_get_device is used to prevent devices from being deleted during
>>> sysfs execution. However, now kernfs ensures that devices will not
>>> be deleted before all reference counting are released.
>>> The key process is shown in the following stack.
> 
> ubi_get_device() in dev_attribute_show() is used to detect whether
> the ubi device got detached while the sysfs file is open.
> 
> Hmm. I thought for sysfs this is not the case since sysfs does not implement
> a release() method. So kernfs_drain_open_files() will return early.
> But there is a good chance that I don't got all kernfs/sysfs details.
> 

kernfs_drain() will wait 'root->deactivate_waitq' if 
atomic_read(&kn->active) not equals to KN_DEACTIVATED_BIAS.

The UBI seq_show callback is invoked with avtive cnt taken:
vfs_read
   kernfs_fop_read_iter
     seq_read_iter
       m->op->start (kernfs_seq_start)   // kernfs_get_active(of->kn)
       kernfs_seq_show
         dev_attribute_show [ubi]
       m->op->stop (kernfs_seq_stop)     // kernfs_put_active(of->kn)

The kernfs_drain() is stuck at wait_event() until sysfs reading 
finished, in my local test.

Re: [PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[libaokun (A)]

在 2021/12/24 5:06, Richard Weinberger 写道:
> ----- Ursprüngliche Mail -----
>> Von: "libaokun (A)" <libaokun1@huawei.com>
>> 在 2021/11/5 17:30, Baokun Li 写道:
>>
>> ping
> Thanks for your patience.
>   
>>> Hulk Robot reported a KASAN report about use-after-free:
> [...]
>
>>> The cause of this problem is that commit 714fb87e8bc0 make device
>>> "available" before it becomes accessible via sysfs. Therefore, we
>>> roll back the modification. We will fix the race condition between
>>> ubi device creation and udev by removing ubi_get_device in
>>> vol_attribute_show and dev_attribute_show.This avoids accessing
>>> uninitialized ubi_devices[ubi_num].
>>>
>>> ubi_get_device is used to prevent devices from being deleted during
>>> sysfs execution. However, now kernfs ensures that devices will not
>>> be deleted before all reference counting are released.
>>> The key process is shown in the following stack.
> ubi_get_device() in dev_attribute_show() is used to detect whether
> the ubi device got detached while the sysfs file is open.
>
> Hmm. I thought for sysfs this is not the case since sysfs does not implement
> a release() method. So kernfs_drain_open_files() will return early.
> But there is a good chance that I don't got all kernfs/sysfs details.
>
> Thanks,
> //richard
> .

Thank you for your review!

-- 
With Best Regards,
Baokun Li

Re: [PATCH -next V3] ubi: fix race condition between ctrl_cdev_ioctl and ubi_cdev_ioctl

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@huawei.com>
> kernfs_drain() will wait 'root->deactivate_waitq' if
> atomic_read(&kn->active) not equals to KN_DEACTIVATED_BIAS.
> 
> The UBI seq_show callback is invoked with avtive cnt taken:
> vfs_read
>   kernfs_fop_read_iter
>     seq_read_iter
>       m->op->start (kernfs_seq_start)   // kernfs_get_active(of->kn)
>       kernfs_seq_show
>         dev_attribute_show [ubi]
>       m->op->stop (kernfs_seq_stop)     // kernfs_put_active(of->kn)
> 
> The kernfs_drain() is stuck at wait_event() until sysfs reading
> finished, in my local test.

You are right. This means UBI does this extra check in vain.
Maybe even since ever.

Thanks,
//richard