Message ID | 20221111085809.1676804-1-yangyingliang@huawei.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1otPwZ-005myp-Po; Fri, 11 Nov 2022 09:03:24 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233504AbiKKJDT (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 11 Nov 2022 04:03:19 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48260 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233512AbiKKJDC (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 11 Nov 2022 04:03:02 -0500 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E71D97FC07 for <linux-media@vger.kernel.org>; Fri, 11 Nov 2022 00:59:42 -0800 (PST) Received: from dggpemm500021.china.huawei.com (unknown [172.30.72.56]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4N7syF51mJzJnZN; Fri, 11 Nov 2022 16:56:37 +0800 (CST) Received: from dggpemm500007.china.huawei.com (7.185.36.183) by dggpemm500021.china.huawei.com (7.185.36.109) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Fri, 11 Nov 2022 16:59:41 +0800 Received: from huawei.com (10.175.103.91) by dggpemm500007.china.huawei.com (7.185.36.183) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Fri, 11 Nov 2022 16:59:40 +0800 From: Yang Yingliang <yangyingliang@huawei.com> To: <linux-media@vger.kernel.org> CC: <mchehab@kernel.org>, <sakari.ailus@linux.intel.com>, <laurent.pinchart+renesas@ideasonboard.com>, <hverkuil-cisco@xs4all.nl>, <yangyingliang@huawei.com> Subject: [PATCH] media: v4l2-dev: fix possible name leak in __video_register_device() Date: Fri, 11 Nov 2022 16:58:09 +0800 Message-ID: <20221111085809.1676804-1-yangyingliang@huawei.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.175.103.91] X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To dggpemm500007.china.huawei.com (7.185.36.183) X-CFilter-Loop: Reflected X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.4 (--) X-LSpam-Report: No, score=-2.4 required=5.0 tests=BAYES_00=-1.9,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
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
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
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
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
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 --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