[PATCH v1 0/2] Skip parsing frames of type UVC_VS_UNDEFINED in

[PATCH v1 0/2] Skip parsing frames of type UVC_VS_UNDEFINED in

[Benoit Sevens]

This can lead to out of bounds writes since frames of this type were not
taken into account when calculating the size of the frames buffer in
uvc_parse_streaming.

I am proposing a series of 2 patches. First patch is very small and
fixes the issue. The idea is to have this patch merged quickly.

The second patch refactors the code into a new uvc_parse_frame function.

I do not have the hardware setup to test this out. The second patch
should definitely be tested.

Benoit Sevens (2):
  media: uvcvideo: Skip parsing frames of type UVC_VS_UNDEFINED in
    uvc_parse_format
  media: uvcvideo: Refactor frame parsing code into a uvc_parse_frame
    function

 drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++-------------
 1 file changed, 123 insertions(+), 105 deletions(-)

-- 
2.47.0.277.g8800431eea-goog

[PATCH v1 1/2] media: uvcvideo: Skip parsing frames of type UVC_VS_UNDEFINED in uvc_parse_format

[Benoit Sevens]

This can lead to out of bounds writes since frames of this type were not
taken into account when calculating the size of the frames buffer in
uvc_parse_streaming.

Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
Signed-off-by: Benoit Sevens <bsevens@google.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 0fac689c6350..13db0026dc1a 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -371,7 +371,7 @@ static int uvc_parse_format(struct uvc_device *dev,
 	 * Parse the frame descriptors. Only uncompressed, MJPEG and frame
 	 * based formats have frame descriptors.
 	 */
-	while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
+	while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
 	       buffer[2] == ftype) {
 		unsigned int maxIntervalIndex;
 
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v1 1/2] media: uvcvideo: Skip parsing frames of type UVC_VS_UNDEFINED in uvc_parse_format

[Greg KH]

On Thu, Nov 07, 2024 at 02:22:02PM +0000, Benoit Sevens wrote:
> This can lead to out of bounds writes since frames of this type were not
> taken into account when calculating the size of the frames buffer in
> uvc_parse_streaming.
> 
> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> Signed-off-by: Benoit Sevens <bsevens@google.com>

Cc: stable <stable@kernel.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v1 1/2] media: uvcvideo: Skip parsing frames of type UVC_VS_UNDEFINED in uvc_parse_format

[Laurent Pinchart]

Hi Benoît,

Thank you for the patch.

On Thu, Nov 07, 2024 at 02:22:02PM +0000, Benoit Sevens wrote:
> This can lead to out of bounds writes since frames of this type were not
> taken into account when calculating the size of the frames buffer in
> uvc_parse_streaming.
> 
> Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> Signed-off-by: Benoit Sevens <bsevens@google.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 0fac689c6350..13db0026dc1a 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -371,7 +371,7 @@ static int uvc_parse_format(struct uvc_device *dev,
>  	 * Parse the frame descriptors. Only uncompressed, MJPEG and frame
>  	 * based formats have frame descriptors.
>  	 */
> -	while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> +	while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
>  	       buffer[2] == ftype) {
>  		unsigned int maxIntervalIndex;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/2] media: uvcvideo: Skip parsing frames of type UVC_VS_UNDEFINED in uvc_parse_format

[Laurent Pinchart]

On Thu, Nov 07, 2024 at 05:04:32PM +0200, Laurent Pinchart wrote:
> Hi Benoît,
> 
> Thank you for the patch.
> 
> On Thu, Nov 07, 2024 at 02:22:02PM +0000, Benoit Sevens wrote:
> > This can lead to out of bounds writes since frames of this type were not
> > taken into account when calculating the size of the frames buffer in
> > uvc_parse_streaming.
> > 
> > Fixes: c0efd232929c ("V4L/DVB (8145a): USB Video Class driver")
> > Signed-off-by: Benoit Sevens <bsevens@google.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Mauro, Hans, could you pick this as a fix for v6.12 ?

> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 0fac689c6350..13db0026dc1a 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -371,7 +371,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> >  	 * Parse the frame descriptors. Only uncompressed, MJPEG and frame
> >  	 * based formats have frame descriptors.
> >  	 */
> > -	while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> > +	while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> >  	       buffer[2] == ftype) {
> >  		unsigned int maxIntervalIndex;
> >  

-- 
Regards,

Laurent Pinchart

[PATCH v1 2/2] media: uvcvideo: Refactor frame parsing code into a uvc_parse_frame function

[Benoit Sevens]

The ftype value does not change in the while loop so we can check it
before entering the while loop. Refactoring the frame parsing code into
a dedicated uvc_parse_frame function makes this more readable.

Signed-off-by: Benoit Sevens <bsevens@google.com>
---
 drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++-------------
 1 file changed, 123 insertions(+), 105 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 13db0026dc1a..99f811ace5d6 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
  * Descriptors parsing
  */
 
+static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming,
+		struct uvc_format *format, struct uvc_frame *frame, u32 **intervals,
+		u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen)
+{
+	struct usb_interface *intf = streaming->intf;
+	struct usb_host_interface *alts = intf->cur_altsetting;
+	unsigned int i, n;
+	unsigned int interval;
+	unsigned int maxIntervalIndex;
+
+	if (ftype != UVC_VS_FRAME_FRAME_BASED)
+		n = buflen > 25 ? buffer[25] : 0;
+	else
+		n = buflen > 21 ? buffer[21] : 0;
+
+	n = n ? n : 3;
+
+	if (buflen < 26 + 4*n) {
+		uvc_dbg(dev, DESCR,
+			"device %d videostreaming interface %d FRAME error\n",
+			dev->udev->devnum,
+			alts->desc.bInterfaceNumber);
+		return -EINVAL;
+	}
+
+	frame->bFrameIndex = buffer[3];
+	frame->bmCapabilities = buffer[4];
+	frame->wWidth = get_unaligned_le16(&buffer[5])
+		      * width_multiplier;
+	frame->wHeight = get_unaligned_le16(&buffer[7]);
+	frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
+	frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
+	if (ftype != UVC_VS_FRAME_FRAME_BASED) {
+		frame->dwMaxVideoFrameBufferSize =
+			get_unaligned_le32(&buffer[17]);
+		frame->dwDefaultFrameInterval =
+			get_unaligned_le32(&buffer[21]);
+		frame->bFrameIntervalType = buffer[25];
+	} else {
+		frame->dwMaxVideoFrameBufferSize = 0;
+		frame->dwDefaultFrameInterval =
+			get_unaligned_le32(&buffer[17]);
+		frame->bFrameIntervalType = buffer[21];
+	}
+
+	/*
+	 * Copy the frame intervals.
+	 *
+	 * Some bogus devices report dwMinFrameInterval equal to
+	 * dwMaxFrameInterval and have dwFrameIntervalStep set to
+	 * zero. Setting all null intervals to 1 fixes the problem and
+	 * some other divisions by zero that could happen.
+	 */
+	frame->dwFrameInterval = *intervals;
+
+	for (i = 0; i < n; ++i) {
+		interval = get_unaligned_le32(&buffer[26+4*i]);
+		(*intervals)[i] = interval ? interval : 1;
+	}
+
+	/*
+	 * Apply more fixes, quirks and workarounds to handle incorrect
+	 * or broken descriptors.
+	 */
+
+	/*
+	 * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
+	 * completely. Observed behaviours range from setting the
+	 * value to 1.1x the actual frame size to hardwiring the
+	 * 16 low bits to 0. This results in a higher than necessary
+	 * memory usage as well as a wrong image size information. For
+	 * uncompressed formats this can be fixed by computing the
+	 * value from the frame size.
+	 */
+	if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
+		frame->dwMaxVideoFrameBufferSize = format->bpp
+			* frame->wWidth * frame->wHeight / 8;
+
+	/*
+	 * Clamp the default frame interval to the boundaries. A zero
+	 * bFrameIntervalType value indicates a continuous frame
+	 * interval range, with dwFrameInterval[0] storing the minimum
+	 * value and dwFrameInterval[1] storing the maximum value.
+	 */
+	maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
+	frame->dwDefaultFrameInterval =
+		clamp(frame->dwDefaultFrameInterval,
+		      frame->dwFrameInterval[0],
+		      frame->dwFrameInterval[maxIntervalIndex]);
+
+	/*
+	 * Some devices report frame intervals that are not functional.
+	 * If the corresponding quirk is set, restrict operation to the
+	 * first interval only.
+	 */
+	if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
+		frame->bFrameIntervalType = 1;
+		(*intervals)[0] = frame->dwDefaultFrameInterval;
+	}
+
+	uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
+		frame->wWidth, frame->wHeight,
+		10000000 / frame->dwDefaultFrameInterval,
+		(100000000 / frame->dwDefaultFrameInterval) % 10);
+
+	*intervals += n;
+
+	return buffer[0];
+}
+
+
 static int uvc_parse_format(struct uvc_device *dev,
 	struct uvc_streaming *streaming, struct uvc_format *format,
 	struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer,
@@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev,
 	struct uvc_frame *frame;
 	const unsigned char *start = buffer;
 	unsigned int width_multiplier = 1;
-	unsigned int interval;
 	unsigned int i, n;
 	u8 ftype;
+	int ret;
 
 	format->type = buffer[2];
 	format->index = buffer[3];
@@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev,
 	 * Parse the frame descriptors. Only uncompressed, MJPEG and frame
 	 * based formats have frame descriptors.
 	 */
-	while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
-	       buffer[2] == ftype) {
-		unsigned int maxIntervalIndex;
-
-		frame = &frames[format->nframes];
-		if (ftype != UVC_VS_FRAME_FRAME_BASED)
-			n = buflen > 25 ? buffer[25] : 0;
-		else
-			n = buflen > 21 ? buffer[21] : 0;
-
-		n = n ? n : 3;
-
-		if (buflen < 26 + 4*n) {
-			uvc_dbg(dev, DESCR,
-				"device %d videostreaming interface %d FRAME error\n",
-				dev->udev->devnum,
-				alts->desc.bInterfaceNumber);
-			return -EINVAL;
-		}
-
-		frame->bFrameIndex = buffer[3];
-		frame->bmCapabilities = buffer[4];
-		frame->wWidth = get_unaligned_le16(&buffer[5])
-			      * width_multiplier;
-		frame->wHeight = get_unaligned_le16(&buffer[7]);
-		frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
-		frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
-		if (ftype != UVC_VS_FRAME_FRAME_BASED) {
-			frame->dwMaxVideoFrameBufferSize =
-				get_unaligned_le32(&buffer[17]);
-			frame->dwDefaultFrameInterval =
-				get_unaligned_le32(&buffer[21]);
-			frame->bFrameIntervalType = buffer[25];
-		} else {
-			frame->dwMaxVideoFrameBufferSize = 0;
-			frame->dwDefaultFrameInterval =
-				get_unaligned_le32(&buffer[17]);
-			frame->bFrameIntervalType = buffer[21];
-		}
-
-		/*
-		 * Copy the frame intervals.
-		 *
-		 * Some bogus devices report dwMinFrameInterval equal to
-		 * dwMaxFrameInterval and have dwFrameIntervalStep set to
-		 * zero. Setting all null intervals to 1 fixes the problem and
-		 * some other divisions by zero that could happen.
-		 */
-		frame->dwFrameInterval = *intervals;
-
-		for (i = 0; i < n; ++i) {
-			interval = get_unaligned_le32(&buffer[26+4*i]);
-			(*intervals)[i] = interval ? interval : 1;
-		}
-
-		/*
-		 * Apply more fixes, quirks and workarounds to handle incorrect
-		 * or broken descriptors.
-		 */
-
-		/*
-		 * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
-		 * completely. Observed behaviours range from setting the
-		 * value to 1.1x the actual frame size to hardwiring the
-		 * 16 low bits to 0. This results in a higher than necessary
-		 * memory usage as well as a wrong image size information. For
-		 * uncompressed formats this can be fixed by computing the
-		 * value from the frame size.
-		 */
-		if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
-			frame->dwMaxVideoFrameBufferSize = format->bpp
-				* frame->wWidth * frame->wHeight / 8;
-
-		/*
-		 * Clamp the default frame interval to the boundaries. A zero
-		 * bFrameIntervalType value indicates a continuous frame
-		 * interval range, with dwFrameInterval[0] storing the minimum
-		 * value and dwFrameInterval[1] storing the maximum value.
-		 */
-		maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
-		frame->dwDefaultFrameInterval =
-			clamp(frame->dwDefaultFrameInterval,
-			      frame->dwFrameInterval[0],
-			      frame->dwFrameInterval[maxIntervalIndex]);
-
-		/*
-		 * Some devices report frame intervals that are not functional.
-		 * If the corresponding quirk is set, restrict operation to the
-		 * first interval only.
-		 */
-		if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
-			frame->bFrameIntervalType = 1;
-			(*intervals)[0] = frame->dwDefaultFrameInterval;
+	if (ftype) {
+		while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
+		       buffer[2] == ftype) {
+			frame = &frames[format->nframes];
+			ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype,
+					width_multiplier, buffer, buflen);
+			if (ret < 0)
+				return ret;
+			format->nframes++;
+			buflen -= ret;
+			buffer += ret;
 		}
-
-		uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
-			frame->wWidth, frame->wHeight,
-			10000000 / frame->dwDefaultFrameInterval,
-			(100000000 / frame->dwDefaultFrameInterval) % 10);
-
-		format->nframes++;
-		*intervals += n;
-
-		buflen -= buffer[0];
-		buffer += buffer[0];
 	}
 
 	if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
-- 
2.47.0.277.g8800431eea-goog

Re: [PATCH v1 2/2] media: uvcvideo: Refactor frame parsing code into a uvc_parse_frame function

[Laurent Pinchart]

Hi Benoît,

Thank you for the patch.

On Thu, Nov 07, 2024 at 02:22:03PM +0000, Benoit Sevens wrote:
> The ftype value does not change in the while loop so we can check it
> before entering the while loop. Refactoring the frame parsing code into
> a dedicated uvc_parse_frame function makes this more readable.
> 
> Signed-off-by: Benoit Sevens <bsevens@google.com>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++-------------
>  1 file changed, 123 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 13db0026dc1a..99f811ace5d6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
>   * Descriptors parsing
>   */
>  
> +static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming,
> +		struct uvc_format *format, struct uvc_frame *frame, u32 **intervals,
> +		u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen)
> +{
> +	struct usb_interface *intf = streaming->intf;
> +	struct usb_host_interface *alts = intf->cur_altsetting;

The intf variable is only used here, so you can write

	struct usb_host_interface *alts = streaming->intf->cur_altsetting;

> +	unsigned int i, n;
> +	unsigned int interval;
> +	unsigned int maxIntervalIndex;

We usually sort variables in (more or less) decreasing line length order
if nothing makes that inpractical.

> +
> +	if (ftype != UVC_VS_FRAME_FRAME_BASED)
> +		n = buflen > 25 ? buffer[25] : 0;
> +	else
> +		n = buflen > 21 ? buffer[21] : 0;
> +
> +	n = n ? n : 3;
> +
> +	if (buflen < 26 + 4*n) {
> +		uvc_dbg(dev, DESCR,
> +			"device %d videostreaming interface %d FRAME error\n",
> +			dev->udev->devnum,
> +			alts->desc.bInterfaceNumber);

We can now reflow the code as the indentation level has decreased, for instance

			dev->udev->devnum, alts->desc.bInterfaceNumber);

> +		return -EINVAL;
> +	}
> +
> +	frame->bFrameIndex = buffer[3];
> +	frame->bmCapabilities = buffer[4];
> +	frame->wWidth = get_unaligned_le16(&buffer[5])
> +		      * width_multiplier;
> +	frame->wHeight = get_unaligned_le16(&buffer[7]);
> +	frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> +	frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> +	if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> +		frame->dwMaxVideoFrameBufferSize =
> +			get_unaligned_le32(&buffer[17]);
> +		frame->dwDefaultFrameInterval =
> +			get_unaligned_le32(&buffer[21]);
> +		frame->bFrameIntervalType = buffer[25];
> +	} else {
> +		frame->dwMaxVideoFrameBufferSize = 0;
> +		frame->dwDefaultFrameInterval =
> +			get_unaligned_le32(&buffer[17]);
> +		frame->bFrameIntervalType = buffer[21];
> +	}
> +
> +	/*
> +	 * Copy the frame intervals.
> +	 *
> +	 * Some bogus devices report dwMinFrameInterval equal to
> +	 * dwMaxFrameInterval and have dwFrameIntervalStep set to
> +	 * zero. Setting all null intervals to 1 fixes the problem and
> +	 * some other divisions by zero that could happen.
> +	 */
> +	frame->dwFrameInterval = *intervals;
> +
> +	for (i = 0; i < n; ++i) {
> +		interval = get_unaligned_le32(&buffer[26+4*i]);
> +		(*intervals)[i] = interval ? interval : 1;
> +	}
> +
> +	/*
> +	 * Apply more fixes, quirks and workarounds to handle incorrect
> +	 * or broken descriptors.
> +	 */
> +
> +	/*
> +	 * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> +	 * completely. Observed behaviours range from setting the
> +	 * value to 1.1x the actual frame size to hardwiring the
> +	 * 16 low bits to 0. This results in a higher than necessary
> +	 * memory usage as well as a wrong image size information. For
> +	 * uncompressed formats this can be fixed by computing the
> +	 * value from the frame size.
> +	 */
> +	if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> +		frame->dwMaxVideoFrameBufferSize = format->bpp
> +			* frame->wWidth * frame->wHeight / 8;
> +
> +	/*
> +	 * Clamp the default frame interval to the boundaries. A zero
> +	 * bFrameIntervalType value indicates a continuous frame
> +	 * interval range, with dwFrameInterval[0] storing the minimum
> +	 * value and dwFrameInterval[1] storing the maximum value.
> +	 */
> +	maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> +	frame->dwDefaultFrameInterval =
> +		clamp(frame->dwDefaultFrameInterval,
> +		      frame->dwFrameInterval[0],
> +		      frame->dwFrameInterval[maxIntervalIndex]);
> +
> +	/*
> +	 * Some devices report frame intervals that are not functional.
> +	 * If the corresponding quirk is set, restrict operation to the
> +	 * first interval only.
> +	 */
> +	if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> +		frame->bFrameIntervalType = 1;
> +		(*intervals)[0] = frame->dwDefaultFrameInterval;
> +	}
> +
> +	uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> +		frame->wWidth, frame->wHeight,
> +		10000000 / frame->dwDefaultFrameInterval,
> +		(100000000 / frame->dwDefaultFrameInterval) % 10);
> +
> +	*intervals += n;
> +
> +	return buffer[0];
> +}
> +
> +
>  static int uvc_parse_format(struct uvc_device *dev,
>  	struct uvc_streaming *streaming, struct uvc_format *format,
>  	struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer,
> @@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev,

While at it, we can also merge the intf and alts variables here too.

With this addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can make all those small changes locally when applying the patch,
unless you prefer submitting a new version yourself.

>  	struct uvc_frame *frame;
>  	const unsigned char *start = buffer;
>  	unsigned int width_multiplier = 1;
> -	unsigned int interval;
>  	unsigned int i, n;
>  	u8 ftype;
> +	int ret;
>  
>  	format->type = buffer[2];
>  	format->index = buffer[3];
> @@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev,
>  	 * Parse the frame descriptors. Only uncompressed, MJPEG and frame
>  	 * based formats have frame descriptors.
>  	 */
> -	while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> -	       buffer[2] == ftype) {
> -		unsigned int maxIntervalIndex;
> -
> -		frame = &frames[format->nframes];
> -		if (ftype != UVC_VS_FRAME_FRAME_BASED)
> -			n = buflen > 25 ? buffer[25] : 0;
> -		else
> -			n = buflen > 21 ? buffer[21] : 0;
> -
> -		n = n ? n : 3;
> -
> -		if (buflen < 26 + 4*n) {
> -			uvc_dbg(dev, DESCR,
> -				"device %d videostreaming interface %d FRAME error\n",
> -				dev->udev->devnum,
> -				alts->desc.bInterfaceNumber);
> -			return -EINVAL;
> -		}
> -
> -		frame->bFrameIndex = buffer[3];
> -		frame->bmCapabilities = buffer[4];
> -		frame->wWidth = get_unaligned_le16(&buffer[5])
> -			      * width_multiplier;
> -		frame->wHeight = get_unaligned_le16(&buffer[7]);
> -		frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> -		frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> -		if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> -			frame->dwMaxVideoFrameBufferSize =
> -				get_unaligned_le32(&buffer[17]);
> -			frame->dwDefaultFrameInterval =
> -				get_unaligned_le32(&buffer[21]);
> -			frame->bFrameIntervalType = buffer[25];
> -		} else {
> -			frame->dwMaxVideoFrameBufferSize = 0;
> -			frame->dwDefaultFrameInterval =
> -				get_unaligned_le32(&buffer[17]);
> -			frame->bFrameIntervalType = buffer[21];
> -		}
> -
> -		/*
> -		 * Copy the frame intervals.
> -		 *
> -		 * Some bogus devices report dwMinFrameInterval equal to
> -		 * dwMaxFrameInterval and have dwFrameIntervalStep set to
> -		 * zero. Setting all null intervals to 1 fixes the problem and
> -		 * some other divisions by zero that could happen.
> -		 */
> -		frame->dwFrameInterval = *intervals;
> -
> -		for (i = 0; i < n; ++i) {
> -			interval = get_unaligned_le32(&buffer[26+4*i]);
> -			(*intervals)[i] = interval ? interval : 1;
> -		}
> -
> -		/*
> -		 * Apply more fixes, quirks and workarounds to handle incorrect
> -		 * or broken descriptors.
> -		 */
> -
> -		/*
> -		 * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> -		 * completely. Observed behaviours range from setting the
> -		 * value to 1.1x the actual frame size to hardwiring the
> -		 * 16 low bits to 0. This results in a higher than necessary
> -		 * memory usage as well as a wrong image size information. For
> -		 * uncompressed formats this can be fixed by computing the
> -		 * value from the frame size.
> -		 */
> -		if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> -			frame->dwMaxVideoFrameBufferSize = format->bpp
> -				* frame->wWidth * frame->wHeight / 8;
> -
> -		/*
> -		 * Clamp the default frame interval to the boundaries. A zero
> -		 * bFrameIntervalType value indicates a continuous frame
> -		 * interval range, with dwFrameInterval[0] storing the minimum
> -		 * value and dwFrameInterval[1] storing the maximum value.
> -		 */
> -		maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> -		frame->dwDefaultFrameInterval =
> -			clamp(frame->dwDefaultFrameInterval,
> -			      frame->dwFrameInterval[0],
> -			      frame->dwFrameInterval[maxIntervalIndex]);
> -
> -		/*
> -		 * Some devices report frame intervals that are not functional.
> -		 * If the corresponding quirk is set, restrict operation to the
> -		 * first interval only.
> -		 */
> -		if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> -			frame->bFrameIntervalType = 1;
> -			(*intervals)[0] = frame->dwDefaultFrameInterval;
> +	if (ftype) {
> +		while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> +		       buffer[2] == ftype) {
> +			frame = &frames[format->nframes];
> +			ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype,
> +					width_multiplier, buffer, buflen);
> +			if (ret < 0)
> +				return ret;
> +			format->nframes++;
> +			buflen -= ret;
> +			buffer += ret;
>  		}
> -
> -		uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> -			frame->wWidth, frame->wHeight,
> -			10000000 / frame->dwDefaultFrameInterval,
> -			(100000000 / frame->dwDefaultFrameInterval) % 10);
> -
> -		format->nframes++;
> -		*intervals += n;
> -
> -		buflen -= buffer[0];
> -		buffer += buffer[0];
>  	}
>  
>  	if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/2] media: uvcvideo: Refactor frame parsing code into a uvc_parse_frame function

[Benoît Sevens]

Thank you for the review, Laurent.

On Thu, 7 Nov 2024 at 17:46, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Benoît,
>
> Thank you for the patch.
>
> On Thu, Nov 07, 2024 at 02:22:03PM +0000, Benoit Sevens wrote:
> > The ftype value does not change in the while loop so we can check it
> > before entering the while loop. Refactoring the frame parsing code into
> > a dedicated uvc_parse_frame function makes this more readable.
> >
> > Signed-off-by: Benoit Sevens <bsevens@google.com>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++-------------
> >  1 file changed, 123 insertions(+), 105 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 13db0026dc1a..99f811ace5d6 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
> >   * Descriptors parsing
> >   */
> >
> > +static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming,
> > +             struct uvc_format *format, struct uvc_frame *frame, u32 **intervals,
> > +             u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen)
> > +{
> > +     struct usb_interface *intf = streaming->intf;
> > +     struct usb_host_interface *alts = intf->cur_altsetting;
>
> The intf variable is only used here, so you can write
>
>         struct usb_host_interface *alts = streaming->intf->cur_altsetting;
>
> > +     unsigned int i, n;
> > +     unsigned int interval;
> > +     unsigned int maxIntervalIndex;
>
> We usually sort variables in (more or less) decreasing line length order
> if nothing makes that inpractical.
>
> > +
> > +     if (ftype != UVC_VS_FRAME_FRAME_BASED)
> > +             n = buflen > 25 ? buffer[25] : 0;
> > +     else
> > +             n = buflen > 21 ? buffer[21] : 0;
> > +
> > +     n = n ? n : 3;
> > +
> > +     if (buflen < 26 + 4*n) {
> > +             uvc_dbg(dev, DESCR,
> > +                     "device %d videostreaming interface %d FRAME error\n",
> > +                     dev->udev->devnum,
> > +                     alts->desc.bInterfaceNumber);
>
> We can now reflow the code as the indentation level has decreased, for instance
>
>                         dev->udev->devnum, alts->desc.bInterfaceNumber);
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     frame->bFrameIndex = buffer[3];
> > +     frame->bmCapabilities = buffer[4];
> > +     frame->wWidth = get_unaligned_le16(&buffer[5])
> > +                   * width_multiplier;
> > +     frame->wHeight = get_unaligned_le16(&buffer[7]);
> > +     frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> > +     frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> > +     if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> > +             frame->dwMaxVideoFrameBufferSize =
> > +                     get_unaligned_le32(&buffer[17]);
> > +             frame->dwDefaultFrameInterval =
> > +                     get_unaligned_le32(&buffer[21]);
> > +             frame->bFrameIntervalType = buffer[25];
> > +     } else {
> > +             frame->dwMaxVideoFrameBufferSize = 0;
> > +             frame->dwDefaultFrameInterval =
> > +                     get_unaligned_le32(&buffer[17]);
> > +             frame->bFrameIntervalType = buffer[21];
> > +     }
> > +
> > +     /*
> > +      * Copy the frame intervals.
> > +      *
> > +      * Some bogus devices report dwMinFrameInterval equal to
> > +      * dwMaxFrameInterval and have dwFrameIntervalStep set to
> > +      * zero. Setting all null intervals to 1 fixes the problem and
> > +      * some other divisions by zero that could happen.
> > +      */
> > +     frame->dwFrameInterval = *intervals;
> > +
> > +     for (i = 0; i < n; ++i) {
> > +             interval = get_unaligned_le32(&buffer[26+4*i]);
> > +             (*intervals)[i] = interval ? interval : 1;
> > +     }
> > +
> > +     /*
> > +      * Apply more fixes, quirks and workarounds to handle incorrect
> > +      * or broken descriptors.
> > +      */
> > +
> > +     /*
> > +      * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> > +      * completely. Observed behaviours range from setting the
> > +      * value to 1.1x the actual frame size to hardwiring the
> > +      * 16 low bits to 0. This results in a higher than necessary
> > +      * memory usage as well as a wrong image size information. For
> > +      * uncompressed formats this can be fixed by computing the
> > +      * value from the frame size.
> > +      */
> > +     if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> > +             frame->dwMaxVideoFrameBufferSize = format->bpp
> > +                     * frame->wWidth * frame->wHeight / 8;
> > +
> > +     /*
> > +      * Clamp the default frame interval to the boundaries. A zero
> > +      * bFrameIntervalType value indicates a continuous frame
> > +      * interval range, with dwFrameInterval[0] storing the minimum
> > +      * value and dwFrameInterval[1] storing the maximum value.
> > +      */
> > +     maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> > +     frame->dwDefaultFrameInterval =
> > +             clamp(frame->dwDefaultFrameInterval,
> > +                   frame->dwFrameInterval[0],
> > +                   frame->dwFrameInterval[maxIntervalIndex]);
> > +
> > +     /*
> > +      * Some devices report frame intervals that are not functional.
> > +      * If the corresponding quirk is set, restrict operation to the
> > +      * first interval only.
> > +      */
> > +     if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> > +             frame->bFrameIntervalType = 1;
> > +             (*intervals)[0] = frame->dwDefaultFrameInterval;
> > +     }
> > +
> > +     uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> > +             frame->wWidth, frame->wHeight,
> > +             10000000 / frame->dwDefaultFrameInterval,
> > +             (100000000 / frame->dwDefaultFrameInterval) % 10);
> > +
> > +     *intervals += n;
> > +
> > +     return buffer[0];
> > +}
> > +
> > +
> >  static int uvc_parse_format(struct uvc_device *dev,
> >       struct uvc_streaming *streaming, struct uvc_format *format,
> >       struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer,
> > @@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev,
>
> While at it, we can also merge the intf and alts variables here too.
>
> With this addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I can make all those small changes locally when applying the patch,
> unless you prefer submitting a new version yourself.

If you can do those changes on your side, that would be great, thanks!

>
> >       struct uvc_frame *frame;
> >       const unsigned char *start = buffer;
> >       unsigned int width_multiplier = 1;
> > -     unsigned int interval;
> >       unsigned int i, n;
> >       u8 ftype;
> > +     int ret;
> >
> >       format->type = buffer[2];
> >       format->index = buffer[3];
> > @@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev,
> >        * Parse the frame descriptors. Only uncompressed, MJPEG and frame
> >        * based formats have frame descriptors.
> >        */
> > -     while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> > -            buffer[2] == ftype) {
> > -             unsigned int maxIntervalIndex;
> > -
> > -             frame = &frames[format->nframes];
> > -             if (ftype != UVC_VS_FRAME_FRAME_BASED)
> > -                     n = buflen > 25 ? buffer[25] : 0;
> > -             else
> > -                     n = buflen > 21 ? buffer[21] : 0;
> > -
> > -             n = n ? n : 3;
> > -
> > -             if (buflen < 26 + 4*n) {
> > -                     uvc_dbg(dev, DESCR,
> > -                             "device %d videostreaming interface %d FRAME error\n",
> > -                             dev->udev->devnum,
> > -                             alts->desc.bInterfaceNumber);
> > -                     return -EINVAL;
> > -             }
> > -
> > -             frame->bFrameIndex = buffer[3];
> > -             frame->bmCapabilities = buffer[4];
> > -             frame->wWidth = get_unaligned_le16(&buffer[5])
> > -                           * width_multiplier;
> > -             frame->wHeight = get_unaligned_le16(&buffer[7]);
> > -             frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> > -             frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> > -             if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> > -                     frame->dwMaxVideoFrameBufferSize =
> > -                             get_unaligned_le32(&buffer[17]);
> > -                     frame->dwDefaultFrameInterval =
> > -                             get_unaligned_le32(&buffer[21]);
> > -                     frame->bFrameIntervalType = buffer[25];
> > -             } else {
> > -                     frame->dwMaxVideoFrameBufferSize = 0;
> > -                     frame->dwDefaultFrameInterval =
> > -                             get_unaligned_le32(&buffer[17]);
> > -                     frame->bFrameIntervalType = buffer[21];
> > -             }
> > -
> > -             /*
> > -              * Copy the frame intervals.
> > -              *
> > -              * Some bogus devices report dwMinFrameInterval equal to
> > -              * dwMaxFrameInterval and have dwFrameIntervalStep set to
> > -              * zero. Setting all null intervals to 1 fixes the problem and
> > -              * some other divisions by zero that could happen.
> > -              */
> > -             frame->dwFrameInterval = *intervals;
> > -
> > -             for (i = 0; i < n; ++i) {
> > -                     interval = get_unaligned_le32(&buffer[26+4*i]);
> > -                     (*intervals)[i] = interval ? interval : 1;
> > -             }
> > -
> > -             /*
> > -              * Apply more fixes, quirks and workarounds to handle incorrect
> > -              * or broken descriptors.
> > -              */
> > -
> > -             /*
> > -              * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> > -              * completely. Observed behaviours range from setting the
> > -              * value to 1.1x the actual frame size to hardwiring the
> > -              * 16 low bits to 0. This results in a higher than necessary
> > -              * memory usage as well as a wrong image size information. For
> > -              * uncompressed formats this can be fixed by computing the
> > -              * value from the frame size.
> > -              */
> > -             if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> > -                     frame->dwMaxVideoFrameBufferSize = format->bpp
> > -                             * frame->wWidth * frame->wHeight / 8;
> > -
> > -             /*
> > -              * Clamp the default frame interval to the boundaries. A zero
> > -              * bFrameIntervalType value indicates a continuous frame
> > -              * interval range, with dwFrameInterval[0] storing the minimum
> > -              * value and dwFrameInterval[1] storing the maximum value.
> > -              */
> > -             maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> > -             frame->dwDefaultFrameInterval =
> > -                     clamp(frame->dwDefaultFrameInterval,
> > -                           frame->dwFrameInterval[0],
> > -                           frame->dwFrameInterval[maxIntervalIndex]);
> > -
> > -             /*
> > -              * Some devices report frame intervals that are not functional.
> > -              * If the corresponding quirk is set, restrict operation to the
> > -              * first interval only.
> > -              */
> > -             if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> > -                     frame->bFrameIntervalType = 1;
> > -                     (*intervals)[0] = frame->dwDefaultFrameInterval;
> > +     if (ftype) {
> > +             while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> > +                    buffer[2] == ftype) {
> > +                     frame = &frames[format->nframes];
> > +                     ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype,
> > +                                     width_multiplier, buffer, buflen);
> > +                     if (ret < 0)
> > +                             return ret;
> > +                     format->nframes++;
> > +                     buflen -= ret;
> > +                     buffer += ret;
> >               }
> > -
> > -             uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> > -                     frame->wWidth, frame->wHeight,
> > -                     10000000 / frame->dwDefaultFrameInterval,
> > -                     (100000000 / frame->dwDefaultFrameInterval) % 10);
> > -
> > -             format->nframes++;
> > -             *intervals += n;
> > -
> > -             buflen -= buffer[0];
> > -             buffer += buffer[0];
> >       }
> >
> >       if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
>
> --
> Regards,
>
> Laurent Pinchart