Hi Laurent, On Tue, Dec 18, 2018 at 1:42 AM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Alistair, > > Thank you for the patch. > > On Monday, 17 December 2018 23:02:22 EET Alistair Strachan wrote: > > When initially testing the Camera Terminal Descriptor wTerminalType > > field (buffer[4]), no mask is used. Later in the function, the MSB is > > overloaded to store the descriptor subtype, and so a mask of 0x7fff > > is used to check the type. > > > > If a descriptor is specially crafted to set this overloaded bit in the > > original wTerminalType field, the initial type check will fail (falling > > through, without adjusting the buffer size), but the later type checks > > will pass, assuming the buffer has been made suitably large, causing an > > overflow. > > > > This problem could be resolved in a few different ways, but this fix > > applies the same initial type check as used by UVC_ENTITY_TYPE (once we > > have a 'term' structure.) Such crafted wTerminalType fields will then be > > treated as *generic* Input Terminals, not as CAMERA or > > MEDIA_TRANSPORT_INPUT, avoiding an overflow. > > > > Originally reported here: > > https://groups.google.com/forum/#!topic/syzkaller/Ot1fOE6v1d8 > > A similar (non-compiling) patch was provided at that time. > > > > Reported-by: syzbot <syzkaller@xxxxxxxxxxxxxxxx> > > Signed-off-by: Alistair Strachan <astrachan@xxxxxxxxxx> > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> > > Cc: linux-media@xxxxxxxxxxxxxxx > > Cc: kernel-team@xxxxxxxxxxx > > --- > > drivers/media/usb/uvc/uvc_driver.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c > > b/drivers/media/usb/uvc/uvc_driver.c index bc369a0934a3..279a967b8264 > > 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -1082,11 +1082,11 @@ static int uvc_parse_standard_control(struct > > uvc_device *dev, p = 0; > > len = 8; > > > > - if (type == UVC_ITT_CAMERA) { > > + if ((type & 0x7fff) == UVC_ITT_CAMERA) { > > n = buflen >= 15 ? buffer[14] : 0; > > len = 15; > > > > - } else if (type == UVC_ITT_MEDIA_TRANSPORT_INPUT) { > > + } else if ((type & 0x7fff) == UVC_ITT_MEDIA_TRANSPORT_INPUT) { > > n = buflen >= 9 ? buffer[8] : 0; > > p = buflen >= 10 + n ? buffer[9+n] : 0; > > len = 10; > > How about rejecting invalid types instead ? Something along the lines of This looks simpler/better to me. I just re-sent your version of the change (build + runtime tested.) Thanks for reviewing. > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index b62cbd800111..33a22c016456 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1106,11 +1106,19 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > return -EINVAL; > } > > - /* Make sure the terminal type MSB is not null, otherwise it > - * could be confused with a unit. > + /* > + * Reject invalid terminal types that would cause issues: > + * > + * - The high byte must be non-zero, otherwise it would be > + * confused with a unit. > + * > + * - Bit 15 must be 0, as we use it internally as a terminal > + * direction flag. > + * > + * Other unknown types are accepted. > */ > type = get_unaligned_le16(&buffer[4]); > - if ((type & 0xff00) == 0) { > + if ((type & 0x7f00) == 0 || (type & 0x8000) != 0) { > uvc_trace(UVC_TRACE_DESCR, "device %d videocontrol " > "interface %d INPUT_TERMINAL %d has invalid " > "type 0x%04x, skipping\n", udev->devnum, > > -- > Regards, > > Laurent Pinchart > > >
