diff mbox series

media: v4l2-dev: fix possible name leak in __video_register_device()

Message ID 20221111085809.1676804-1-yangyingliang@huawei.com
State New
Headers show
Series media: v4l2-dev: fix possible name leak in __video_register_device() | expand

Commit Message

Yang Yingliang Nov. 11, 2022, 8:58 a.m. UTC
Afer commit 1fa5ae857bb1 ("driver core: get rid of struct device's
bus_id string array"), the name of device is allocated dynamically,
so call kfree_const() to freed it in error case.

Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Laurent Pinchart Nov. 14, 2022, 2:37 p.m. UTC | #1
On Fri, Nov 11, 2022 at 09:23:29PM +0800, Yang Yingliang wrote:
> On 2022/11/11 17:19, Laurent Pinchart wrote:
> > On Fri, Nov 11, 2022 at 04:58:09PM +0800, Yang Yingliang wrote:
> >> Afer commit 1fa5ae857bb1 ("driver core: get rid of struct device's
> >> bus_id string array"), the name of device is allocated dynamically,
> >> so call kfree_const() to freed it in error case.
> >>
> >> Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> >> ---
> >>   drivers/media/v4l2-core/v4l2-dev.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >> index 397d553177fa..1fce1c29c60f 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -1036,6 +1036,9 @@ int __video_register_device(struct video_device *vdev,
> >>   	ret = device_register(&vdev->dev);
> >>   	if (ret < 0) {
> >>   		pr_err("%s: device_register failed\n", __func__);
> >> +		kfree_const(vdev->dev.kobj.name);
> >> +		/* set it to null to avoid callers using a bad pointer */
> >> +		vdev->dev.kobj.name = NULL;
> >
> > This doesn't seem right to me. We shouldn't be poking into the kobj.
> > This should be handled in the driver core, or if that can't be done for
> > a reason specific to how the device is used in the V4L2 core, we need a
> > helper function from the driver core to perform the cleanup.
>
> In technical, we should call put_device() here, but the release()
> function has not been set in this case, and v4l2_device_release() will
> release something that not need be, so we can not handle it in the
> driver core well, I think free the name here directly is the best way
> to fix it.

The documentation of device_register() states

 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.

We thus need to fix this in __video_register_device(), not to pile
another workaround :-)

> >>   		goto cleanup;
> >>   	}
> >>   	/* Register the release callback that will be called when the last
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 397d553177fa..1fce1c29c60f 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1036,6 +1036,9 @@  int __video_register_device(struct video_device *vdev,
 	ret = device_register(&vdev->dev);
 	if (ret < 0) {
 		pr_err("%s: device_register failed\n", __func__);
+		kfree_const(vdev->dev.kobj.name);
+		/* set it to null to avoid callers using a bad pointer */
+		vdev->dev.kobj.name = NULL;
 		goto cleanup;
 	}
 	/* Register the release callback that will be called when the last