linux-kernel - Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by other fh

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiDSCuukO7kQw=VHyHS3ir3y4mCERbnRoUTTTSoRHZkMpLBdg@mail.gmail.com>
Date: Wed, 27 Nov 2024 11:19:55 +0100
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>, 
	Mauro Carvalho Chehab <mchehab+samsung@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] media: uvcvideo: Do not set an async control owned by
 other fh

On Wed, 27 Nov 2024 at 10:42, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
> > On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
> > > On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
> > > > If a file handle is waiting for a response from an async control, avoid
> > > > that other file handle operate with it.
> > > >
> > > > Without this patch, the first file handle will never get the event
> > > > associated to that operation.
> > >
> > > Please explain why that is an issue (both for the commit message and for
> > > me, as I'm not sure what you're fixing here).
> >
> > What about something like this:
> >
> > Without this patch, the first file handle will never get the event
> > associated with that operation, which can lead to endless loops in
> > applications. Eg:
> > If an application A wants to change the zoom and to know when the
> > operation has completed:
> > it will open the video node, subscribe to the zoom event, change the
> > control and wait for zoom to finish.
> > If before the zoom operation finishes, another application B changes
> > the zoom, the first app A will loop forever.
>
> So it's related to the userspace-visible behaviour, there are no issues
> with this inside the kernel ?

It is also required by the granular PM patchset.


>
> Applications should in any case implement timeouts, as UVC devices are
> fairly unreliable. What bothers me with this patch is that if the device
> doesn't generate the event, ctrl->handle will never be reset to NULL,
> and the control will never be settable again. I think the current
> behaviour is a lesser evil.

The same app can set the control as many times as it wants, and if
that app closes the next patch will clean out the handle.
Maybe I should invert the patches?


>
> > > What may be an issue is that ctrl->handle seem to be accessed from
> > > different contexts without proper locking :-S
> >
> > Isn't it always protected by ctrl_mutex?
>
> Not that I can tell. At least uvc_ctrl_status_event_async() isn't called
> with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the
> beginning of the function wouldn't hurt).

ctrl->handle = NULL in uvc_ctrl_status_event_async() is completely redundant.

The only place we set the value of the handle is in uvc_ctrl_set, and
that can only happen for controls with mappings.
I am sending another patch to remove that set to make it clear.

>
> As uvc_ctrl_status_event_async() is the URB completion handler, a
> spinlock would be nicer than a mutex to protect ctrl->handle.
>
> > > > Cc: stable@...r.kernel.org
> > > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 4fe26e82e3d1..5d3a28edf7f0 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > >       if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> > > >               return -EACCES;
> > > >
> > > > +     /* Other file handle is waiting a response from this async control. */
> > > > +     if (ctrl->handle && ctrl->handle != handle)
> > > > +             return -EBUSY;
> > > > +
> > > >       /* Clamp out of range values. */
> > > >       switch (mapping->v4l2_type) {
> > > >       case V4L2_CTRL_TYPE_INTEGER:
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda