[PATCH] media: uvcvideo: Fix 'type' check leading to overflow

[PATCH] media: uvcvideo: Fix 'type' check leading to overflow

[Alistair Strachan]

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@googlegroups.com>
Signed-off-by: Alistair Strachan <astrachan@google.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Cc: kernel-team@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;
-- 
2.20.0.405.gbc1bbc6f85-goog

Re: [PATCH] media: uvcvideo: Fix 'type' check leading to overflow

[Laurent Pinchart]

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@googlegroups.com>
> Signed-off-by: Alistair Strachan <astrachan@google.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: linux-media@vger.kernel.org
> Cc: kernel-team@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

Re: [PATCH] media: uvcvideo: Fix 'type' check leading to overflow

[Alistair Strachan]

Hi Laurent,

On Tue, Dec 18, 2018 at 1:42 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@googlegroups.com>
> > Signed-off-by: Alistair Strachan <astrachan@google.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Cc: linux-media@vger.kernel.org
> > Cc: kernel-team@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

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
>
>
>