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 <syzkal...@googlegroups.com> > Signed-off-by: Alistair Strachan <astrac...@google.com> > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Cc: Mauro Carvalho Chehab <mche...@kernel.org> > Cc: linux-media@vger.kernel.org > Cc: kernel-t...@android.com > --- > 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 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
