Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Alistair Strachan <astrachan@google.com>
Cc:     linux-kernel@vger.kernel.org, syzbot <syzkaller@googlegroups.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH] media: uvcvideo: Fix 'type' check leading to overflow
Date:   Tue, 18 Dec 2018 11:42:54 +0200
Message-ID: <45456214.XvNNoR8qLh@avalon>
Organization: Ideas on Board Oy
In-Reply-To: <20181217210222.115419-1-astrachan@google.com>
References: <20181217210222.115419-1-astrachan@google.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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