[PATCH for v5.12] v4l2-ctrls.c: fix race condition in hdl->requests list

[PATCH for v5.12] v4l2-ctrls.c: fix race condition in hdl->requests list

[Hans Verkuil]

When a request is re-inited it will release all control handler
objects that are still in the request. It does that by unbinding
and putting all those objects. When the object is unbound the
obj->req pointer is set to NULL, and the object's unbind op is
called. When the object it put the object's release op is called
to free the memory.

For a request object that contains a control handler that means
that v4l2_ctrl_handler_free() is called in the release op.

A control handler used in a request has a pointer to the main
control handler that is created by the driver and contains the
current state of all controls. If the device is unbound (due to
rmmod or a forced unbind), then that main handler is freed, again
by calling v4l2_ctrl_handler_free(), and any outstanding request
objects that refer to that main handler have to be unbound and put
as well.

It does that by this test:

	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {

I.e. the handler has no pointer to a request, so is the main
handler, and one or more request objects refer to this main
handler.

However, this test is wrong since hdl->req_obj.req is actually
NULL when re-initing a request (the object unbind will set req to
NULL), and the only reason this seemingly worked is that the
requests list is typically empty since the request's unbind op
will remove the handler from the requests list.

But if another thread is at the same time adding a new control
to a request, then there is a race condition where one thread
is removing a control handler object from the requests list and
another thread is adding one. The result is that hdl->requests
is no longer empty and the code thinks that a main handler is
being freed instead of a control handler that is part of a request.

There are two bugs here: first the test for hdl->req_obj.req: this
should be hdl->req_obj.ops since only the main control handler will
have a NULL pointer there.

The second is that adding or deleting request objects from the
requests list of the main handler isn't protected by taking the
main handler's lock.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: John Cox <jsc66@cam.ac.uk>
Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 39038c6ad8fb..757d215c2be4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2560,7 +2560,15 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 	if (hdl == NULL || hdl->buckets == NULL)
 		return;

-	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
+	/*
+	 * If the main handler is freed and it is used by handler objects in
+	 * outstanding requests, then unbind and put those objects before
+	 * freeing the main handler.
+	 *
+	 * The main handler can be identified by having a NULL ops pointer in
+	 * the request object.
+	 */
+	if (!hdl->req_obj.ops && !list_empty(&hdl->requests)) {
 		struct v4l2_ctrl_handler *req, *next_req;

 		list_for_each_entry_safe(req, next_req, &hdl->requests, requests) {
@@ -3634,8 +3642,8 @@ static void v4l2_ctrl_request_unbind(struct media_request_object *obj)
 		container_of(obj, struct v4l2_ctrl_handler, req_obj);
 	struct v4l2_ctrl_handler *main_hdl = obj->priv;

-	list_del_init(&hdl->requests);
 	mutex_lock(main_hdl->lock);
+	list_del_init(&hdl->requests);
 	if (hdl->request_is_queued) {
 		list_del_init(&hdl->requests_queued);
 		hdl->request_is_queued = false;
@@ -3694,8 +3702,11 @@ static int v4l2_ctrl_request_bind(struct media_request *req,
 	if (!ret) {
 		ret = media_request_object_bind(req, &req_ops,
 						from, false, &hdl->req_obj);
-		if (!ret)
+		if (!ret) {
+			mutex_lock(from->lock);
 			list_add_tail(&hdl->requests, &from->requests);
+			mutex_unlock(from->lock);
+		}
 	}
 	return ret;
 }
-- 
2.30.1

Re: [PATCH for v5.12] v4l2-ctrls.c: fix race condition in hdl->requests list

[John Cox]

On Sat, 27 Mar 2021 12:27:40 +0100, you wrote:

>When a request is re-inited it will release all control handler
>objects that are still in the request. It does that by unbinding
>and putting all those objects. When the object is unbound the
>obj->req pointer is set to NULL, and the object's unbind op is
>called. When the object it put the object's release op is called
>to free the memory.
>
>For a request object that contains a control handler that means
>that v4l2_ctrl_handler_free() is called in the release op.
>
>A control handler used in a request has a pointer to the main
>control handler that is created by the driver and contains the
>current state of all controls. If the device is unbound (due to
>rmmod or a forced unbind), then that main handler is freed, again
>by calling v4l2_ctrl_handler_free(), and any outstanding request
>objects that refer to that main handler have to be unbound and put
>as well.
>
>It does that by this test:
>
>	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
>
>I.e. the handler has no pointer to a request, so is the main
>handler, and one or more request objects refer to this main
>handler.
>
>However, this test is wrong since hdl->req_obj.req is actually
>NULL when re-initing a request (the object unbind will set req to
>NULL), and the only reason this seemingly worked is that the
>requests list is typically empty since the request's unbind op
>will remove the handler from the requests list.
>
>But if another thread is at the same time adding a new control
>to a request, then there is a race condition where one thread
>is removing a control handler object from the requests list and
>another thread is adding one. The result is that hdl->requests
>is no longer empty and the code thinks that a main handler is
>being freed instead of a control handler that is part of a request.
>
>There are two bugs here: first the test for hdl->req_obj.req: this
>should be hdl->req_obj.ops since only the main control handler will
>have a NULL pointer there.
>
>The second is that adding or deleting request objects from the
>requests list of the main handler isn't protected by taking the
>main handler's lock.
>
>Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>Reported-by: John Cox <jsc66@cam.ac.uk>
>Fixes: 6fa6f831f095 ("media: v4l2-ctrls: add core request support")
>---
> drivers/media/v4l2-core/v4l2-ctrls.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>index 39038c6ad8fb..757d215c2be4 100644
>--- a/drivers/media/v4l2-core/v4l2-ctrls.c
>+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>@@ -2560,7 +2560,15 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> 	if (hdl == NULL || hdl->buckets == NULL)
> 		return;
>
>-	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
>+	/*
>+	 * If the main handler is freed and it is used by handler objects in
>+	 * outstanding requests, then unbind and put those objects before
>+	 * freeing the main handler.
>+	 *
>+	 * The main handler can be identified by having a NULL ops pointer in
>+	 * the request object.
>+	 */
>+	if (!hdl->req_obj.ops && !list_empty(&hdl->requests)) {
> 		struct v4l2_ctrl_handler *req, *next_req;
>
> 		list_for_each_entry_safe(req, next_req, &hdl->requests, requests) {
>@@ -3634,8 +3642,8 @@ static void v4l2_ctrl_request_unbind(struct media_request_object *obj)
> 		container_of(obj, struct v4l2_ctrl_handler, req_obj);
> 	struct v4l2_ctrl_handler *main_hdl = obj->priv;
>
>-	list_del_init(&hdl->requests);
> 	mutex_lock(main_hdl->lock);
>+	list_del_init(&hdl->requests);
> 	if (hdl->request_is_queued) {
> 		list_del_init(&hdl->requests_queued);
> 		hdl->request_is_queued = false;
>@@ -3694,8 +3702,11 @@ static int v4l2_ctrl_request_bind(struct media_request *req,
> 	if (!ret) {
> 		ret = media_request_object_bind(req, &req_ops,
> 						from, false, &hdl->req_obj);
>-		if (!ret)
>+		if (!ret) {
>+			mutex_lock(from->lock);
> 			list_add_tail(&hdl->requests, &from->requests);
>+			mutex_unlock(from->lock);
>+		}
> 	}
> 	return ret;
> }

Tested-by: John Cox <jc@kynesim.co.uk>

Could you also change the report to

Reported-by: John Cox <jc@kynesim.co.uk>

Many thanks

John Cox