| 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: <332fce27-8a11-4c70-9753-1da968c3e45a@xs4all.nl>
Date: Thu, 3 Apr 2025 09:01:06 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Ricardo Ribalda <ribalda@...omium.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans de Goede <hdegoede@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: 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 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.
> --
> ---
> 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.
> + }
> +
> + 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?
> + } 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
Powered by blists - more mailing lists