| 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: <CANiDSCu4krUbXq_RiaP5dSZVGdA6CxmUBzxEEPqMNCS4Ar05mw@mail.gmail.com>
Date: Thu, 19 Dec 2024 09:17:31 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans Verkuil <hverkuil@...all.nl>, 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] media: uvcvideo: Filter hw errors while enumerating controls
On Thu, 19 Dec 2024 at 00:27, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Dec 13, 2024 at 11:21:02AM +0000, Ricardo Ribalda wrote:
> > To implement VIDIOC_QUERYCTRL, we need to read from the hardware all the
> > values that were not cached previously. If that read fails, we used to
> > report back the error to the user.
> >
> > Unfortunately this does not play nice with userspace. When they are
> > enumerating the contols, the only expect an error when there are no
> > "next" control.
> >
> > This is probably a corner case, and could be handled in userspace, but
> > both v4l2-ctl and yavta fail to enumerate all the controls if we return
> > then -EIO during VIDIOC_QUERYCTRL. I suspect that there are tons of
> > userspace apps handling this wrongly as well.
> >
> > This patch get around this issue by ignoring the hardware errors and
> > always returning 0 if a control exists.
> >
> > 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.
>
> Was it transient and random errors, or does the device always fail for
> those controls ?
For one of the devices the control is always failing (or I could not
find a combination that made it work).
For the other it was more or less random.
>
> > This could be solved in userspace.. but I suspect that a lot of people
> > has copied the implementation of v4l-utils or yavta.
> >
> > What do you think of this workaround?
>
> Pretending that the control could be queried is problematic. We'll
> return invalid values to the user, I don't think that's a good idea. If
> the problematic device always returns error for focus controls, we could
> add a quirk, and extend the uvc_device_info structure to list the
> controls to ignore.
>
> Another option would be to skip over controls that return -EIO within
> the kernel, and mark those controls are broken. I think this could be
> done transparently for userspace, the first time we try to populate the
> cache for such controls, a -EIO error would mark the control as broken,
> and from a userspace point of view it wouldn't be visible through as
> ioctl.
I see a couple of issues with this:
- There are controls that fail randomly.
- There are controls that fail based on the value of other controls
(yeah, I know).
- There are controls that do not implement RES, MIN, or MAX, but
besides that, they are completely functional.
In any of those cases we do not want to skip those controls.
I am not against quirking specific cameras once we detect that they
are broken... but we still need a solution for every camera that
allows enumerating all the controls, even if there is a non compliant
control.
Now that I look into the patch... I think I have to remove the
can_fail argument and let it always remove invalid values.
>
> > 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
> >
> > --
> > ---
> > drivers/media/usb/uvc/uvc_ctrl.c | 31 ++++++++++++++++++++++---------
> > 1 file changed, 22 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 4fe26e82e3d1..a11048c9a105 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1283,7 +1283,8 @@ static u32 uvc_get_ctrl_bitmap(struct uvc_control *ctrl,
> > static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > struct uvc_control *ctrl,
> > struct uvc_control_mapping *mapping,
> > - struct v4l2_queryctrl *v4l2_ctrl)
> > + struct v4l2_queryctrl *v4l2_ctrl,
> > + bool can_fail)
> > {
> > struct uvc_control_mapping *master_map = NULL;
> > struct uvc_control *master_ctrl = NULL;
> > @@ -1307,17 +1308,28 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > if (master_ctrl && (master_ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR)) {
> > s32 val;
> > int ret = __uvc_ctrl_get(chain, master_ctrl, master_map, &val);
> > - if (ret < 0)
> > - return ret;
> >
> > - if (val != mapping->master_manual)
> > - v4l2_ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > + if (ret < 0) {
> > + dev_warn_ratelimited(&chain->dev->udev->dev,
> > + "UVC non compliance: Error %d querying master control %x\n",
> > + ret, master_map->id);
> > + if (can_fail)
> > + return ret;
> > + } 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;
> > +
> > + if (ret < 0) {
> > + dev_warn_ratelimited(&chain->dev->udev->dev,
> > + "UVC non compliance: Error %d populating cache of control %x\n",
> > + ret, mapping->id);
> > + if (can_fail)
> > + return ret;
> > + }
> > }
> >
> > if (ctrl->info.flags & UVC_CTRL_FLAG_GET_DEF) {
> > @@ -1420,7 +1432,8 @@ int uvc_query_v4l2_ctrl(struct uvc_video_chain *chain,
> > goto done;
> > }
> >
> > - ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl);
> > + ret = __uvc_query_v4l2_ctrl(chain, ctrl, mapping, v4l2_ctrl,
> > + !(v4l2_ctrl->id & V4L2_CTRL_FLAG_NEXT_CTRL));
> > done:
> > mutex_unlock(&chain->ctrl_mutex);
> > return ret;
> > @@ -1513,7 +1526,7 @@ static void uvc_ctrl_fill_event(struct uvc_video_chain *chain,
> > {
> > struct v4l2_queryctrl v4l2_ctrl;
> >
> > - __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &v4l2_ctrl);
> > + __uvc_query_v4l2_ctrl(chain, ctrl, mapping, &v4l2_ctrl, true);
> >
> > memset(ev, 0, sizeof(*ev));
> > ev->type = V4L2_EVENT_CTRL;
> >
> > ---
> > base-commit: a7f5b36b34824415c28875d615c49a3cf5070615
> > change-id: 20241213-uvc-eaccess-755cc061a360
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Powered by blists - more mailing lists