Thank you for your contribution! Sashiko AI review found 18 potential issue(s) to consider: - [High] media: virtio: missing bounds check on host-provided MMAP offset - [High] media: virtio: unhandled vzalloc failure leaves queue->buffers NULL - [High] media: virtio: missing wakeup in clear_queue causes DQBUF hang - [High] media: virtio: TOCTOU race in dqbuf leads to list corruption - [High] media: virtio: vma->vm_pgoff overwritten with physical PFN corrupts interval tree - [High] media: virtio: un-decremented queued_bufs permanently blocks EPOLLOUT - [High] media: virtio: integer overflow in create_bufs leads to OOB write - [High] media: virtio: multiplication overflow in reqbufs results in OOB heap access - [High] media: virtio: list corruption via memcpy in array reallocation - [High] media: virtio: UAF due to devm_kzalloc of structure embedding video_device - [High] media: virtio: asynchronous session free leads to UAF on close - [High] media: virtio: premature unpinning of user pages allows physical memory corruption - [High] media: virtio: malicious host length overwrite leaks kernel memory - [High] media: virtio: TOCTOU in ext_controls check allows host to trigger OOB read - [Medium] media: virtio: host-side MMAP mapping leaked on io_remap_pfn_range failure - [Medium] media: virtio: missing bounds check underflows resp_len and masks device error - [Medium] media: virtio: leaked session resources when host close command fails - [Low] media: virtio: incorrect -EINVAL returned for copy_from_user failure --
commit 4f0fbb5128d42f2c68d224b7a3a87ede11d32bde Author: Alexandre Courbot <[email protected]> media: virtio: Add ioctl operations and driver logic This patch adds the ioctl operations and the remaining driver logic for polling and mmapping. It adds drivers/media/virtio/virtio_media_ioctls.c and updates virtio_media_driver.c to support poll, mmap, and ioctls. > diff --git a/drivers/media/virtio/virtio_media_driver.c > b/drivers/media/virtio/virtio_media_driver.c > index ef8065e344929..5840bb34972f6 100644 > --- a/drivers/media/virtio/virtio_media_driver.c > +++ b/drivers/media/virtio/virtio_media_driver.c > @@ -593,10 +602,191 @@ static int virtio_media_device_close(struct file *file) > { > 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->private_data); > > return virtio_media_session_close(vv, session); > } [Severity: High] Does this code introduce a Use-After-Free on close? If the host sends a VIRTIO_MEDIA_EVT_ERROR event, the driver asynchronously destroys and frees the session structure. When the file descriptor is subsequently closed, virtio_media_device_close() is invoked and dereferences the already freed session pointer. [Severity: Medium] Can this leak session resources when the host close command fails? If virtio_media_send_command() inside virtio_media_session_close() fails, the function returns early. The VFS will still tear down the file struct, permanently leaving the session in vv->sessions and leaking all associated memory allocations, shadow buffers, and queue buffers. [ ... ] > +static __poll_t virtio_media_device_poll(struct file *file, poll_table *wait) > +{ [ ... ] > + if (req_events & (EPOLLOUT | EPOLLWRNORM)) { > + if (!output_queue->streaming) > + rc |= EPOLLERR; > + else if (output_queue->queued_bufs < > + output_queue->allocated_bufs) > + rc |= EPOLLOUT | EPOLLWRNORM; > + } [Severity: High] Does a broken counter permanently block EPOLLOUT? Because virtio_media_qbuf() increments queue->queued_bufs but virtio_media_dqbuf() never decrements it when returning the buffer to userspace, the counter will quickly reach queue->allocated_bufs. This causes virtio_media_device_poll() to permanently fail the queued_bufs < allocated_bufs condition. [ ... ] > +static void virtio_media_vma_close_locked(struct vm_area_struct *vma) > +{ [ ... ] > + cmd_munmap->hdr.cmd = VIRTIO_MEDIA_CMD_MUNMAP; > + cmd_munmap->driver_addr = > + (vma->vm_pgoff << PAGE_SHIFT) - vv->mmap_region.addr; > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_munmap), > + NULL); [Severity: High] Does overwriting vma->vm_pgoff with a physical PFN corrupt the core MM interval tree? Because this is a VM_SHARED file mapping, the core MM uses vm_pgoff as the index coordinate to insert the VMA into mapping->i_mmap. Clobbering it with a raw physical PFN breaks memory unmapping and forking. [ ... ] > +static int virtio_media_device_mmap(struct file *file, > + struct vm_area_struct *vma) > +{ [ ... ] > + vma->vm_private_data = vv; > + /* > + * Keep the guest address at which the buffer is mapped since we will > + * use that to unmap. > + */ > + vma->vm_pgoff = (resp_mmap->driver_addr + vv->mmap_region.addr) >> > + PAGE_SHIFT; [Severity: High] Does this missing bounds check on the host-provided MMAP offset allow mapping arbitrary physical memory into guest userspace? The host provides a potentially malicious driver_addr offset during the MMAP command, which is added to mmap_region.addr and used for the page frame number mapping without bounds checking against mmap_region.len. [ ... ] > + ret = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff, > + vma->vm_end - vma->vm_start, > + vma->vm_page_prot); > + if (ret) > + goto end; > + > + vma->vm_ops = &virtio_media_vm_ops; > + > +end: > + mutex_unlock(&vv->vlock); > + return ret; > +} [Severity: Medium] Does the error path leak the host-side MMAP mapping? If io_remap_pfn_range() fails, the function exits early without installing virtio_media_vm_ops. The VMA is torn down by the core mm, but because vm_ops is missing, the .close callback is never executed and VIRTIO_MEDIA_CMD_MUNMAP is never sent to the host. [ ... ] > @@ -654,9 +844,14 @@ static int virtio_media_probe(struct virtio_device > *virtio_dev) > vv->eventq = vqs[1]; > INIT_WORK(&vv->eventq_work, virtio_media_event_work); > > + /* Get MMAP buffer mapping SHM region */ > + virtio_get_shm_region(virtio_dev, &vv->mmap_region, > + VIRTIO_MEDIA_SHM_MMAP); > + > vd = &vv->video_dev; > vd->v4l2_dev = &vv->v4l2_dev; > vd->vfl_type = VFL_TYPE_VIDEO; > + vd->ioctl_ops = &virtio_media_ioctl_ops; > vd->fops = &virtio_media_fops; > vd->release = video_device_release_empty; > strscpy(vd->name, "virtio-media", sizeof(vd->name)); [Severity: High] Does this code introduce a Use-After-Free on unbind? The structure containing video_device (virtio_media) is allocated via devm_kzalloc(). The release callback does not free the enclosing structure, deferring to devres which disregards file descriptor lifetimes. When the virtio device is unbound, devres frees the structure. If userspace still has an open file descriptor, subsequent operations will dereference the freed memory. > diff --git a/drivers/media/virtio/virtio_media_ioctls.c > b/drivers/media/virtio/virtio_media_ioctls.c > new file mode 100644 > index 0000000000000..f0b82b5ec7b4a > --- /dev/null > +++ b/drivers/media/virtio/virtio_media_ioctls.c [ ... ] > +static int virtio_media_send_ext_controls_ioctl(struct v4l2_fh *fh, u32 > ioctl, > + struct v4l2_ext_controls *ctrls) > +{ [ ... ] > + ret = virtio_media_send_command(vv, builder.sgs, num_cmd_sgs, > + builder.cur_sg - num_cmd_sgs, > + sizeof(struct virtio_media_resp_ioctl) + > + sizeof(*ctrls), > + &resp_len); > + > + /* Just in case the host touched these. */ > + ctrls->controls = controls_backup; > + if (ctrls->count != num_ctrls) { > + v4l2_err(&vv->v4l2_dev, > + "device returned a number of controls different than > the one submitted\n"); > + } > + if (ctrls->count > num_ctrls) > + return -ENOSPC; [Severity: High] Can an untrusted host exploit this Time-of-Check to Time-of-Use race to trigger an out-of-bounds read? The security check if (ctrls->count > num_ctrls) is executed before scatterlist_builder_retrieve_ext_ctrls() runs. The check passes on unmodified userspace input, but then data retrieval populates ctrls with the host's response, allowing the host to inject a huge count which the V4L2 core uses for an out-of-bounds copy. > + > + /* > + * Even if we have received an error, we may need to read our payload > + * back. > + */ > + if (ret < 0 && resp_len >= sizeof(struct virtio_media_resp_ioctl) + > + sizeof(*ctrls)) { > + /* > + * Deliberately ignore the error here as we want to return the > + * previous one. > + */ > + scatterlist_builder_retrieve_ext_ctrls(&builder, > + num_cmd_sgs + 1, ctrls); > + return ret; > + } > + > + resp_len -= sizeof(struct virtio_media_resp_ioctl); > + > + /* Make sure that the reply's length covers our v4l2_ext_controls */ > + if (resp_len < sizeof(*ctrls)) > + return -EINVAL; [Severity: Medium] Does this code silently mask device errors and return garbage data as success? If ret < 0 and resp_len is smaller than expected, the first size check is bypassed. The driver then executes resp_len -= sizeof(struct virtio_media_resp_ioctl), which underflows to a huge positive integer, defeating the subsequent < sizeof(*ctrls) check and falsely returning 0. [ ... ] > +static void virtio_media_clear_queue(struct virtio_media_session *session, > + struct virtio_media_queue_state *queue) > +{ [ ... ] > + queue->queued_bufs = 0; > + queue->streaming = false; > + queue->is_capture_last = false; > + > + mutex_unlock(&session->queues_lock); > +} [Severity: High] Does this missing wakeup cause a thread to hang indefinitely in VIDIOC_DQBUF? A userspace thread blocks in VIDIOC_DQBUF on wait_event_interruptible(). If a concurrent thread calls VIDIOC_STREAMOFF which triggers virtio_media_clear_queue(), the queue is cleared and streaming = false is set, but wake_up_all(&session->dqbuf_wait) is never called. The waiting thread will sleep forever. [ ... ] > +static int virtio_media_reqbufs(struct file *file, void *fh, > + struct v4l2_requestbuffers *b) > +{ [ ... ] > + /* REQBUFS(0) is an implicit STREAMOFF. */ > + if (b->count == 0) > + virtio_media_clear_queue(session, queue); > + > + vfree(queue->buffers); > + queue->buffers = NULL; [Severity: High] Can this reallocation cause a Use-After-Free or list corruption? The old array is freed without unlinking any currently pending buffers. If any of the buffers are currently queued in the queue->pending_dqbufs list, freeing the array leaves the active list pointing to freed memory. > + > + if (b->count > 0) { > + queue->buffers = > + vzalloc(sizeof(struct virtio_media_buffer) * b->count); > + if (!queue->buffers) > + return -ENOMEM; > + } > + > + queue->allocated_bufs = b->count; [Severity: High] Can a large b->count cause an integer multiplication overflow on 32-bit platforms? The driver allocates an array using vzalloc(sizeof(struct virtio_media_buffer) * b->count). If this overflows the 32-bit size_t, an undersized array is allocated. queue->allocated_bufs is set to the large b->count, allowing future userspace calls to bypass index bounds checks and access out-of-bounds heap memory. [Severity: High] Does an unhandled vzalloc() failure leave the queue in an inconsistent state? If vzalloc() fails, it returns -ENOMEM but leaves queue->allocated_bufs at its previous non-zero value. A subsequent VIDIOC_QBUF or VIDIOC_QUERYBUF call from userspace will pass the index check and dereference the NULL queue->buffers array, causing a kernel panic. [ ... ] > +static int virtio_media_create_bufs(struct file *file, void *fh, > + struct v4l2_create_buffers *b) > +{ [ ... ] > + queue->buffers = > + vzalloc(sizeof(*queue->buffers) * (b->index + b->count)); > + if (!queue->buffers) { > + queue->buffers = buffers; > + return -ENOMEM; > + } > + > + memcpy(queue->buffers, buffers, > + sizeof(*buffers) * queue->allocated_bufs); > + vfree(buffers); > + > + queue->allocated_bufs = b->index + b->count; [Severity: High] Can an untrusted host trigger a heap overflow here? The allocation size is computed using sizeof(*queue->buffers) * (b->index + b->count). Since index and count are 32-bit integers controlled by the virtio host, their sum can overflow, leading to an undersized allocation. The subsequent memcpy() copies queue->allocated_bufs items, causing a massive out-of-bounds write on the vmalloc heap. [Severity: High] Does the memcpy() and vfree() cause a Use-After-Free and list corruption? If any of the buffers are currently queued in the queue->pending_dqbufs list, memcpy() shallow-copies the list.next and list.prev pointers. When the old array is freed, the active list still points to the freed memory. [ ... ] > +static int virtio_media_qbuf(struct file *file, void *fh, struct v4l2_buffer > *b) > +{ [ ... ] > + ret = virtio_media_send_buffer_ioctl(vfh, VIDIOC_QBUF, b); > + if (ret) { > + /* Rollback the previous flags as the buffer is not queued. */ > + buffer->buffer.flags = old_flags; > + return ret; > + } > + > + queue->queued_bufs += 1; > + > + return 0; > +} [Severity: High] Does failing to decrement queue->queued_bufs permanently block EPOLLOUT? virtio_media_qbuf() increments queue->queued_bufs += 1 when queuing a buffer. virtio_media_dqbuf() never decrements it when returning the buffer to userspace. During normal device operation, the counter will quickly reach queue->allocated_bufs, causing virtio_media_device_poll() to permanently fail the queued_bufs < allocated_bufs condition. [ ... ] > +static int virtio_media_dqbuf(struct file *file, void *fh, > + struct v4l2_buffer *b) > +{ [ ... ] > + mutex_unlock(&vv->vlock); > + ret = wait_event_interruptible(session->dqbuf_wait, > + !list_empty(buffer_queue)); > + mutex_lock(&vv->vlock); > + if (ret) > + return -EINTR; > + > + mutex_lock(&session->queues_lock); > + dqbuf = list_first_entry(buffer_queue, struct virtio_media_buffer, > + list); > + list_del(&dqbuf->list); > + mutex_unlock(&session->queues_lock); [Severity: High] Does this logic contain a Time-of-Check to Time-of-Use race that leads to list corruption? wait_event_interruptible() verifies !list_empty(), but drops locks. When the thread resumes, it acquires the locks and unconditionally calls list_first_entry() and list_del(). If multiple threads are waiting in DQBUF, they can all wake up. The first thread empties the list, and subsequent threads corrupt the list by operating on an empty head. > + > + /* 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: High] Can a malicious host length overwrite leak kernel memory to userspace? The driver blindly executes memcpy(b, &dqbuf->buffer, sizeof(*b)), copying the host-provided buffer structure. If the untrusted host returns a larger b->length field, the V4L2 core video_usercopy() will use this modified length to copy planes to userspace, reading beyond the bounds of the original kernel-allocated plane array. [ ... ] > +long virtio_media_device_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ [ ... ] > + switch (cmd) { > + case VIDIOC_S_STD: > + ret = copy_from_user(&std_id, (void __user *)arg, > + sizeof(std_id)); > + if (ret) { > + ret = -EINVAL; > + break; > + } [Severity: Low] Should this return -EFAULT instead of -EINVAL for a failed copy_from_user()? Userspace expects -EFAULT when passing an invalid pointer to these ioctls. > diff --git a/drivers/media/virtio/scatterlist_builder.c > b/drivers/media/virtio/scatterlist_builder.c > --- a/drivers/media/virtio/scatterlist_builder.c > +++ b/drivers/media/virtio/scatterlist_builder.c [ ... ] > @@ -424,9 +424,6 @@ __scatterlist_builder_add_userptr(struct > scatterlist_builder *builder, > free_sg: > sg_free_table(&sg_table); > > done: > vb2_destroy_framevec(framevec); > return ret; > } [Severity: High] Does this prematurely unpin user pages while their physical addresses are still being used by the host? In __scatterlist_builder_add_userptr(), user memory is pinned to get physical addresses for the scatterlist. Before the function returns, vb2_destroy_framevec() unpins the pages unconditionally, even though the host device might still be actively performing DMA to those physical addresses asynchronously, allowing arbitrary physical memory corruption. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
