linux-kernel - Re: [PATCH v4 09/11] media: uvcvideo: Increase the size of UVC_METADATA_BUF

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]

Message-ID: <df9feb80-6a31-478c-005c-13c12c4bf05e@xs4all.nl>
Date:   Tue, 16 Mar 2021 10:45:11 +0100
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Ricardo Ribalda <ribalda@...omium.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 09/11] media: uvcvideo: Increase the size of
 UVC_METADATA_BUF_SIZE

Hi Ricardo, Laurent,

On 15/03/2021 18:36, Ricardo Ribalda wrote:
> Hans has discovered that in his test device, for the H264 format
> bytesused goes up to about 570, for YUYV it will actually go up
> to a bit over 5000 bytes, and for MJPG up to about 2706 bytes.
> 
> Credit-to: Hans Verkuil <hverkuil@...all.nl>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvcvideo.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 1f17e4253673..91fc00ff311b 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -528,7 +528,7 @@ struct uvc_stats_stream {
>  	unsigned int max_sof;		/* Maximum STC.SOF value */
>  };
>  
> -#define UVC_METADATA_BUF_SIZE 1024
> +#define UVC_METADATA_BUF_SIZE 10240
>  
>  /**
>   * struct uvc_copy_op: Context structure to schedule asynchronous memcpy
> 

I've been doing some tests here, and this is tricky.

I think the core bug is in uvc_video_decode_meta():

        if (meta_buf->length - meta_buf->bytesused <
            length + sizeof(meta->ns) + sizeof(meta->sof)) {
                meta_buf->error = 1;
                return;
        }

This checks for a buffer overflow and by setting meta_buf->error to 1 it causes
the whole buffer to be discarded. But according to the V4L2_META_FMT_UVC docs here

https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/pixfmt-meta-uvc.html

it should "drop headers when the buffer is full", which suggests to me that
the 'meta_buf->error = 1;' is wrong and should be removed.

Looking at the number of headers I receive for various frame sizes and frame rates
when choosing YUYV as the pixelformat I see that the frame rate is the main input
to that: I get (very roughly) one header for every 150 microseconds. So that's
roughly 667 headers of 22 bytes for a 10 fps capture. The frame size has some
effect, but it is fairly small. This means that for slow captures (i.e. 2 fps)
you need about 75000 bytes, let's round it up to 102400.

I did tests with 1920x1080 at 5 fps for YUYV, H264 and MJPEG and saw the following:

Format		Video Size	Metadata Size

YUYV		4147200		29964
MJPG		130000		3608
H264 (P-frame)	70000		2600
H264 (I-frame)	150000		5500

The difference here is most likely due to YUYV being transmitted over time as
video lines arrive, so it is spread out over time, while H264 and MJPG are
bursty, i.e. the whole compressed frame is transferred in one go.

I think that 10240 is a good value for the metadata buffers since it is enough
for the worst-case for the compressed formats, and that if this is combined
with removing the 'meta_buf->error = 1;' it will also do its job for YUYV
even though data will be dropped, but that's better than not getting anything
at all.

Regards,

	Hans