linux-kernel - Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 26 Mar 2014 18:41:58 +0100
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Anton Leontiev <bunder@...5.ru>
Cc:	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

Hi Anton,

Thank you for the patch.

On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> Set error bit for incomplete buffers when end of buffer is detected by
> FID toggling (for example when last transaction with EOF is lost).
> This prevents passing incomplete buffers to the userspace.

But this would also breaks buggy webcams that toggle the FID bit but don't set 
the EOF bit. Support for this was added before the driver got merged in the 
mainline kernel, and the SVN log is a bit terse I'm afraid:

V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.

I don't remember which webcam(s) exhibit this behaviour.

Your patch would also mark valid buffers as erroneous when the list EOF bit is 
in a packet of its own with no data.

As the uvcvideo driver already marks buffers smaller than the expected image 
size as erroneous, missing EOF packets that contain data should already result 
in buffers with the error bit set. Are you concerned about compressed formats 
only ? While this patch would correctly detect the missing EOF packet in that 
case, any other missing packet would still result in a corrupt image, so I'm 
not sure if this would be worth it.

> Signed-off-by: Anton Leontiev <bunder@...5.ru>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
> *stream, */
> 
>  /*
> + * Set error flag for incomplete buffer.
> + */
> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> stream,
> +	struct uvc_buffer *const buf)
> +{
> +	if (buf->length != buf->bytesused &&
> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> +		buf->error = 1;
> +}
> +
> +/*
>   * Completion handler for video URBs.
>   */
>  static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> urb *urb, struct uvc_streaming *stream, do {
>  			ret = uvc_video_decode_start(stream, buf, mem,
>  				urb->iso_frame_desc[i].actual_length);
> -			if (ret == -EAGAIN)
> +			if (ret == -EAGAIN) {
> +				uvc_buffer_check_bytesused(stream, buf);
>  				buf = uvc_queue_next_buffer(&stream->queue,
>  							    buf);
> +			}
>  		} while (ret == -EAGAIN);
> 
>  		if (ret < 0)
> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
> 
>  		if (buf->state == UVC_BUF_STATE_READY) {
> -			if (buf->length != buf->bytesused &&
> -			    !(stream->cur_format->flags &
> -			      UVC_FMT_FLAG_COMPRESSED))
> -				buf->error = 1;
> -
> +			uvc_buffer_check_bytesused(stream, buf);
>  			buf = uvc_queue_next_buffer(&stream->queue, buf);
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/