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

[Kleber Souza]

On 7/18/19 11:27 AM, Paolo Pisati wrote:
> From: Alistair Strachan <astrachan at google.com>
> 
> 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.
> 
> Avoid this problem by checking for the MSB in the wTerminalType field.
> If the bit is set, assume the descriptor is bad, and abort parsing it.
> 
> 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 at googlegroups.com>
> Signed-off-by: Alistair Strachan <astrachan at google.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung at kernel.org>
> 
> CVE-2019-2101
> 
> (cherry picked from commit 47bb117911b051bbc90764a8bff96543cbd2005f)
> Signed-off-by: Paolo Pisati <paolo.pisati at canonical.com>

Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>


> ---
>  drivers/media/usb/uvc/uvc_driver.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 28b91b7d756f..1778fdeded94 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1054,11 +1054,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,
>