linux-kernel - Re: [PATCH 2/3] media: videodev2.h: clarify v4l2_pix_format_mplane.sizeimage docs when to set to zero

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: <acae9f51-f8ca-b5f6-9af0-f0acc04e911d@xs4all.nl>
Date:   Mon, 25 Jan 2021 10:31:12 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Helen Koike <helen.koike@...labora.com>,
        linux-media@...r.kernel.org
Cc:     mchehab@...nel.org, hans.verkuil@...co.com, kernel@...labora.com,
        linux-kernel@...r.kernel.org, tfiga@...omium.org
Subject: Re: [PATCH 2/3] media: videodev2.h: clarify
 v4l2_pix_format_mplane.sizeimage docs when to set to zero

On 14/01/2021 19:01, Helen Koike wrote:
> sizeimage field should be set to zero for unused planes, even when
> v4l2_pix_format_mplane.num_planes is smaller then the index of planes.

then -> than

> 
> Signed-off-by: Helen Koike <helen.koike@...labora.com>
> 
> ---
> 
> I caught this with v4l2-compliance, which throws an error if we dirty
> planes, even if invalid, so I would like to make it clear in the docs.

What is the error? And with which driver?

I wonder if this isn't a v4l2-compliance bug. And if we want this to be
zeroed, then it wouldn't it be better to do that in the V4L2 core rather
than bother drivers with this?

> ---
>  include/uapi/linux/videodev2.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 79dbde3bcf8d..d9b7c9177605 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2227,6 +2227,7 @@ struct v4l2_mpeg_vbi_fmt_ivtv {
>   * struct v4l2_plane_pix_format - additional, per-plane format definition
>   * @sizeimage:		maximum size in bytes required for data, for which
>   *			this plane will be used
> + *			Drivers should be set it zero for unused planes.

This sentence is a bit garbled.

You probably meant: Drivers must set this to zero for unused planes.

But it makes no sense to just zero this field. I would zero the whole struct
contents for the unused planes.

>   * @bytesperline:	distance in bytes between the leftmost pixels in two
>   *			adjacent lines
>   */
> 

The API doesn't mention whether unused plane formats should be zeroed or not,
but it does make sense that they are. I don't think that the userspace API
should be changed (esp. since there are apparently already drivers that do
not zero these unused plane formats), but it makes sense that the compliance
test does verify this, and that the V4L2 core would zero unused plane formats.

I never like it when undefined values are allowed in an API, so it makes sense
that this is done.

Regards,

	Hans