An out-of-tree driver created a control handler, added controls, then
called v4l2_ctrl_add_handler to add references to controls from another
handler, and finally added another control that happened to have the same
control ID as one of the controls from that other handler.
This caused a NULL pointer crash when an attempt was made to use that last
control.
Besides the fact that two out-of-tree drivers used the same control ID for
different (private) controls, which is obviously a bug, this specific
scenario should have been caught. The root cause is the 'duplicate ID'
check in handler_new_ref(): it expects that drivers will first add all
controls to a control handler before calling v4l2_ctrl_add_handler. That
way the local controls will always override references to controls from
another handler.
It never considered the case where new local controls were added after
calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return
an error in the case that a new local control is added with the same ID as
an existing control reference. Also use WARN_ON since this is a driver bug.
This situation can only happen when out-of-tree drivers are used or during
driver development, since mainlined drivers all have their own control
ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs.
Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx>
---
Changes since v1:
Improved the comment.
---
drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++----
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index eeab6a5eb7ba..8fac12e78481 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl,
u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1;
int bucket = id % hdl->nr_of_buckets; /* which bucket to use */
unsigned int size_extra_req = 0;
+ int ret = 0;
if (ctrl_ref)
*ctrl_ref = NULL;
@@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl,
list_for_each_entry(ref, &hdl->ctrl_refs, node) {
if (ref->ctrl->id < id)
continue;
- /* Don't add duplicates */
- if (ref->ctrl->id == id) {
- kfree(new_ref);
- goto unlock;
+ /* Check we're not adding a duplicate */
+ if (ref->ctrl->id != id) {
+ list_add(&new_ref->node, ref->node.prev);
+ break;
}
- list_add(&new_ref->node, ref->node.prev);
- break;
+
+ /*
+ * If we add a new control to this control handler, and we find
+ * that it is a duplicate, then that is a driver bug. Warn and
+ * return an error.
+ *
+ * It can be caused by either adding the same control twice, or
+ * by first calling v4l2_ctrl_add_handler, and then adding a new
+ * control to this control handler.
+ *
+ * Either sequence is incorrect.
+ *
+ * However, if the control is owned by another handler, and
+ * a control with that ID already exists in the list, then we
+ * can safely skip it: in that case it the control is overridden
+ * by the existing control.
+ */
+ if (WARN_ON(hdl == ctrl->handler))
+ ret = -EEXIST;
+ kfree(new_ref);
+ goto unlock;
}
insert_in_hash:
@@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl,
unlock:
mutex_unlock(hdl->lock);
+ if (ret)
+ return handler_set_err(hdl, ret);
return 0;
}
--
2.45.2
