[PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Sergey Shtylyov]

If CONFIG_MEDIA_CONTROLLER is undefined, media_request_object_find() will
always return NULL, so its 2nd call in v4l2_ctrl_request_complete() would
fail as well as the 1st one and thus cause hdl to have a wrong value (at
the top of memory) and list_for_each_entry() to iterate over the garbage
data located there. Add NULL check for the 2nd call and place the error
cleanup at the end of v4l2_ctrl_request_complete()...

Found by Linux Verification Center (linuxtesting.org) with the Svace static
analysis tool.

Fixes: c3bf5129f339 ("media: v4l2-ctrls: always copy the controls on completion")
Signed-off-by: Sergey Shtylyov <s.shtylyov@auroraos.dev>

---
The patch is against the fixes branch of the git.linuxtv.org/media.git repo...

 drivers/media/v4l2-core/v4l2-ctrls-request.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-request.c b/drivers/media/v4l2-core/v4l2-ctrls-request.c
index 4b7e6981b8d6..5c2fc767c6a6 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-request.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-request.c
@@ -348,13 +348,12 @@ void v4l2_ctrl_request_complete(struct media_request *req,
 		ret = v4l2_ctrl_handler_init(hdl, (main_hdl->nr_of_buckets - 1) * 8);
 		if (!ret)
 			ret = v4l2_ctrl_request_bind(req, hdl, main_hdl);
-		if (ret) {
-			v4l2_ctrl_handler_free(hdl);
-			kfree(hdl);
-			return;
-		}
+		if (ret)
+			goto error;
 		hdl->request_is_queued = true;
 		obj = media_request_object_find(req, &req_ops, main_hdl);
+		if (!obj)
+			goto error;
 	}
 	hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
 
@@ -389,6 +388,11 @@ void v4l2_ctrl_request_complete(struct media_request *req,
 	mutex_unlock(main_hdl->lock);
 	media_request_object_complete(obj);
 	media_request_object_put(obj);
+	return;
+
+error:
+	v4l2_ctrl_handler_free(hdl);
+	kfree(hdl);
 }
 EXPORT_SYMBOL(v4l2_ctrl_request_complete);
 
-- 
2.53.0

Re: [PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Sergey Shtylyov]

Hello!

   Oops, a stray comma got somehow added to the subject --
do I need to resend?

MBR, Sergey

Re: [PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Sergey Shtylyov]

On 5/2/26 11:52 AM, Sergey Shtylyov wrote:
[...]

>    Oops, a stray comma got somehow added to the subject --
> do I need to resend?

   Actually, it got added as I prepared to send the patch from Thunderbird
(git send-email is broken for me again), the patch on my disk is OK... :-/

MBR, Sergey

Re: [PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Fedor Pchelkin]

Hi,

On Fri, 01. May 23:28, Sergey Shtylyov wrote:
> If CONFIG_MEDIA_CONTROLLER is undefined, media_request_object_find() will
> always return NULL, so its 2nd call in v4l2_ctrl_request_complete() would
> fail as well as the 1st one and thus cause hdl to have a wrong value (at
> the top of memory) and list_for_each_entry() to iterate over the garbage
> data located there. Add NULL check for the 2nd call and place the error
> cleanup at the end of v4l2_ctrl_request_complete()...

another way may be to modify the stub version of
media_request_object_bind() to e.g. return -EINVAL instead of 0.  Thus
then v4l2_ctrl_request_bind() will return an error which is already
handled.  Looking at what the API implies, this seems reasonable, too.
Anyway, left for your preference.

Re: [PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Sergey Shtylyov]

On 5/7/26 6:44 PM, Fedor Pchelkin wrote:
[...]

>> If CONFIG_MEDIA_CONTROLLER is undefined, media_request_object_find() will
>> always return NULL, so its 2nd call in v4l2_ctrl_request_complete() would
>> fail as well as the 1st one and thus cause hdl to have a wrong value (at
>> the top of memory) and list_for_each_entry() to iterate over the garbage
>> data located there. Add NULL check for the 2nd call and place the error
>> cleanup at the end of v4l2_ctrl_request_complete()...
> 
> another way may be to modify the stub version of
> media_request_object_bind() to e.g. return -EINVAL instead of 0.  Thus

   You mean ERR_PTR(-EINVAL)? Because it returns a pointer now. :-)

> then v4l2_ctrl_request_bind() will return an error which is already

   Not sure I follow you there...

> handled.  Looking at what the API implies, this seems reasonable, too.
> Anyway, left for your preference.

MBR, Sergey

Re: [PATCH] media: v4l2-ctrls-request: add NULL check in, v4l2_ctrl_request_complete()

[Sergey Shtylyov]

On 5/7/26 7:18 PM, Sergey Shtylyov wrote:
[...]

>>> If CONFIG_MEDIA_CONTROLLER is undefined, media_request_object_find() will
>>> always return NULL, so its 2nd call in v4l2_ctrl_request_complete() would
>>> fail as well as the 1st one and thus cause hdl to have a wrong value (at
>>> the top of memory) and list_for_each_entry() to iterate over the garbage
>>> data located there. Add NULL check for the 2nd call and place the error
>>> cleanup at the end of v4l2_ctrl_request_complete()...
>>
>> another way may be to modify the stub version of
>> media_request_object_bind() to e.g. return -EINVAL instead of 0.  Thus
> 
>    You mean ERR_PTR(-EINVAL)? Because it returns a pointer now. :-)

   Ah, I was looking at the wrong function. Sorry! :-)

[...]

MBR, Sergey