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 <[email protected]> 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/[email protected]?part=5
