Hello,
On Fri, Nov 08, 2024 at 11:49:01AM +0100, Hans Verkuil wrote:
> On 08/11/2024 11:15, Sakari Ailus wrote:
> > On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote:
> >> 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.
Naughty driver :-)
> >>
> >> 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.
> >
> > Do we support adding new controls after adding the handler or is there a
> > valid use case for it? I'd rather say it's not supported and prevent it,
> > for simplicity. Things like this will likely make it more difficult to move
> > the controls to the device state.
>
> Blocking this completely is out of scope of this patch. I am not quite sure
> if doing that wouldn't break some drivers (in or out of tree).
>
> If this turns out to be an issue when moving controls to the device state,
> then we can revisit this.
I tend to agree with Sakari here. I believe the control framework is
already complex enough, and I don't think we should allow drivers to add
cnotrols after calling v4l2_ctrl_add_handler(). If there are any in-tree
drivers doing so, we can probably fix them fairly easily.
As for generating a warning instead of crashing when the control is
accessed, we could generate a warning if a control is added by the
driver after calling v4l2_ctrl_add_handler(). That could even cause the
control handler to flag an error, and that would be very visible to
driver authors.
> > Cc Laurent and Jacopo.
> >
> >> 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;
> >> }
> >>
--
Regards,
Laurent Pinchart
