| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <003d5c4d-d277-4555-82aa-0d21dce64577@xs4all.nl>
Date: Thu, 3 Apr 2025 10:12:03 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans de Goede <hdegoede@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] media: uvcvideo: Set V4L2_CTRL_FLAG_DISABLED during
queryctrl errors
On 03/04/2025 09:59, Ricardo Ribalda wrote:
> On Thu, 3 Apr 2025 at 09:01, Hans Verkuil <hverkuil@...all.nl> wrote:
>>
>> On 10/03/2025 14:21, Ricardo Ribalda wrote:
>>> To implement VIDIOC_QUERYCTRL, we need to know the minimum, maximum,
>>> step and flags of the control. For some of the controls, this involves
>>> querying the actual hardware.
>>>
>>> Some non-compliant cameras produce errors when we query them. These
>>> error can be triggered every time, sometimes, or when other controls do
>>> not have the "right value". Right now, we populate that error to userspace.
>>> When an error happens, the v4l2 framework does not copy the v4l2_queryctrl
>>> struct to userspace. Also, userspace apps are not ready to handle any
>>> other error than -EINVAL.
>>>
>>> One of the main usecases of VIDIOC_QUERYCTRL is enumerating the controls
>>> of a device. This is done using the V4L2_CTRL_FLAG_NEXT_CTRL flag. In
>>> that usecase, a non-compliant control will make it almost impossible to
>>> enumerate all controls of the device.
>>>
>>> A control with an invalid max/min/step/flags is better than non being
>>> able to enumerate the rest of the controls.
>>>
>>> This patch:
>>> - Retries for an extra attempt to read the control, to avoid spurious
>>> errors. More attempts do not seem to produce better results in the
>>> tested hardware.
>>> - Makes VIDIOC_QUERYCTRL return 0 in all the error cases different than
>>> -EINVAL.
>>> - Introduces a warning in dmesg so we can have a trace of what has happened
>>> and sets the V4L2_CTRL_FLAG_DISABLED.
>>> - Makes sure we keep returning V4L2_CTRL_FLAG_DISABLED for all the next
>>> attempts to query that control (other operations have the same
>>> functionality as now).
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
>>> ---
>>> Hi 2*Hans and Laurent!
>>>
>>> I came around a device that was listing just a couple of controls when
>>> it should be listing much more.
>>>
>>> Some debugging latter I found that the device was returning -EIO when
>>> all the focal controls were read.
>>>
>>> Lots of good arguments in favor/against this patch in the v1. Please
>>> check!
>>>
>>> Without this patch:
>>> $ v4l2-ctl --list-ctrls
>>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>>> exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive
>>> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>>>
>>> With this patch:
>>> $ v4l2-ctl --list-ctrls
>>> auto_exposure 0x009a0901 (menu) : min=0 max=3 default=3 value=3 (Aperture Priority Mode)
>>> exposure_time_absolute 0x009a0902 (int) : min=50 max=10000 step=1 default=166 value=166 flags=inactive
>>> exposure_dynamic_framerate 0x009a0903 (bool) : default=0 value=0
>>> error 5 getting ext_ctrl Focus, Absolute
>>> error 5 getting ext_ctrl Focus, Automatic Continuous
>>> region_of_interest_rectangle 0x009a1901 (unknown): type=107 value=unsupported payload type flags=has-payload
>>> region_of_interest_auto_control 0x009a1902 (bitmask): max=0x00000001 default=0x00000001 value=1
>>
>> This output still refers to the result from the v1 patch (I think).
>> Can you redo this test with this v6 applied? You probably want to update these
>> comments anyway.
>
> I do not have access to the broken device right now. But I will fake a
> -EIO error.
>
>>
>>> --
>>> ---
>>> Changes in v6:
>>> - Keep returning V4L2_CTRL_FLAG_DISABLED in future control queries.
>>> - Link to v5: https://lore.kernel.org/r/20250224-uvc-eaccess-v5-1-690d73bcef28@chromium.org
>>>
>>> Changes in v5:
>>> - Explain the retry in the commit message (Thanks Laurent).
>>> - Link to v4: https://lore.kernel.org/r/20250111-uvc-eaccess-v4-1-c7759bfd1bd4@chromium.org
>>>
>>> Changes in v4:
>>> - Display control name (Thanks Hans)
>>> - Link to v3: https://lore.kernel.org/r/20250107-uvc-eaccess-v3-1-99f3335d5133@chromium.org
>>>
>>> Changes in v3:
>>> - Add a retry mechanism during error.
>>> - Set V4L2_CTRL_FLAG_DISABLED flag.
>>> - Link to v2: https://lore.kernel.org/r/20241219-uvc-eaccess-v2-1-bf6520c8b86d@chromium.org
>>>
>>> Changes in v2:
>>> - Never return error, even if we are not enumerating the controls
>>> - Improve commit message.
>>> - Link to v1: https://lore.kernel.org/r/20241213-uvc-eaccess-v1-1-62e0b4fcc634@chromium.org
>>> ---
>>> drivers/media/usb/uvc/uvc_ctrl.c | 46 +++++++++++++++++++++++++++++++++-------
>>> drivers/media/usb/uvc/uvcvideo.h | 2 ++
>>> 2 files changed, 40 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
>>> index 4e58476d305efddac331417feda8cb064e340a13..4b282ac714220b26581fe468d9ecb1109a28483f 100644
>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
>>> @@ -1280,6 +1280,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
>>> return ~0;
>>> }
>>>
>>> +#define MAX_QUERY_RETRIES 2
>>
>> As you mentioned in the commit log, trying more times didn't make a difference.
>> Add that as a comment here as well, it is good to have that documented in the code.
>>
>>> +
>>> static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> struct uvc_control *ctrl,
>>> struct uvc_control_mapping *mapping,
>>> @@ -1305,19 +1307,44 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> __uvc_find_control(ctrl->entity, mapping->master_id,
>>> &master_map, &master_ctrl, 0);
>>> if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
>>> + unsigned int retries;
>>> s32 val;
>>> - int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
>>> - if (ret < 0)
>>> - return ret;
>>> + int ret;
>>>
>>> - if (val != mapping->master_manual)
>>> - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> + ret = __uvc_ctrl_get(chain, master_ctrl, master_map,
>>> + &val);
>>> + if (ret >= 0)
>>> + break;
>>
>> This retries regardless of the error code. Isn't a retry only needed for -EIO?
>> Are there other error codes that need a retry? Or, perhaps easier, are there
>> errors codes for which a retry is *not* appropriate? E.g. -EACCESS, -ERANGE.
>
> So far I have only seen -EIO, so i will start with -EIO, if we see
> other behaviour we can fix this later.
>
>
>>
>>> + }
>>> +
>>> + if (ret < 0) {
>>> + dev_warn_ratelimited(&chain->dev->udev->dev,
>>> + "UVC non compliance: Error %d querying master control %x (%s)\n",
>>> + ret, master_map->id,
>>> + uvc_map_get_name(master_map));
>>
>> Shouldn't you mark this control as disabled and return here instead of
>> continuing?
>
> Do you mean marking as disabled the master control or the actual control?
>
> I think there might be cases where the master control is "disabled",
> but the actual control works.
> Semantically could be similar to cameras that do not have the
> master_control, and we support that usecase today.
>
> I am marking the master_map as disabled.
Ah, I missed that there are two different controls here. Just ignore
my comment.
Regards,
Hans
>
>>
>>> + } else if (val != mapping->master_manual) {
>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> + }
>>> }
>>>
>>> if (!ctrl->cached) {
>>> - int ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> - if (ret < 0)
>>> - return ret;
>>> + unsigned int retries;
>>> + int ret;
>>> +
>>> + for (retries = 0; retries < MAX_QUERY_RETRIES; retries++) {
>>> + ret = uvc_ctrl_populate_cache(chain, ctrl);
>>> + if (ret >= 0)
>>> + break;
>>
>> Same question about the error code as above.
>>
>>> + }
>>> +
>>> + if (ret < 0) {
>>> + dev_warn_ratelimited(&chain->dev->udev->dev,
>>> + "UVC non compliance: permanently disabling control %x (%s), due to error %d\n",
>>> + mapping->id,
>>> + uvc_map_get_name(mapping), ret);
>>> + mapping->disabled = true;
>>> + }
>>> }
>>>
>>> if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
>>> @@ -1325,6 +1352,9 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
>>> uvc_ctrl_data(ctrl, UVC_CTRL_DATA_DEF));
>>> }
>>>
>>> + if (mapping->disabled)
>>> + v4l2_ctrl->flags |= V4L2_CTRL_FLAG_DISABLED;
>>> +
>>> switch (mapping->v4l2_type) {
>>> case V4L2_CTRL_TYPE_MENU:
>>> v4l2_ctrl->minimum = ffs(mapping->menu_mask) - 1;
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 5e388f05f3fcaf0e4c503a34745d05837ecb0184..63687d7e97738a50d037b1f456f5215241909c13 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -129,6 +129,8 @@ struct uvc_control_mapping {
>>> s32 master_manual;
>>> u32 slave_ids[2];
>>>
>>> + bool disabled;
>>> +
>>> const struct uvc_control_mapping *(*filter_mapping)
>>> (struct uvc_video_chain *chain,
>>> struct uvc_control *ctrl);
>>>
>>> ---
>>> base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
>>> change-id: 20241213-uvc-eaccess-755cc061a360
>>>
>>> Best regards,
>>
>> Regards,
>>
>> Hans
>
>
> Thanks :)
>
>
Powered by blists - more mailing lists