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 <[email protected]>
---
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);
}
--
2.25.1
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 <[email protected]>
> ---
> 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 <[email protected]>
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
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
On 4/22/22 17:16, Arnaud POULIQUEN wrote:
> 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 <[email protected]>
>> ---
>> 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?
My apologies, I just saw your second patch[1], so what I have to do is to send a new one to fix
rpmsg_virtio_del_ctrl_dev()
[1]https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
For this one:
Tested-by: Arnaud Pouliquen <[email protected]>
Regards
Arnaud
>
> 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 <[email protected]>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> 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: