diff mbox series

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

Message ID 20221111085809.1676804-1-yangyingliang@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Hans Verkuil
Headers
Series media: v4l2-dev: fix possible name leak in __video_register_device() |

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. 11, 2022, 9:19 a.m. UTC | #1 
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.

>  		goto cleanup;
>  	}
>  	/* Register the release callback that will be called when the last
Yang Yingliang Nov. 11, 2022, 9:38 a.m. UTC | #2 
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.
May be we should  move 'vdev->dev.release = v4l2_device_release' before 
device_register(),
then call put_device() to fix it.

Thanks,
Yang
>
>>   		goto cleanup;
>>   	}
>>   	/* Register the release callback that will be called when the last
Yang Yingliang Nov. 11, 2022, 1:23 p.m. UTC | #3 
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.

Thanks,
Yang
>
>>   		goto cleanup;
>>   	}
>>   	/* Register the release callback that will be called when the last
Laurent Pinchart Nov. 14, 2022, 2:37 p.m. UTC | #4 
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