Thank you for your contribution! Sashiko AI review found 15 potential issue(s) to consider: - [Critical] media: virtio: session use-after-free in virtio_media_process_events() - [Critical] media: virtio: session use-after-free in virtio_media_remove() - [Critical] media: virtio: pending_dqbufs use-after-free on buffer reallocation - [Critical] media: virtio: out-of-bounds buffer array access when host increases count - [Critical] media: virtio: missing queues_lock synchronization on queue state and buffers - [Critical] media: virtio: compat heap out-of-bounds read on multiplanar buffer ioctls - [High] media: virtio: deadlock between vlock and events_lock in event processing - [High] media: virtio: duplicate session close after VIRTIO_MEDIA_EVT_ERROR - [High] media: virtio: pending_dqbufs list corruption on duplicate QBUF or DQBUF - [High] media: virtio: broken EPOLLOUT, EPOLLERR, and mplane handling in poll() - [High] media: virtio: missing streamoff wakeup and broken O_NONBLOCK in dqbuf() - [High] media: virtio: unlocked eventq setup and missing cleanup in probe() - [High] media: virtio: eventq_work cancelled before device reset in remove() - [Medium] media: virtio: memory leak in virtio_media_create_bufs() when count is 0 - [Medium] media: virtio: SG index mismatch when length is 0 and uninitialized resp_len --
commit ce9e8e91b26679cbd71baf6f6009e5e696c3100b Author: Alexandre Courbot <[email protected]> media: virtio: Add buffer streaming, event handling, and poll This commit adds V4L2 buffer queue state tracking, streaming ioctls, asynchronous event virtqueue processing, V4L2 event subscription, and poll support to the virtio-media driver. > diff --git a/drivers/media/virtio/virtio_media_driver.c > b/drivers/media/virtio/virtio_media_driver.c > index 2f2e95077b556..877ac154b3221 100644 > --- a/drivers/media/virtio/virtio_media_driver.c > +++ b/drivers/media/virtio/virtio_media_driver.c [ ... ] > @@ -152,6 +174,31 @@ static int virtio_media_session_close(struct > virtio_media *vv, > return 0; > } > > +/** > + * virtio_media_find_session() - Look up a session with a given ID. > + * @vv: virtio-media device to lookup the session from. > + * @id: ID of the session to lookup. > + */ > +static struct virtio_media_session * > +virtio_media_find_session(struct virtio_media *vv, u32 id) > +{ > + struct list_head *p; > + struct virtio_media_session *session = NULL; > + > + mutex_lock(&vv->sessions_lock); > + list_for_each(p, &vv->sessions) { > + struct virtio_media_session *s = > + list_entry(p, struct virtio_media_session, list); > + if (s->id == id) { > + session = s; > + break; > + } > + } > + mutex_unlock(&vv->sessions_lock); > + > + return session; > +} [Severity: Critical] Can this lead to a use-after-free on struct virtio_media_session in virtio_media_process_events()? virtio_media_find_session() drops vv->sessions_lock and returns an unreferenced session pointer. virtio_media_process_events() then uses that session pointer under vv->events_lock to process VIRTIO_MEDIA_EVT_ERROR, VIRTIO_MEDIA_EVT_DQBUF, and VIRTIO_MEDIA_EVT_EVENT. Concurrently, when the file descriptor is closed, virtio_media_device_close() does not acquire vv->events_lock: virtio_media_device_close() virtio_media_session_close(vv, session) virtio_media_session_remove(vv, session) virtio_media_session_free(session) If virtio_media_device_close() removes and frees the session after virtio_media_find_session() returns it, won't virtio_media_process_events() read and write freed session and queue->buffers memory? [ ... ] > +static void > +virtio_media_process_dqbuf_event(struct virtio_media *vv, > + struct virtio_media_session *session, > + struct virtio_media_event_dqbuf *dqbuf_evt) > +{ > + struct virtio_media_buffer *dqbuf; > + const enum v4l2_buf_type queue_type = dqbuf_evt->buffer.type; > + struct virtio_media_queue_state *queue; > + typeof(dqbuf->buffer.m) buffer_m; > + typeof(dqbuf->buffer.m.planes[0].m) plane_m; > + int i; > + > + if (queue_type >= ARRAY_SIZE(session->queues)) { > + v4l2_err(&vv->v4l2_dev, > + "unmanaged queue %d passed to dqbuf event", > + dqbuf_evt->buffer.type); > + return; > + } > + queue = &session->queues[queue_type]; > + > + if (dqbuf_evt->buffer.index >= queue->allocated_bufs) { > + v4l2_err(&vv->v4l2_dev, > + "invalid buffer ID %d for queue %d in dqbuf event", > + dqbuf_evt->buffer.index, dqbuf_evt->buffer.type); > + return; > + } > + > + dqbuf = &queue->buffers[dqbuf_evt->buffer.index]; [Severity: Critical] Is session->queues_lock needed before reading queue->allocated_bufs and accessing queue->buffers[] here, as well as in the ioctl handlers? virtio_media_process_dqbuf_event() runs on vv->eventq_work without holding vv->vlock. It reads queue->allocated_bufs, indexes queue->buffers[], and writes to dqbuf->buffer and dqbuf->planes before acquiring session->queues_lock. Meanwhile, virtio_media_reqbufs() and virtio_media_create_bufs() free and replace queue->buffers and update queue->allocated_bufs without holding session->queues_lock, and virtio_media_qbuf(), virtio_media_prepare_buf(), virtio_media_querybuf(), virtio_media_streamon(), and virtio_media_decoder_cmd() also modify queue state unlocked. Could virtio_media_process_dqbuf_event() dereference queue->buffers while virtio_media_reqbufs() or virtio_media_create_bufs() frees it via vfree(), or race on queue->queued_bufs with virtio_media_qbuf()? > + > + /* > + * Preserve the 'm' union that was passed to us during QBUF so userspace > + * gets back the information it submitted. > + */ > + buffer_m = dqbuf->buffer.m; > + memcpy(&dqbuf->buffer, &dqbuf_evt->buffer, sizeof(dqbuf->buffer)); > + dqbuf->buffer.m = buffer_m; > + if (V4L2_TYPE_IS_MULTIPLANAR(dqbuf->buffer.type)) { > + if (dqbuf->buffer.length > VIDEO_MAX_PLANES) { > + v4l2_err(&vv->v4l2_dev, > + "invalid number of planes received from host > for a multiplanar buffer\n"); > + return; > + } > + for (i = 0; i < dqbuf->buffer.length; i++) { > + plane_m = dqbuf->planes[i].m; > + memcpy(&dqbuf->planes[i], &dqbuf_evt->planes[i], > + sizeof(struct v4l2_plane)); > + dqbuf->planes[i].m = plane_m; > + } > + } > + > + /* Set the DONE flag as the buffer is waiting to be dequeued. */ > + dqbuf->buffer.flags |= V4L2_BUF_FLAG_DONE; > + > + mutex_lock(&session->queues_lock); > + list_add_tail(&dqbuf->list, &queue->pending_dqbufs); > + queue->queued_bufs -= 1; > + mutex_unlock(&session->queues_lock); [Severity: High] What happens if userspace calls VIDIOC_QBUF on a buffer that is already in queue->pending_dqbufs (with V4L2_BUF_FLAG_DONE set) before dequeuing it with VIDIOC_DQBUF, or if a duplicate VIRTIO_MEDIA_EVT_DQBUF arrives? In virtio_media_qbuf(), buffer->buffer.flags is overwritten with V4L2_BUF_FLAG_QUEUED without checking if the buffer is already queued or waiting in queue->pending_dqbufs. Since the host already considers the buffer dequeued after sending the first VIRTIO_MEDIA_EVT_DQBUF, it can accept the VIDIOC_QBUF command and emit another VIRTIO_MEDIA_EVT_DQBUF, causing virtio_media_process_dqbuf_event() to call list_add_tail(&dqbuf->list, &queue->pending_dqbufs) on an already-linked list_head and corrupt queue->pending_dqbufs. > + > + wake_up(&session->dqbuf_wait); > +} > + > +/** > + * virtio_media_process_events() - Process all pending events on a device. [ ... ] > + switch (evt->event) { > + case VIRTIO_MEDIA_EVT_ERROR: > + if (len < sizeof(*error_evt)) { > + v4l2_err(&vv->v4l2_dev, > + "error event is too short: got %u, > expected %zu\n", > + len, sizeof(*error_evt)); > + break; > + } > + error_evt = (struct virtio_media_event_error *)evt; > + v4l2_err(&vv->v4l2_dev, > + "received error %d for session %d", > + error_evt->error, error_evt->hdr.session_id); > + > + mutex_lock(&session->error_lock); > + session->error = true; > + mutex_unlock(&session->error_lock); > + > + virtio_media_session_close(vv, session); [Severity: High] Can calling virtio_media_session_close() here deadlock on vv->vlock and vv->events_lock? When userspace calls VIDIOC_SUBSCRIBE_EVENT with V4L2_EVENT_SUB_FL_SEND_INITIAL, the call chain is: virtio_media_device_ioctl() mutex_lock(&vv->vlock) video_ioctl2() ... virtio_media_subscribe_event() virtio_media_process_events() mutex_lock(&vv->events_lock) virtio_media_session_close() mutex_lock(&vv->vlock) If a VIRTIO_MEDIA_EVT_ERROR event is processed during virtio_media_subscribe_event(), this attempts to acquire vv->vlock while already holding vv->vlock on the same thread, and also inverts the lock order (vv->events_lock -> vv->vlock) relative to virtio_media_subscribe_event() (vv->vlock -> vv->events_lock) when virtio_media_event_work() runs on the workqueue. [Severity: High] Also, should VIRTIO_MEDIA_EVT_ERROR call virtio_media_session_close() if virtio_media_device_close() will also call it when the file is later closed? The comment on struct virtio_media_event_error in include/uapi/linux/virtio_media.h states: * Upon receiving this event, the session mentioned in the header is considered * corrupted and closed. Calling virtio_media_session_close() here and then again unconditionally in virtio_media_device_close() sends VIRTIO_MEDIA_CMD_CLOSE twice for the same session->id, which could close a newly opened session if the host reuses the session ID. [ ... ] > @@ -358,10 +633,65 @@ static int virtio_media_device_close(struct file *file) > return ret; > } > > +/** > + * virtio_media_device_poll() - Poll logic for a virtio-media device. > + * @file: file of the session to poll. > + * @wait: poll table to wait on. > + */ > +static __poll_t virtio_media_device_poll(struct file *file, poll_table *wait) > +{ > + struct virtio_media_session *session = > + fh_to_session(file->private_data); > + enum v4l2_buf_type capture_type = > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE : > + V4L2_BUF_TYPE_VIDEO_CAPTURE; > + enum v4l2_buf_type output_type = > + session->uses_mplane ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE : > + V4L2_BUF_TYPE_VIDEO_OUTPUT; > + struct virtio_media_queue_state *capture_queue = > + &session->queues[capture_type]; > + struct virtio_media_queue_state *output_queue = > + &session->queues[output_type]; > + __poll_t req_events = poll_requested_events(wait); > + __poll_t rc = 0; > + > + poll_wait(file, &session->dqbuf_wait, wait); > + poll_wait(file, &session->fh.wait, wait); > + > + mutex_lock(&session->error_lock); > + if (session->error) > + rc |= EPOLLERR; > + mutex_unlock(&session->error_lock); > + > + mutex_lock(&session->queues_lock); > + if (req_events & (EPOLLIN | EPOLLRDNORM)) { > + if (!capture_queue->streaming || > + (capture_queue->queued_bufs == 0 && > + list_empty(&capture_queue->pending_dqbufs))) > + rc |= EPOLLERR; > + else if (!list_empty(&capture_queue->pending_dqbufs)) > + rc |= EPOLLIN | EPOLLRDNORM; > + } > + if (req_events & (EPOLLOUT | EPOLLWRNORM)) { > + if (!output_queue->streaming) > + rc |= EPOLLERR; > + else if (output_queue->queued_bufs < > + output_queue->allocated_bufs) > + rc |= EPOLLOUT | EPOLLWRNORM; > + } > + mutex_unlock(&session->queues_lock); [Severity: High] Are the poll conditions for EPOLLOUT, EPOLLIN, and EPOLLERR here matching V4L2 semantics? 1. For EPOLLOUT | EPOLLWRNORM, checking output_queue->queued_bufs < output_queue->allocated_bufs causes poll() to return immediately whenever any allocated output buffer is not currently queued on the host, even when output_queue->pending_dqbufs is empty and VIDIOC_DQBUF would return -EAGAIN or block. Shouldn't this check !list_empty(&output_queue->pending_dqbufs) instead? 2. When the last capture buffer (with V4L2_BUF_FLAG_LAST) has been dequeued, capture_queue->is_capture_last is true, capture_queue->queued_bufs is 0, and capture_queue->pending_dqbufs is empty. Doesn't this branch return EPOLLERR instead of EPOLLIN | EPOLLRDNORM so userspace can call VIDIOC_DQBUF and receive -EPIPE? 3. If an application polls for both directions (for example select() or poll(POLLIN | POLLOUT) on an M2M device when only one queue is streaming), won't checking !capture_queue->streaming and !output_queue->streaming independently set EPOLLERR even though the other queue is validly streaming? 4. capture_type and output_type rely on session->uses_mplane, which is set in virtio_media_reqbufs() but is never set in virtio_media_create_bufs(). If multiplanar buffers are allocated via VIDIOC_CREATE_BUFS, won't virtio_media_device_poll() inspect the non-MPLANE queues and always return EPOLLERR? [ ... ] > @@ -449,12 +799,25 @@ static int virtio_media_probe(struct virtio_device > *virtio_dev) > if (ret) > goto err_del_vqs; > > + for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) { > + void *ebuf = vv->event_buffer + > + virtio_media_event_aligned_size * i; > + > + ret = virtio_media_send_event_buffer(vv, ebuf); > + if (ret) > + goto err_video_unregister_device; > + } > + > return 0; > > +err_video_unregister_device: > + video_unregister_device(&vv->video_dev); > err_del_vqs: > virtio_dev->config->del_vqs(virtio_dev); > err_v4l2_device_unregister: > v4l2_device_unregister(&vv->v4l2_dev); > +err_free_event_buffer: > + kfree(vv->event_buffer); > err_free_virtio_media: > kfree(vv); > return ret; [Severity: High] In virtio_media_probe(), virtio_media_send_event_buffer() is called in a loop after virtio_device_ready() and video_register_device() without holding vv->events_lock. If an event callback fires (scheduling vv->eventq_work) or userspace calls VIDIOC_SUBSCRIBE_EVENT while this loop is still running, can virtio_media_process_events() access vv->eventq concurrently with virtio_media_send_event_buffer()? Also, if virtio_media_send_event_buffer() fails partway through the loop and jumps to err_video_unregister_device, the error path deletes the virtqueues and frees vv->event_buffer and vv without calling virtio_reset_device() or cancel_work_sync(&vv->eventq_work). Can the host or a pending eventq_work access freed memory on this failure path? > } > > static void virtio_media_remove(struct virtio_device *virtio_dev) > @@ -468,6 +831,9 @@ static void virtio_media_remove(struct virtio_device > *virtio_dev) > /* Remove the video device to stop accessing the commandq virtqueue */ > video_unregister_device(&vv->video_dev); > > + /* Stop event handling work queue from accessing eventq virtqueue */ > + cancel_work_sync(&vv->eventq_work); > + > /* > * Acquire vlock to ensure no concurrent accesses to the commandq > * virtqueue > @@ -483,6 +849,8 @@ static void virtio_media_remove(struct virtio_device > *virtio_dev) > */ > while (virtqueue_detach_unused_buf(vv->commandq)) > ; > + while (virtqueue_detach_unused_buf(vv->eventq)) > + ; > > /* Delete the virtqueues */ > virtio_dev->config->del_vqs(virtio_dev); [Severity: High] In virtio_media_remove(), cancel_work_sync(&vv->eventq_work) is called before virtio_reset_device(virtio_dev): virtio_media_remove() video_unregister_device(&vv->video_dev) cancel_work_sync(&vv->eventq_work) mutex_lock(&vv->vlock) virtio_reset_device(virtio_dev) If an eventq interrupt arrives between cancel_work_sync() and virtio_reset_device(), won't eventq_callback() call schedule_work(&vv->eventq_work) again, allowing virtio_media_event_work() to run while the virtqueues and struct virtio_media are being torn down? [Severity: Critical] Additionally, right after unlocking vv->vlock in virtio_media_remove(), the driver frees all sessions in vv->sessions: list_for_each_safe(p, n, &vv->sessions) { struct virtio_media_session *s = list_entry(p, struct virtio_media_session, list); virtio_media_session_remove(vv, s); virtio_media_session_free(s); } Since virtio_media_dqbuf() drops vv->vlock while sleeping in wait_event_interruptible(session->dqbuf_wait, ...), and video_unregister_device() does not wait for open file descriptors or in-flight ioctls to complete, won't freeing the sessions here cause a use-after-free in any thread sleeping in virtio_media_dqbuf() as well as when open file descriptors are subsequently closed in virtio_media_device_close()? > diff --git a/drivers/media/virtio/virtio_media_ioctls.c > b/drivers/media/virtio/virtio_media_ioctls.c > index 51f46b334800c..9bf2834691fd0 100644 > --- a/drivers/media/virtio/virtio_media_ioctls.c > +++ b/drivers/media/virtio/virtio_media_ioctls.c [ ... ] > +static int virtio_media_send_buffer_ioctl(struct v4l2_fh *fh, u32 ioctl, > + struct v4l2_buffer *b) > +{ > + struct video_device *video_dev = fh->vdev; > + struct virtio_media *vv = to_virtio_media(video_dev); > + struct virtio_media_session *session = fh_to_session(fh); > + struct v4l2_plane *orig_planes = NULL; > + struct scatterlist *sgs[64]; > + /* > + * End of the device-readable buffer SGs, to reuse in device-writable > + * section. > + */ > + size_t num_cmd_sgs; > + size_t end_buf_sg; > + struct scatterlist_builder builder = { > + .descs = session->command_sgs.sgl, > + .num_descs = DESC_CHAIN_MAX_LEN, > + .cur_desc = 0, > + .shadow_buffer = session->shadow_buf, > + .shadow_buffer_size = VIRTIO_SHADOW_BUF_SIZE, > + .shadow_buffer_pos = 0, > + .sgs = sgs, > + .num_sgs = ARRAY_SIZE(sgs), > + .cur_sg = 0, > + .framevec = NULL, > + }; > + size_t resp_len; > + int ret; > + int i; > + > + if (b->type > VIRTIO_MEDIA_LAST_QUEUE) > + return -EINVAL; > + > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) > + orig_planes = b->m.planes; > + > + /* Command descriptor */ > + ret = scatterlist_builder_add_ioctl_cmd(&builder, session, ioctl); > + if (ret) > + return ret; > + > + /* Command payload (struct v4l2_buffer) */ > + ret = scatterlist_builder_add_buffer(&builder, b); > + if (ret < 0) > + return ret; > + > + end_buf_sg = builder.cur_sg; > + num_cmd_sgs = builder.cur_sg; > + > + /* Response descriptor */ > + ret = scatterlist_builder_add_ioctl_resp(&builder, session); > + if (ret) > + return ret; > + > + /* Response payload (same as input, but no userptr mapping) */ > + for (i = 1; i < end_buf_sg; i++) { > + ret = scatterlist_builder_add_descriptor(&builder, i); > + if (ret < 0) > + return ret; > + } > + > + ret = virtio_media_send_command(vv, builder.sgs, num_cmd_sgs, > + builder.cur_sg - num_cmd_sgs, > + sizeof(struct virtio_media_resp_ioctl) + > + sizeof(*b), &resp_len); > + if (ret < 0) > + return ret; > + > + resp_len -= sizeof(struct virtio_media_resp_ioctl); > + > + /* Make sure that the reply length covers our v4l2_buffer */ > + if (resp_len < sizeof(*b)) > + return -EINVAL; > + > + ret = scatterlist_builder_retrieve_buffer(&builder, num_cmd_sgs + 1, b, > + orig_planes); [Severity: Medium] What happens here when V4L2_TYPE_IS_MULTIPLANAR(b->type) is true, b->length is 0, and b->m.planes is non-NULL? In v4l2-ioctl.c:check_array_args(), b->m.planes is only replaced with a kernel buffer when b->length > 0. Likewise, scatterlist_builder_add_buffer() only adds an SG descriptor for b->m.planes when b->length > 0. However, virtio_media_send_buffer_ioctl() sets orig_planes = b->m.planes without checking b->length > 0, and scatterlist_builder_retrieve_buffer() calls scatterlist_builder_retrieve_data(builder, sg_index++, b->m.planes) whenever orig_planes is non-NULL, which will attempt to retrieve a non-existent SG entry into an unvalidated user pointer and fail with -ERANGE. Also, resp_len is uninitialized in virtio_media_send_buffer_ioctl() before being passed to virtio_media_send_command(), which reads *resp_len on entry: size_t local_resp_len = resp_len ? *resp_len : 0; [ ... ] > +static int virtio_media_reqbufs(struct file *file, void *fh, > + struct v4l2_requestbuffers *b) > +{ > + struct v4l2_fh *vfh = file_to_v4l2_fh(file); > + struct virtio_media_session *session = fh_to_session(vfh); > + struct virtio_media_buffer *buffers = NULL; > + struct virtio_media_queue_state *queue; > + int ret; > + > + if (b->type > VIRTIO_MEDIA_LAST_QUEUE) > + return -EINVAL; > + > + if (b->memory == V4L2_MEMORY_USERPTR) > + return -EINVAL; > + > + if (b->count > 0) { > + buffers = > + vcalloc(b->count, sizeof(struct virtio_media_buffer)); > + if (!buffers) > + return -ENOMEM; > + } > + > + ret = virtio_media_send_wr_ioctl(vfh, VIDIOC_REQBUFS, b, sizeof(*b), > + sizeof(*b)); > + if (ret) { > + vfree(buffers); > + return ret; > + } > + > + queue = &session->queues[b->type]; > + > + /* REQBUFS(0) is an implicit STREAMOFF. */ > + if (b->count == 0) > + virtio_media_clear_queue(session, queue); > + > + vfree(queue->buffers); > + queue->buffers = buffers; > + > + queue->allocated_bufs = b->count; [Severity: Critical] Can this overflow queue->buffers[] if the host increases b->count? In V4L2, a driver or host handling VIDIOC_REQBUFS or VIDIOC_CREATE_BUFS may round b->count upward to satisfy minimum buffer requirements. Here in virtio_media_reqbufs(), buffers is allocated using the pre-ioctl b->count, and then queue->allocated_bufs is set to the host-updated b->count. Similarly, in virtio_media_create_bufs(), buffers is allocated with queue->allocated_bufs + b->count elements before the ioctl, and queue->allocated_bufs is set to b->index + b->count after the ioctl without checking that it fits within the allocated array. If the host returns a larger count than requested, won't subsequent indexing of queue->buffers[] up to queue->allocated_bufs read and write out of bounds? [ ... ] > +static int virtio_media_create_bufs(struct file *file, void *fh, > + struct v4l2_create_buffers *b) > +{ > + struct v4l2_fh *vfh = file_to_v4l2_fh(file); > + struct virtio_media_session *session = fh_to_session(vfh); > + struct virtio_media_queue_state *queue; > + struct virtio_media_buffer *buffers = NULL; > + u32 type = b->format.type; > + int ret; > + > + if (type > VIRTIO_MEDIA_LAST_QUEUE) > + return -EINVAL; > + > + queue = &session->queues[type]; > + > + if (b->count > 0) { > + /* > + * This allocates a new buffer large enough for the existing > + * buffers (``queue->allocated_bufs``) as well as the newly > + * requested buffers (``b->count``). The host may allocate > + * fewer buffers than requested, as indicated by the final > + * value of ``b->count``. In this case, we do not re-allocate > + * to avoid the case where this new allocation fails and the > + * host buffers need to be freed. Instead we simply use the > + * larger buffer. > + */ > + buffers = vcalloc(queue->allocated_bufs + b->count, > + sizeof(*queue->buffers)); > + if (!buffers) > + return -ENOMEM; > + } > + > + ret = virtio_media_send_wr_ioctl(vfh, VIDIOC_CREATE_BUFS, b, sizeof(*b), > + sizeof(*b)); > + if (ret) { > + if (buffers) > + vfree(buffers); > + > + return ret; > + } > + > + /* If count is zero, we were just checking for format. */ > + if (b->count == 0) > + return 0; [Severity: Medium] Does this leak buffers if b->count was non-zero on input and the host overwrites b->count with 0 in the VIDIOC_CREATE_BUFS response? > + > + memcpy(buffers, queue->buffers, > + sizeof(*queue->buffers) * queue->allocated_bufs); > + vfree(queue->buffers); > + queue->buffers = buffers; [Severity: Critical] Can reallocating queue->buffers here (or in virtio_media_reqbufs() when b->count > 0) corrupt queue->pending_dqbufs and cause a use-after-free? Each struct virtio_media_buffer in queue->buffers contains an embedded struct list_head list node that is linked into queue->pending_dqbufs when a buffer is ready to be dequeued. In virtio_media_create_bufs(), memcpy() shallow-copies queue->buffers into the newly allocated buffers array and then calls vfree(queue->buffers), leaving queue->pending_dqbufs and any linked list_head pointers pointing into the freed queue->buffers array. Similarly, in virtio_media_reqbufs(), virtio_media_clear_queue() is only called when b->count == 0; if VIDIOC_REQBUFS is called with b->count > 0 while queue->pending_dqbufs is non-empty, vfree(queue->buffers) frees the active list nodes while they remain linked in queue->pending_dqbufs. [ ... ] > +static int virtio_media_dqbuf(struct file *file, void *fh, > + struct v4l2_buffer *b) > +{ > + struct video_device *video_dev = video_devdata(file); > + struct virtio_media *vv = to_virtio_media(video_dev); > + struct virtio_media_session *session = > + fh_to_session(file_to_v4l2_fh(file)); > + struct virtio_media_buffer *dqbuf; > + struct virtio_media_queue_state *queue; > + struct list_head *buffer_queue; > + struct v4l2_plane *planes_backup = NULL; > + const bool is_multiplanar = V4L2_TYPE_IS_MULTIPLANAR(b->type); > + int ret; > + bool error; > + > + if (b->type > VIRTIO_MEDIA_LAST_QUEUE) > + return -EINVAL; > + > + queue = &session->queues[b->type]; > + > + /* > + * If a buffer with the LAST flag has been returned, subsequent > + * calls to DQBUF must return -EPIPE until the queue is cleared. > + */ > + if (queue->is_capture_last) > + return -EPIPE; > + > + buffer_queue = &queue->pending_dqbufs; > + > + if (session->nonblocking_dequeue) { > + if (list_empty(buffer_queue)) > + return -EAGAIN; > + } else if (queue->allocated_bufs == 0) { > + return -EINVAL; > + } else if (!queue->streaming) { > + return -EINVAL; > + } > + > + /* > + * vv->lock has been acquired by virtio_media_device_ioctl. Release it > + * while we wait so that other ioctls for this session can be processed > + * and potentially trigger dqbuf_wait. > + */ > + mutex_unlock(&vv->vlock); > + > + /* > + * It's not possible to acquire ``session->error_lock`` during the > + * check, however this shouldn't cause issues because: > + * - The lock is acquired when actually checking the value > + * - The ``session->error`` flag is never reset once set > + */ > + ret = wait_event_interruptible(session->dqbuf_wait, > + !list_empty(buffer_queue) || > + session->error); [Severity: High] What happens if VIDIOC_STREAMOFF or VIDIOC_REQBUFS(0) is called while a thread is blocked in wait_event_interruptible() here? virtio_media_clear_queue() sets queue->streaming = false and empties queue->pending_dqbufs without calling wake_up(&session->dqbuf_wait), and the wait condition only checks !list_empty(buffer_queue) || session->error. Won't the blocked VIDIOC_DQBUF thread sleep indefinitely when the queue is streamed off? Also, because the checks above use else if: if (session->nonblocking_dequeue) { if (list_empty(buffer_queue)) return -EAGAIN; } else if (queue->allocated_bufs == 0) { return -EINVAL; } else if (!queue->streaming) { return -EINVAL; } when nonblocking_dequeue is true, calling VIDIOC_DQBUF on a queue with allocated_bufs == 0 or !queue->streaming skips the -EINVAL checks and returns -EAGAIN. And since session->nonblocking_dequeue is only initialized once in virtio_media_session_alloc() from file->f_flags & O_NONBLOCK, toggling O_NONBLOCK later via fcntl(F_SETFL) will not be reflected here. > + mutex_lock(&vv->vlock); > + if (ret) > + return -EINTR; > + > + mutex_lock(&session->error_lock); > + error = session->error; > + mutex_unlock(&session->error_lock); > + > + if (error) > + return -EIO; > + > + mutex_lock(&session->queues_lock); > + dqbuf = list_first_entry_or_null(buffer_queue, > + struct virtio_media_buffer, list); > + > + if (dqbuf) > + list_del(&dqbuf->list); > + > + mutex_unlock(&session->queues_lock); > + > + if (!dqbuf) { > + v4l2_err(&vv->v4l2_dev, "No buffer found when dqbuf event > triggered\n"); > + return -EIO; > + } > + > + /* Clear the DONE flag as the buffer is now being dequeued. */ > + dqbuf->buffer.flags &= ~V4L2_BUF_FLAG_DONE; > + > + if (is_multiplanar) { > + size_t nb_planes = min_t(u32, b->length, VIDEO_MAX_PLANES); > + > + memcpy(b->m.planes, dqbuf->planes, > + nb_planes * sizeof(struct v4l2_plane)); > + planes_backup = b->m.planes; > + } > + > + memcpy(b, &dqbuf->buffer, sizeof(*b)); > + > + if (is_multiplanar) > + b->m.planes = planes_backup; [Severity: Critical] Can overwriting b->length here (and in scatterlist_builder_retrieve_buffer() for VIDIOC_QUERYBUF, VIDIOC_PREPARE_BUF, and VIDIOC_QBUF) cause a kernel heap out-of-bounds read in 32-bit compat mode? In v4l2-ioctl.c:check_array_args() and video_usercopy(), the kernel allocates array_buf (passed as b->m.planes) sized to the user-supplied input b->length. Here in virtio_media_dqbuf(), if a 32-bit compat caller passes a smaller b->length (for example b->length = 1 when dqbuf->buffer.length is 2), memcpy(b, &dqbuf->buffer, sizeof(*b)) overwrites b->length with dqbuf->buffer.length (and if b->length was 0, b->m.planes is not even a kernel pointer). Similarly, scatterlist_builder_retrieve_buffer() overwrites b->length from the host response without clamping it to the input b->length. When video_usercopy() finishes a 32-bit compat ioctl, v4l2_compat_put_array_args() iterates b64->length times copying elements from array_buf to userspace, reading past the end of the allocated array_buf heap buffer. Shouldn't multiplanar buffer ioctls validate that b->m.planes is non-NULL and that the caller's b->length is sufficient before dequeuing or updating b->length? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
