On Fri, Nov 08, 2024 at 03:38:27PM +0100, Hans Verkuil wrote:
> On 08/11/2024 14:28, Laurent Pinchart wrote:
> > 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.
>
> While I agree with this, I don't want to do this without first doing
> some analysis for existing drivers.
Better safe than sorry, sure.
> Would you be OK with me merging this patch, and that I do the analysis later
> and post a follow-up patch?
I'm OK with that. Could you then mention in the comment that adding
controls after calling v4l2_ctrl_add_handler() isn't allowed ? That will
make me feel better about people not getting the wrong impression.
> This issue causes a somewhat hard-to-find crash and it hit me twice
> within a week.
>
> >>> 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
