* [PATCH] media: rockchip/rga: fix a use after free in rga_probe()
@ 2021-07-09 6:38 Dan Carpenter
2021-07-09 9:49 ` Dongliang Mu
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-07-09 6:38 UTC (permalink / raw)
To: Jacob Chen
Cc: Ezequiel Garcia, Dongliang Mu, Mauro Carvalho Chehab,
Heiko Stuebner, Hans Verkuil, linux-media, linux-rockchip,
linux-kernel, kernel-janitors
The video_device_release() frees the "vfd" pointer so passing it to
video_unregister_device() on the next line results in a use after free.
Calling video_unregister_device() on a device that hasn't been
registered is supposed to be a no-op so that can be removed. The paths
with to goto unreg_video_dev have a memory leak and should be updated to
goto rel_vdev instead.
Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/media/platform/rockchip/rga/rga.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index bf3fd71ec3af..37f7fd060c38 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -863,12 +863,12 @@ static int rga_probe(struct platform_device *pdev)
if (IS_ERR(rga->m2m_dev)) {
v4l2_err(&rga->v4l2_dev, "Failed to init mem2mem device\n");
ret = PTR_ERR(rga->m2m_dev);
- goto unreg_video_dev;
+ goto rel_vdev;
}
ret = pm_runtime_resume_and_get(rga->dev);
if (ret < 0)
- goto unreg_video_dev;
+ goto rel_vdev;
rga->version.major = (rga_read(rga, RGA_VERSION_INFO) >> 24) & 0xFF;
rga->version.minor = (rga_read(rga, RGA_VERSION_INFO) >> 20) & 0x0F;
@@ -904,8 +904,6 @@ static int rga_probe(struct platform_device *pdev)
rel_vdev:
video_device_release(vfd);
-unreg_video_dev:
- video_unregister_device(rga->vfd);
unreg_v4l2_dev:
v4l2_device_unregister(&rga->v4l2_dev);
err_put_clk:
--
2.30.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] media: rockchip/rga: fix a use after free in rga_probe() 2021-07-09 6:38 [PATCH] media: rockchip/rga: fix a use after free in rga_probe() Dan Carpenter @ 2021-07-09 9:49 ` Dongliang Mu 2021-07-09 13:45 ` Dan Carpenter 0 siblings, 1 reply; 3+ messages in thread From: Dongliang Mu @ 2021-07-09 9:49 UTC (permalink / raw) To: Dan Carpenter Cc: Jacob Chen, Ezequiel Garcia, Mauro Carvalho Chehab, Heiko Stuebner, Hans Verkuil, linux-media, linux-rockchip, linux-kernel, kernel-janitors On Fri, Jul 9, 2021 at 2:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The video_device_release() frees the "vfd" pointer so passing it to > video_unregister_device() on the next line results in a use after free. > Calling video_unregister_device() on a device that hasn't been > registered is supposed to be a no-op so that can be removed. The paths > with to goto unreg_video_dev have a memory leak and should be updated to > goto rel_vdev instead. > > Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/media/platform/rockchip/rga/rga.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c > index bf3fd71ec3af..37f7fd060c38 100644 > --- a/drivers/media/platform/rockchip/rga/rga.c > +++ b/drivers/media/platform/rockchip/rga/rga.c > @@ -863,12 +863,12 @@ static int rga_probe(struct platform_device *pdev) > if (IS_ERR(rga->m2m_dev)) { > v4l2_err(&rga->v4l2_dev, "Failed to init mem2mem device\n"); > ret = PTR_ERR(rga->m2m_dev); > - goto unreg_video_dev; > + goto rel_vdev; > } > > ret = pm_runtime_resume_and_get(rga->dev); > if (ret < 0) > - goto unreg_video_dev; > + goto rel_vdev; > > rga->version.major = (rga_read(rga, RGA_VERSION_INFO) >> 24) & 0xFF; > rga->version.minor = (rga_read(rga, RGA_VERSION_INFO) >> 20) & 0x0F; > @@ -904,8 +904,6 @@ static int rga_probe(struct platform_device *pdev) > > rel_vdev: > video_device_release(vfd); > -unreg_video_dev: > - video_unregister_device(rga->vfd); > unreg_v4l2_dev: > v4l2_device_unregister(&rga->v4l2_dev); > err_put_clk: From the analysis of rga_probe and rga_remove function, the init and cleanup functions are in pairs as follows: pm_runtime_enable(rga->dev); ret = v4l2_device_register(&pdev->dev, &rga->v4l2_dev); vfd = video_device_alloc(); rga->m2m_dev = v4l2_m2m_init(&rga_m2m_ops); rga->cmdbuf_virt = dma_alloc_attrs(rga->dev, RGA_CMDBUF_SIZE, &rga->cmdbuf_phy, GFP_KERNEL, DMA_ATTR_WRITE_COMBINE); rga->src_mmu_pages = (unsigned int *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 3); rga->dst_mmu_pages = (unsigned int *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, 3); ret = video_register_device(vfd, VFL_TYPE_VIDEO, -1); ...... video_unregister_device(rga->vfd); free_pages((unsigned long)rga->src_mmu_pages, 3); free_pages((unsigned long)rga->dst_mmu_pages, 3); dma_free_attrs(rga->dev, RGA_CMDBUF_SIZE, rga->cmdbuf_virt, rga->cmdbuf_phy, DMA_ATTR_WRITE_COMBINE); v4l2_m2m_release(rga->m2m_dev); video_device_release(rga->vfd); v4l2_device_unregister(&rga->v4l2_dev); pm_runtime_disable(rga->dev); So I think the error handling code is more complex than this patch. ;( > -- > 2.30.2 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: rockchip/rga: fix a use after free in rga_probe() 2021-07-09 9:49 ` Dongliang Mu @ 2021-07-09 13:45 ` Dan Carpenter 0 siblings, 0 replies; 3+ messages in thread From: Dan Carpenter @ 2021-07-09 13:45 UTC (permalink / raw) To: Dongliang Mu Cc: Jacob Chen, Ezequiel Garcia, Mauro Carvalho Chehab, Heiko Stuebner, Hans Verkuil, linux-media, linux-rockchip, linux-kernel, kernel-janitors On Fri, Jul 09, 2021 at 05:49:26PM +0800, Dongliang Mu wrote: > On Fri, Jul 9, 2021 at 2:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > The video_device_release() frees the "vfd" pointer so passing it to > > video_unregister_device() on the next line results in a use after free. > > Calling video_unregister_device() on a device that hasn't been > > registered is supposed to be a no-op so that can be removed. The paths > > with to goto unreg_video_dev have a memory leak and should be updated to > > goto rel_vdev instead. > > > > Fixes: f7e7b48e6d79 ("[media] rockchip/rga: v4l2 m2m support") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/media/platform/rockchip/rga/rga.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c > > index bf3fd71ec3af..37f7fd060c38 100644 > > --- a/drivers/media/platform/rockchip/rga/rga.c > > +++ b/drivers/media/platform/rockchip/rga/rga.c > > @@ -863,12 +863,12 @@ static int rga_probe(struct platform_device *pdev) > > if (IS_ERR(rga->m2m_dev)) { > > v4l2_err(&rga->v4l2_dev, "Failed to init mem2mem device\n"); > > ret = PTR_ERR(rga->m2m_dev); > > - goto unreg_video_dev; > > + goto rel_vdev; > > } > > > > ret = pm_runtime_resume_and_get(rga->dev); > > if (ret < 0) > > - goto unreg_video_dev; > > + goto rel_vdev; > > > > rga->version.major = (rga_read(rga, RGA_VERSION_INFO) >> 24) & 0xFF; > > rga->version.minor = (rga_read(rga, RGA_VERSION_INFO) >> 20) & 0x0F; > > @@ -904,8 +904,6 @@ static int rga_probe(struct platform_device *pdev) > > > > rel_vdev: > > video_device_release(vfd); > > -unreg_video_dev: > > - video_unregister_device(rga->vfd); > > unreg_v4l2_dev: > > v4l2_device_unregister(&rga->v4l2_dev); > > err_put_clk: > > >From the analysis of rga_probe and rga_remove function, the init and > cleanup functions are in pairs as follows: > Yeah. You're right. It's leaky. This is like the stuff Christophe Jaillet has been working on. This is also slightly complicated because what about if the call to device_register() fails inside video_register_device()? In that case we're not allowed to call video_device_release(). I think the way people deal with that is because device_register() never fails in real life so let's ignore that possibility. I sort of hate device_register() because it introduces a lot of bugs like this which don't affect real life. Inside the __video_register_device() function itself we just ignore errors from video_register_media_controller() because there is no possible way to handle it in a bug free way. Again, it's not a bug which is going to affect real life, but it's just frustrating to not be able to write code which is correct in a technical sense. Ah well... Fine. I'll resend. regards, dan carpenter ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-09 13:46 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-09 6:38 [PATCH] media: rockchip/rga: fix a use after free in rga_probe() Dan Carpenter 2021-07-09 9:49 ` Dongliang Mu 2021-07-09 13:45 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).