Re: [PATCH] rpmsg: virtio: fix possible double free in rpmsg_virtio_add_ctrl_dev()

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

Hi Hangyu,

On 4/18/22 12:17, Hangyu Hua wrote:
> vch will be free in virtio_rpmsg_release_device() when
> rpmsg_ctrldev_register_device() fails. There is no need to call
> kfree() again.
> 
> Fixes: c486682ae1e2 ("rpmsg: virtio: Register the rpmsg_char device")
> Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 603233f0686e..3b7b47f785cf 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -851,7 +851,6 @@ static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
>  
>  	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
>  	if (err) {
> -		kfree(vch);
>  		return ERR_PTR(err);
>  	}
>  

Good catch! I confirmed by testing the error case. There is a double free.

That said this highlight a quite more complex issue as
rpmsg_virtio_del_ctrl_dev[1] and rpmsg_ns_register_device(rpdev_ns)error
case[2] need also some improvements.

[1]
https://elixir.bootlin.com/linux/v5.18-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L861
[2]https://elixir.bootlin.com/linux/v5.18-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L974

Please find at the end of my mail a V2 patch that should fix more error
cases.
As you initiate the fix, do you want to send the V2 or do you prefer
that I send the fix?

Thanks,
Arnaud

Subject: [PATCH V2] rpmsg: virtio: fix possible double free in rpmsg_probe()

the virtio_rpmsg_channel structure will be free in
virtio_rpmsg_release_device() when the device_register() fails or
when device_unregister is called.
There is no need to call kfree() again.

Fixes: c486682ae1e2 ("rpmsg: virtio: Register the rpmsg_char device")

Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
b/drivers/rpmsg/virtio_rpmsg_bus.c
index 3ede25b1f2e4..a65c8be9b11f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -850,10 +850,8 @@ static struct rpmsg_device
*rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev
 	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);

 	err = rpmsg_ctrldev_register_device(rpdev_ctrl);
-	if (err) {
-		kfree(vch);
+	if (err)
 		return ERR_PTR(err);
-	}

 	return rpdev_ctrl;
 }
@@ -862,7 +860,7 @@ static void rpmsg_virtio_del_ctrl_dev(struct
rpmsg_device *rpdev_ctrl)
 {
 	if (!rpdev_ctrl)
 		return;
-	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
+	device_unregister(&rpdev_ctrl->dev);
 }

 static int rpmsg_probe(struct virtio_device *vdev)
@@ -973,7 +971,7 @@ static int rpmsg_probe(struct virtio_device *vdev)

 		err = rpmsg_ns_register_device(rpdev_ns);
 		if (err)
-			goto free_vch;
+			goto free_ctrldev;
 	}

 	/*
@@ -997,8 +995,6 @@ static int rpmsg_probe(struct virtio_device *vdev)

 	return 0;

-free_vch:
-	kfree(vch);
 free_ctrldev:
 	rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
 free_coherent:
-- 
2.24.3