Hi Laurent,
On 08/11/2024 14:28, Laurent Pinchart wrote:
> 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.
While I agree with this, I don't want to do this without first doing
some analysis for existing drivers.
Would you be OK with me merging this patch, and that I do the analysis later
and post a follow-up patch?
This issue causes a somewhat hard-to-find crash and it hit me twice
within a week.
Regards,
Hans
>
>>> 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;
>>>> }
>>>>
>
