linux-kernel - Re: [PATCH v4 03/24] media: videobuf2-v4l2: Warn on holding buffers without support

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, 27 Apr 2022 13:31:11 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>
Cc:     nicolas@...fresne.ca,
        Sebastian Fricke <sebastian.fricke@...labora.com>,
        linux-media@...r.kernel.org,
        Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 03/24] media: videobuf2-v4l2: Warn on holding buffers
 without support

Hi Nicolas, Sebastian,

On Tue, Apr 26, 2022 at 9:58 PM Nicolas Dufresne
<nicolas.dufresne@...labora.com> wrote:
>
> From: Sebastian Fricke <sebastian.fricke@...labora.com>
>
> Using V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF flag without specifying the
> subsystem flag VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF, results in
> silently ignoring it.
> Warn the user via a debug print when the flag is requested but ignored
> by the videobuf2 framework.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@...labora.com>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>

Thanks for the patch. Please see my comments inline.

> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6edf4508c636..812c8d1962e0 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -329,8 +329,13 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>                  */
>                 vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
>                 vbuf->field = b->field;
> -               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF))
> +               if (!(q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)) {
> +                       if (vbuf->flags & V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF)
> +                               dprintk(q, 1,
> +                                       "Request holding buffer (%d), unsupported on output queue\n",
> +                                       b->index);

I wonder if we shouldn't just fail such a QBUF operation. Otherwise
the application would get unexpected behavior from the kernel.
Although it might be too late to do it now if there are applications
that rely on this implicit ignore...

Best regards,
Tomasz