[PATCH v2] virtio: fix reference leak in register_virtio_device()

[PATCH v2] virtio: fix reference leak in register_virtio_device()

[Ma Ke]

Once device_add(&dev->dev) failed, call put_device() to explicitly
release dev->dev. Or it could cause double free problem.

As comment of device_add() says, 'if device_add() succeeds, you should
call device_del() when you want to get rid of it. If device_add() has
not succeeded, use only put_device() to drop the reference count'.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: f2b44cde7e16 ("virtio: split device_register into device_initialize and device_add")
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
Changes in v2:
- modified the bug description to make it more clear;
- changed the Fixes tag.
---
 drivers/virtio/virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index b9095751e43b..ac721b5597e8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -503,6 +503,7 @@ int register_virtio_device(struct virtio_device *dev)
 
 out_of_node_put:
 	of_node_put(dev->dev.of_node);
+	put_device(&dev->dev);
 out_ida_remove:
 	ida_free(&virtio_index_ida, dev->index);
 out:
-- 
2.25.1

Re: [PATCH v2] virtio: fix reference leak in register_virtio_device()

[Markus Elfring]

> Once device_add(&dev->dev) failed, call put_device() to explicitly
> release dev->dev. Or it could cause double free problem.

https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/base/core.c#L3521


…
> Found by code review.

https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/virtio/virtio.c#L498

Will possibilities become interesting to improve automated source code analyses accordingly?


…
> +++ b/drivers/virtio/virtio.c
> @@ -503,6 +503,7 @@ int register_virtio_device(struct virtio_device *dev)
>
>  out_of_node_put:
>  	of_node_put(dev->dev.of_node);
> +	put_device(&dev->dev);
>  out_ida_remove:
>  	ida_free(&virtio_index_ida, dev->index);
>  out:

How much can the ordering matter for such resource cleanup calls here?

Regards,
Markus

Re: [PATCH v2] virtio: fix reference leak in register_virtio_device()

[Michael S. Tsirkin]

On Wed, Dec 18, 2024 at 11:12:01AM +0800, Ma Ke wrote:
> Once device_add(&dev->dev) failed, call put_device() to explicitly
> release dev->dev. Or it could cause double free problem.
> 
> As comment of device_add() says, 'if device_add() succeeds, you should
> call device_del() when you want to get rid of it. If device_add() has
> not succeeded, use only put_device() to drop the reference count'.
> 
> Found by code review.
> 
> Cc: stable@vger.kernel.org
> Fixes: f2b44cde7e16 ("virtio: split device_register into device_initialize and device_add")
> Signed-off-by: Ma Ke <make_ruc2021@163.com>


Did you actually test this, triggering an error,
and with debug options enabled to find double free
and use after free? Which configurations?

Because if you did, you would find for example this
in drivers/virtio/virtio_vdpa.c:

        ret = register_virtio_device(&vd_dev->vdev);
        reg_dev = vd_dev;
        if (ret)
                goto err;

        vdpa_set_drvdata(vdpa, vd_dev);

        return 0;
        
err:    
        if (reg_dev)
                put_device(&vd_dev->vdev.dev);
        else
                kfree(vd_dev);
        return ret;




> ---
> Changes in v2:
> - modified the bug description to make it more clear;
> - changed the Fixes tag.
> ---
>  drivers/virtio/virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index b9095751e43b..ac721b5597e8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -503,6 +503,7 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  out_of_node_put:
>  	of_node_put(dev->dev.of_node);
> +	put_device(&dev->dev);
>  out_ida_remove:
>  	ida_free(&virtio_index_ida, dev->index);
>  out:
> -- 
> 2.25.1
> 

[PATCH v2] virtio: fix reference leak in register_virtio_device()

[Ma Ke]

The reference count of the device incremented in device_initialize() is
not decremented when device_add() fails. Add a put_device() call before
returning from the function.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: f2b44cde7e16 ("virtio: split device_register into device_initialize and device_add")
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
Changes in v2:
- modified the fixes tag according to suggestions;
- modified the bug description.
---
 drivers/virtio/virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index b9095751e43b..ac721b5597e8 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -503,6 +503,7 @@ int register_virtio_device(struct virtio_device *dev)
 
 out_of_node_put:
 	of_node_put(dev->dev.of_node);
+	put_device(&dev->dev);
 out_ida_remove:
 	ida_free(&virtio_index_ida, dev->index);
 out:
-- 
2.25.1