linux-kernel - Re: [PATCH v4 1/2] media: uvcvideo: Support partial control reads

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: <20241127091400.GB31095@pendragon.ideasonboard.com>
Date: Wed, 27 Nov 2024 11:14:00 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	Sakari Ailus <sakari.ailus@...ux.intel.com>, stable@...r.kernel.org
Subject: Re: [PATCH v4 1/2] media: uvcvideo: Support partial control reads

On Wed, Nov 27, 2024 at 09:58:21AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 09:34, Laurent Pinchart wrote:
> > On Tue, Nov 26, 2024 at 07:12:53PM +0100, Ricardo Ribalda wrote:
> > > On Tue, 26 Nov 2024 at 19:06, Laurent Pinchart wrote:
> > > > On Wed, Nov 20, 2024 at 03:26:19PM +0000, Ricardo Ribalda wrote:
> > > > > Some cameras, like the ELMO MX-P3, do not return all the bytes
> > > > > requested from a control if it can fit in less bytes.
> > > > > Eg: Returning 0xab instead of 0x00ab.
> > > > > usb 3-9: Failed to query (GET_DEF) UVC control 3 on unit 2: 1 (exp. 2).
> > > > >
> > > > > Extend the returned value from the camera and return it.
> > > > >
> > > > > Cc: stable@...r.kernel.org
> > > > > Fixes: a763b9fb58be ("media: uvcvideo: Do not return positive errors in uvc_query_ctrl()")
> > > > > Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> > > > > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_video.c | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > index cd9c29532fb0..482c4ceceaac 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > @@ -79,6 +79,22 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > > > >       if (likely(ret == size))
> > > > >               return 0;
> > > > >
> > > > > +     /*
> > > > > +      * In UVC the data is usually represented in little-endian.
> > > >
> > > > I had a comment about this in the previous version, did you ignore it on
> > > > purpose because you disagreed, or was it an oversight ?
> > >
> > > I rephrased the comment. I added "usually" to make it clear that it
> > > might not be the case for all the data types. Like composed or xu.
> >
> > Ah, that's what you meant by "usually". I read it as "usually in
> > little-endian, but could be big-endian too", which confused me.
> >
> > Data types that are not integers will not work nicely with the
> > workaround below. How do you envision that being handled ? Do you
> > consider that the device will return too few bytes only for integer data
> > types, or that affected devices don't have controls that use compound
> > data types ? I don't see what else we could do so I'd be fine with such
> > a heuristic for this workaround, but it needs to be clearly explained.
> 
> Non integer datatypes might work if the last part of the data is
> expected to be zero.
> I do not think that we can find a heuristic that can work for all the cases.
> 
> For years we have ignored partial reads and it has never been an
> issue. I vote for not adding any heuristics, the logging should help
> identify future issues (if there is any).

What you're doing below is already a heuristic :-) I don't think the
code needs to be changed, but I'd like this comment to explain why we
consider that the heuristic in this patch is fine, to help the person
(possibly you or me) who will read this code in a year and wonder what's
going on.

> > > I also r/package/packet/
> > >
> > > Did I miss another comment?
> > >
> > > > > +      * Some devices return shorter USB control packets that expected if the
> > > > > +      * returned value can fit in less bytes. Zero all the bytes that the
> > > > > +      * device have not written.
> > > >
> > > > s/have/has/
> > > >
> > > > And if you meant to start a new paragraph here, a blank line is missing.
> > > > Otherwise, no need to break the line before 80 columns.
> > >
> > > The patch is already in the uvc tree. How do you want to handle this?
> >
> > The branch shared between Hans and me can be rebased, it's a staging
> > area.
> 
> I will send a new version, fixing the typo. and the missing new line.
> I will also remove the sentence
> `* In UVC the data is usually represented in little-endian.`
> It is confusing.
> 
> > > > > +      * We exclude UVC_GET_INFO from the quirk. UVC_GET_LEN does not need to
> > > > > +      * be excluded because its size is always 1.
> > > > > +      */
> > > > > +     if (ret > 0 && query != UVC_GET_INFO) {
> > > > > +             memset(data + ret, 0, size - ret);
> > > > > +             dev_warn_once(&dev->udev->dev,
> > > > > +                           "UVC non compliance: %s control %u on unit %u returned %d bytes when we expected %u.\n",
> > > > > +                           uvc_query_name(query), cs, unit, ret, size);
> > > > > +             return 0;
> > > > > +     }
> > > > > +
> > > > >       if (ret != -EPIPE) {
> > > > >               dev_err(&dev->udev->dev,
> > > > >                       "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",

-- 
Regards,

Laurent Pinchart