linux-kernel - Re: [PATCH 0/3] media: uvcvideo: Prepare deprecation of nodrop

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] [day] [month] [year] [list]

Message-ID: <84b0f212-cd88-46bb-8e6f-b94ec3eccba6@redhat.com>
Date: Wed, 18 Dec 2024 21:37:46 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Ricardo Ribalda <ribalda@...omium.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] media: uvcvideo: Prepare deprecation of nodrop

Hi Ricardo,

Thank you for working on this.

On 17-Dec-24 10:06 PM, Ricardo Ribalda wrote:
> We intend to deprecate the nodrop parameter in the future and adopt the
> default behaviour of the other media drivers: drop invalid packages.

Actually the default behaviour of other media drivers is:

"return buffers with an error to userspace with V4L2_BUF_FLAG_ERROR set
in v4l2_buffer.flags".

It is not "drop invalid packages". The commit messages of patch 1/3
has some related unclear wording, please fix this.

Looking at this I actually have found what arguably is a bug in
the UVC driver when nodrop is set, or at least something which
we must change before making nodrop=1 the default.

Currently uvc_queue_buffer_complete() looks like this:

static void uvc_queue_buffer_complete(struct kref *ref)
{
        struct uvc_buffer *buf = container_of(ref, struct uvc_buffer, ref);
        struct vb2_buffer *vb = &buf->buf.vb2_buf;
        struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);

        if ((queue->flags & UVC_QUEUE_DROP_CORRUPTED) && buf->error) {
                uvc_queue_buffer_requeue(queue, buf);
                return;
        }

        buf->state = buf->error ? UVC_BUF_STATE_ERROR : UVC_BUF_STATE_DONE;
        vb2_set_plane_payload(&buf->buf.vb2_buf, 0, buf->bytesused);
        vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
}

Notice how the last line does not propagate buf->error to the
videobuf2 code, so when nodrop=1 is set then buffers with errors
are not only returned to userspace, they are returned to userspace
without V4L2_BUF_FLAG_ERROR getting set in v4l2_buffer.flags .

The right thing to do in this case is to set V4L2_BUF_FLAG_ERROR
IOW the last line of uvc_queue_buffer_complete() should be changed to:

        vb2_buffer_done(&buf->buf.vb2_buf, buf->error ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);

And this should probably be the first patch in a v2 series for this.

Regards,

Hans