| Message ID | 20220418101724.42174-1-hbh25y@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rpmsg: virtio: fix possible double free in rpmsg_virtio_add_ctrl_dev() | expand |
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:
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 <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? 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/20220418093144.40859-1-hbh25y@gmail.com/ For this one: Tested-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> 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 <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:
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); }
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(-)