[PATCH v2] scsi: bsg: Fix device unregistration

[PATCH v2] scsi: bsg: Fix device unregistration

[Zenghui Yu]

We use device_initialize() to take refcount for the device but forget to
put_device() on device teardown, which ends up leaking private data of the
driver core, dev_name(), etc. This is reported by kmemleak at boot time if
we compile kernel with DEBUG_TEST_DRIVER_REMOVE.

Note that adding the missing put_device() is _not_ sufficient to fix device
unregistration. As we don't provide the .release() method for device, which
turned out to be typically wrong and will be complained loudly by the
driver core.

Fix both of them.

Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
* From v1 [1]:
  - As pointed out by Johan, fix UAF and double-free on error path ...
  - ... so I didn't collect Christoph and Greg's R-b tags (but thanks
    for reviewing)

[1] https://lore.kernel.org/r/20210909034608.1435-1-yuzenghui@huawei.com

 block/bsg.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 351095193788..882f56bff14f 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -165,13 +165,20 @@ static const struct file_operations bsg_fops = {
 	.llseek		=	default_llseek,
 };
 
+static void bsg_device_release(struct device *dev)
+{
+	struct bsg_device *bd = container_of(dev, struct bsg_device, device);
+
+	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
+	kfree(bd);
+}
+
 void bsg_unregister_queue(struct bsg_device *bd)
 {
 	if (bd->queue->kobj.sd)
 		sysfs_remove_link(&bd->queue->kobj, "bsg");
 	cdev_device_del(&bd->cdev, &bd->device);
-	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
-	kfree(bd);
+	put_device(&bd->device);
 }
 EXPORT_SYMBOL_GPL(bsg_unregister_queue);
 
@@ -193,11 +200,13 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
 	if (ret < 0) {
 		if (ret == -ENOSPC)
 			dev_err(parent, "bsg: too many bsg devices\n");
-		goto out_kfree;
+		kfree(bd);
+		return ERR_PTR(ret);
 	}
 	bd->device.devt = MKDEV(bsg_major, ret);
 	bd->device.class = bsg_class;
 	bd->device.parent = parent;
+	bd->device.release = bsg_device_release;
 	dev_set_name(&bd->device, "%s", name);
 	device_initialize(&bd->device);
 
@@ -205,7 +214,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
 	bd->cdev.owner = THIS_MODULE;
 	ret = cdev_device_add(&bd->cdev, &bd->device);
 	if (ret)
-		goto out_ida_remove;
+		goto out_put_device;
 
 	if (q->kobj.sd) {
 		ret = sysfs_create_link(&q->kobj, &bd->device.kobj, "bsg");
@@ -217,10 +226,8 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
 
 out_device_del:
 	cdev_device_del(&bd->cdev, &bd->device);
-out_ida_remove:
-	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
-out_kfree:
-	kfree(bd);
+out_put_device:
+	put_device(&bd->device);
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(bsg_register_queue);
-- 
2.19.1

Re: [PATCH v2] scsi: bsg: Fix device unregistration

[Johan Hovold]

On Sat, Sep 11, 2021 at 06:53:06PM +0800, Zenghui Yu wrote:
> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is _not_ sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> Fix both of them.
> 
> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> * From v1 [1]:
>   - As pointed out by Johan, fix UAF and double-free on error path ...

Looks good now:

Reviewed-by: Johan Hovold <johan@kernel.org>

>   - ... so I didn't collect Christoph and Greg's R-b tags (but thanks
>     for reviewing)
> 
> [1] https://lore.kernel.org/r/20210909034608.1435-1-yuzenghui@huawei.com
> 
>  block/bsg.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/block/bsg.c b/block/bsg.c
> index 351095193788..882f56bff14f 100644
> --- a/block/bsg.c
> +++ b/block/bsg.c
> @@ -165,13 +165,20 @@ static const struct file_operations bsg_fops = {
>  	.llseek		=	default_llseek,
>  };
>  
> +static void bsg_device_release(struct device *dev)
> +{
> +	struct bsg_device *bd = container_of(dev, struct bsg_device, device);
> +
> +	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
> +	kfree(bd);
> +}
> +
>  void bsg_unregister_queue(struct bsg_device *bd)
>  {
>  	if (bd->queue->kobj.sd)
>  		sysfs_remove_link(&bd->queue->kobj, "bsg");
>  	cdev_device_del(&bd->cdev, &bd->device);
> -	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
> -	kfree(bd);
> +	put_device(&bd->device);
>  }
>  EXPORT_SYMBOL_GPL(bsg_unregister_queue);
>  
> @@ -193,11 +200,13 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>  	if (ret < 0) {
>  		if (ret == -ENOSPC)
>  			dev_err(parent, "bsg: too many bsg devices\n");
> -		goto out_kfree;
> +		kfree(bd);
> +		return ERR_PTR(ret);
>  	}
>  	bd->device.devt = MKDEV(bsg_major, ret);
>  	bd->device.class = bsg_class;
>  	bd->device.parent = parent;
> +	bd->device.release = bsg_device_release;
>  	dev_set_name(&bd->device, "%s", name);
>  	device_initialize(&bd->device);
>  
> @@ -205,7 +214,7 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>  	bd->cdev.owner = THIS_MODULE;
>  	ret = cdev_device_add(&bd->cdev, &bd->device);
>  	if (ret)
> -		goto out_ida_remove;
> +		goto out_put_device;
>  
>  	if (q->kobj.sd) {
>  		ret = sysfs_create_link(&q->kobj, &bd->device.kobj, "bsg");
> @@ -217,10 +226,8 @@ struct bsg_device *bsg_register_queue(struct request_queue *q,
>  
>  out_device_del:
>  	cdev_device_del(&bd->cdev, &bd->device);
> -out_ida_remove:
> -	ida_simple_remove(&bsg_minor_ida, MINOR(bd->device.devt));
> -out_kfree:
> -	kfree(bd);
> +out_put_device:
> +	put_device(&bd->device);
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL_GPL(bsg_register_queue);

Re: [PATCH v2] scsi: bsg: Fix device unregistration

[Martin K. Petersen]

On Sat, 11 Sep 2021 18:53:06 +0800, Zenghui Yu wrote:

> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is _not_ sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> [...]

Applied to 5.15/scsi-fixes, thanks!

[1/1] scsi: bsg: Fix device unregistration
      https://git.kernel.org/mkp/scsi/c/1a0db7744e45

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi: bsg: Fix device unregistration

[John Garry]

On 11/09/2021 11:53, Zenghui Yu wrote:
> We use device_initialize() to take refcount for the device but forget to
> put_device() on device teardown, which ends up leaking private data of the
> driver core, dev_name(), etc. This is reported by kmemleak at boot time if
> we compile kernel with DEBUG_TEST_DRIVER_REMOVE.
> 
> Note that adding the missing put_device() is_not_  sufficient to fix device
> unregistration. As we don't provide the .release() method for device, which
> turned out to be typically wrong and will be complained loudly by the
> driver core.
> 
> Fix both of them.
> 
> Fixes: ead09dd3aed5 ("scsi: bsg: Simplify device registration")
> Signed-off-by: Zenghui Yu<yuzenghui@huawei.com>


FWIW,

Tested-by: John Garry <john.garry@huawei.com>