Re: [PATCH v10 5/5] media: virtio: Add MMAP buffer mapping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] media: virtio: missing VMA open callback and broken munmap offset on VMA split
- [High] media: virtio: missing SHM bounds and offset checks in virtio_media_device_mmap()
- [High] media: virtio: read-only mmap leaves VM_MAYWRITE set in virtio_media_device_mmap()
- [High] media: virtio: access to freed commandq in virtio_media_vma_close()
- [High] media: virtio: ABBA deadlock between vv->vlock and mm->mmap_lock
- [Medium] media: virtio: missing session->error check in virtio_media_device_mmap()
--

commit f2fa31d4e435156536271ee6a79a737826897ef4
Author: Alexandre Courbot <gnurou@gmail.com>

media: virtio: Add MMAP buffer mapping support

This commit adds support for mapping V4L2_MEMORY_MMAP buffers into guest
userspace via virtio shared memory region 0 using VIRTIO_MEDIA_CMD_MMAP and
VIRTIO_MEDIA_CMD_MUNMAP.

> diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> index 877ac154b3221..865b6dfad7fb6 100644
> --- a/drivers/media/virtio/virtio_media_driver.c
> +++ b/drivers/media/virtio/virtio_media_driver.c
[ ... ]
> @@ -687,12 +695,148 @@ static __poll_t virtio_media_device_poll(struct file *file, poll_table *wait)
>  	return rc;
>  }
>  
> +static void virtio_media_vma_close_locked(struct vm_area_struct *vma)
> +{
> +	struct virtio_media *vv = vma->vm_private_data;
> +	struct virtio_media_cmd_munmap *cmd_munmap = &vv->cmd.munmap;
> +	struct virtio_media_resp_munmap *resp_munmap = &vv->resp.munmap;
> +	struct scatterlist cmd_sg = {}, resp_sg = {};
> +	struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> +	int ret;
> +
> +	sg_set_buf(&cmd_sg, cmd_munmap, sizeof(*cmd_munmap));
> +	sg_mark_end(&cmd_sg);
> +
> +	sg_set_buf(&resp_sg, resp_munmap, sizeof(*resp_munmap));
> +	sg_mark_end(&resp_sg);
> +
> +	cmd_munmap->hdr.cmd = VIRTIO_MEDIA_CMD_MUNMAP;
> +	cmd_munmap->hdr.__reserved = 0;
> +	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);
> +	if (ret < 0) {
> +		v4l2_err(&vv->v4l2_dev, "host failed to unmap buffer: %d\n",
> +			 ret);
> +	}
> +}
> +
> +/**
> + * virtio_media_vma_close() - Close a MMAP buffer mapping.
> + * @vma: VMA of the mapping to close.
> + *
> + * Inform the host that a previously created MMAP mapping is no longer needed
> + * and can be removed.
> + */
> +static void virtio_media_vma_close(struct vm_area_struct *vma)
> +{
> +	struct virtio_media *vv = vma->vm_private_data;
> +
> +	mutex_lock(&vv->vlock);
> +	virtio_media_vma_close_locked(vma);
> +	mutex_unlock(&vv->vlock);
> +}

[Severity: High]
Can virtio_media_vma_close() dereference a freed vv->commandq if the virtio
device is removed while a buffer remains mapped in userspace?

When a process maps a buffer via virtio_media_device_mmap(), vma->vm_file
keeps the struct file and vv->video_dev alive across virtio_media_remove().
However, virtio_media_remove() calls virtio_dev->config->del_vqs(virtio_dev),
which frees vv->commandq, and frees all sessions in vv->sessions:

drivers/media/virtio/virtio_media_driver.c:virtio_media_remove() {
    ...
    video_unregister_device(&vv->video_dev);
    virtio_media_device_reset(virtio_dev);
    virtio_dev->config->del_vqs(virtio_dev);
    ...
}

When the process later calls munmap() or exits, remove_vma() -> vma_close()
invokes virtio_media_vma_close() -> virtio_media_vma_close_locked() ->
virtio_media_send_command() -> virtio_media_kick_command(), which accesses
vv->commandq without checking if the device was unregistered.

Similarly, in v4l2_mmap() -> virtio_media_device_mmap(), video_is_registered()
is checked before acquiring vv->vlock. If virtio_media_remove() runs before
virtio_media_device_mmap() acquires vv->vlock, won't virtio_media_device_mmap()
write to the freed session->cmd.mmap and dereference the freed vv->commandq?

> +
> +static const struct vm_operations_struct virtio_media_vm_ops = {
> +	.close = virtio_media_vma_close,
> +};

[Severity: High]
What happens when a mapped VMA is duplicated or split via fork(), mremap(), or
partial munmap()/mprotect()?

Because virtio_media_vm_ops only defines .close and omits .open (and
vma->vm_private_data stores vv directly rather than a reference-counted
mapping structure like vb2_common_vm_ops), dup_mmap(), copy_vma(), and
__split_vma() create additional vm_area_struct instances without incrementing
any reference count. When the first VMA closes, virtio_media_vma_close() will
send VIRTIO_MEDIA_CMD_MUNMAP to the host while the remaining VMA(s) are still
mapped in guest userspace, and closing the remaining VMA(s) later will send
unbalanced VIRTIO_MEDIA_CMD_MUNMAP commands.

Furthermore, when __split_vma() splits a VMA at addr, it advances
vma->vm_pgoff on the upper VMA by linear_page_delta(vma, addr). When that
upper VMA is closed, virtio_media_vma_close_locked() computes:

	cmd_munmap->driver_addr =
		(vma->vm_pgoff << PAGE_SHIFT) - vv->mmap_region.addr;

which sends a shifted interior offset instead of the base driver_addr returned
by VIRTIO_MEDIA_CMD_MMAP, leaking the host mapping.

> +
> +/**
> + * virtio_media_device_mmap() - Perform a mmap request from userspace.
> + * @file: opened file of the session to map for.
> + * @vma: VM area struct describing the desired mapping.
> + *
> + * This requests the host to map a MMAP buffer for us, so we can then make that
> + * mapping visible into user-space address space.
> + */
> +static int virtio_media_device_mmap(struct file *file,
> +				    struct vm_area_struct *vma)
> +{
> +	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);
> +	struct virtio_media_cmd_mmap *cmd_mmap = &session->cmd.mmap;
> +	struct virtio_media_resp_mmap *resp_mmap = &session->resp.mmap;
> +	struct scatterlist cmd_sg = {}, resp_sg = {};
> +	struct scatterlist *sgs[2] = { &cmd_sg, &resp_sg };
> +	int ret;
> +
> +	if (!(vma->vm_flags & VM_SHARED))
> +		return -EINVAL;
> +	if (!(vma->vm_flags & (VM_READ | VM_WRITE)))
> +		return -EINVAL;
> +
> +	mutex_lock(&vv->vlock);

[Severity: High]
Could acquiring vv->vlock in virtio_media_device_mmap() and
virtio_media_vma_close() cause an ABBA deadlock with mm->mmap_lock?

Both virtio_media_device_mmap() and virtio_media_vma_close() are called by the
MM core while holding current->mm->mmap_lock, establishing the lock order
mm->mmap_lock -> vv->vlock.

However, in drivers/media/virtio/virtio_media_ioctls.c,
virtio_media_device_ioctl() acquires vv->vlock and holds it across
copy_from_user(), copy_to_user(), and video_ioctl2() (which calls
video_usercopy(), and for VIDIOC_G/S/TRY_EXT_CTRLS calls
virtio_media_send_ext_controls_ioctl() ->
scatterlist_builder_add_ext_ctrls_userptrs() ->
__scatterlist_builder_add_userptr() -> vb2_create_framevec() ->
get_vaddr_frames() -> pin_user_pages_fast()). Those usercopy faults and GUP
paths acquire current->mm->mmap_lock while holding vv->vlock, establishing the
reverse order vv->vlock -> mm->mmap_lock.

> +
> +	cmd_mmap->hdr.cmd = VIRTIO_MEDIA_CMD_MMAP;
> +	cmd_mmap->hdr.__reserved = 0;
> +	cmd_mmap->session_id = session->id;

[Severity: Medium]
Should virtio_media_device_mmap() check session->error before sending
VIRTIO_MEDIA_CMD_MMAP?

When VIRTIO_MEDIA_EVT_ERROR is processed in virtio_media_process_events(), the
driver sets session->error = true and calls virtio_media_session_close(vv,
session) to close session->id on the host while the guest file descriptor is
still open. Unlike virtio_media_device_ioctl(), which checks session->error
and returns -EIO, virtio_media_device_mmap() does not check session->error and
will send VIRTIO_MEDIA_CMD_MMAP with a session_id that has already been closed
on the host.

> +	cmd_mmap->flags =
> +		(vma->vm_flags & VM_WRITE) ? VIRTIO_MEDIA_MMAP_FLAG_RW_MASK : 0;

[Severity: High]
When a user opens the video device with O_RDWR and calls mmap() with PROT_READ
and MAP_SHARED, the MM core sets VM_MAYWRITE on vma->vm_flags while leaving
VM_WRITE clear. Here, virtio_media_device_mmap() requests a read-only mapping
from the host (cmd_mmap->flags = 0), but leaves VM_MAYWRITE set on the VMA.

Can userspace subsequently call mprotect(addr, len, PROT_READ | PROT_WRITE) to
make the guest PTEs writable for a buffer that was mapped read-only on the
host? Should vm_flags_clear(vma, VM_MAYWRITE) be called when VM_WRITE is not
set?

> +	cmd_mmap->offset = vma->vm_pgoff << PAGE_SHIFT;
> +	cmd_mmap->__reserved = 0;
> +
> +	sg_set_buf(&cmd_sg, cmd_mmap, sizeof(*cmd_mmap));
> +	sg_mark_end(&cmd_sg);
> +
> +	sg_set_buf(&resp_sg, resp_mmap, sizeof(*resp_mmap));
> +	sg_mark_end(&resp_sg);
> +
> +	/*
> +	 * The host performs reference counting and is smart enough to return
> +	 * the same guest physical address if this is called several times on
> +	 * the same
> +	 * buffer.
> +	 */
> +	ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_mmap),
> +					NULL);
> +	if (ret < 0)
> +		goto end;
> +
> +	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;
> +
> +	/*
> +	 * We cannot let the mapping be larger than the buffer.
> +	 */
> +	if (vma->vm_end - vma->vm_start > PAGE_ALIGN(resp_mmap->len)) {
> +		dev_dbg(&video_dev->dev,
> +			"invalid MMAP, as it would overflow buffer length\n");
> +		ret = -EINVAL;
> +		goto cleanup;
> +	}
> +
> +	ret = io_remap_pfn_range(vma, vma->vm_start, vma->vm_pgoff,
> +				 vma->vm_end - vma->vm_start,
> +				 vma->vm_page_prot);

[Severity: High]
Does virtio_media_device_mmap() need to validate cmd_mmap->offset and the
returned resp_mmap->driver_addr and resp_mmap->len against vv->mmap_region?

First, cmd_mmap->offset is a 32-bit __le32, so if userspace passes a page
offset where vma->vm_pgoff << PAGE_SHIFT >= 4 GiB, the upper bits are silently
truncated instead of returning -EINVAL.

Second, resp_mmap->driver_addr and resp_mmap->len are used to compute
vma->vm_pgoff and remap the range via io_remap_pfn_range() without checking
that resp_mmap->driver_addr is page-aligned, that the additions do not
overflow, or that the mapped range [driver_addr, driver_addr + size) actually
lies within vv->mmap_region.len. If the host returns an out-of-bounds
driver_addr or len, could io_remap_pfn_range() map arbitrary guest physical
memory outside the virtio shared memory region into userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029153038.3800747-1-briandaniels@google.com?part=5