Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [Critical] media: virtio: session and commandq UAF on device removal with open fds - [High] media: virtio: uninitialized stack leak and wrong dir in g_input/g_output - [High] media: virtio: scatterlist_builder_add_data() maps stack memory for DMA - [High] media: virtio: virtio_media_device_ioctl() bypasses V4L2 std validation - [Medium] media: virtio: missing NUL termination of cap->card in virtio_media_querycap() - [Medium] media: virtio: KASAN tag mismatch in scatterlist_builder_retrieve_data() - [Medium] media: virtio: unaligned DMA allocations in scatterlist_builder_add_data() - [Low] media: virtio: undocumented module_param always_use_shadow_buffer - [Low] media: virtio: missing cpu_to_le32() in scatterlist_builder_add_ioctl_cmd() --
commit b04e2fc2518c3e0e5bfe29cd7359b457cc6bf7a3 Author: Alexandre Courbot <[email protected]> media: virtio: Add V4L2 ioctl forwarding infrastructure This commit introduces a scatterlist builder and synchronous V4L2 ioctl forwarding via VIRTIO_MEDIA_CMD_IOCTL. It uses this infrastructure to implement VIDIOC_QUERYCAP and fixed-size read, write, and read-write V4L2 ioctls. > diff --git a/drivers/media/virtio/scatterlist_builder.c > b/drivers/media/virtio/scatterlist_builder.c > new file mode 100644 > index 0000000000000..ae8ffee82fa6e > --- /dev/null > +++ b/drivers/media/virtio/scatterlist_builder.c > @@ -0,0 +1,222 @@ [ ... ] > +static bool always_use_shadow_buffer; > +module_param(always_use_shadow_buffer, bool, 0660); [Severity: Low] Should the new always_use_shadow_buffer module parameter be documented in Documentation/admin-guide/kernel-parameters.txt? [ ... ] > +int scatterlist_builder_add_data(struct scatterlist_builder *builder, > + void *data, size_t len) > +{ > + const size_t cur_desc = builder->cur_desc; > + > + if (len == 0) > + return 0; > + > + if (builder->cur_desc >= builder->num_descs) > + return -ENOSPC; > + > + if (!always_use_shadow_buffer && virt_addr_valid(data) && > + virt_addr_valid(data + len)) { > + /* > + * If "data" is in the 1:1 physical memory mapping then we can > + * use a single SG entry and avoid copying. > + */ > + struct page *page = virt_to_page(data); > + size_t offset = (((size_t)data) & ~PAGE_MASK); > + struct scatterlist *next_desc = > + &builder->descs[builder->cur_desc]; > + > + memset(next_desc, 0, sizeof(*next_desc)); > + sg_set_page(next_desc, page, len, offset); > + builder->cur_desc++; > + } else if (!always_use_shadow_buffer && is_vmalloc_addr(data)) { [Severity: High] Can scatterlist_builder_add_data() end up mapping kernel stack memory directly into the virtqueue scatterlist for DMA when always_use_shadow_buffer is false? Most V4L2 ioctl payloads passed here reside on the kernel stack: video_usercopy() in drivers/media/v4l2-core/v4l2-ioctl.c places any ioctl argument with _IOC_SIZE(cmd) <= 128 into char sbuf[128] on the stack, and virtio_media_g_input(), virtio_media_s_input(), virtio_media_g_output(), virtio_media_s_output(), virtio_media_s_std(), and virtio_media_device_ioctl() pass addresses of local stack variables (&input, &output, &s, &standard, and &std_id). When CONFIG_VMAP_STACK is disabled, stack addresses satisfy virt_addr_valid(); when CONFIG_VMAP_STACK is enabled, stack addresses satisfy is_vmalloc_addr(). Without a !object_is_on_stack(data) check, stack buffers bypass builder->shadow_buffer and are passed to virtqueue_add_sgs(), triggering check_for_stack() warnings in kernel/dma/debug.c and risking stack corruption on non-cache-coherent platforms while sleeping in wait_event(). Also, shouldn't the end-of-buffer check be virt_addr_valid(data + len - 1) rather than virt_addr_valid(data + len), and doesn't reusing descriptor index 1 in virtio_media_send_wr_ioctl() cause virtqueue_add_sgs() to DMA-map the same scatterlist entry twice concurrently as both DMA_TO_DEVICE and DMA_FROM_DEVICE? [ ... ] > + } else { > + /* > + * As a last resort, copy into the shadow buffer and reference > + * it with a single SG entry. Calling > + * scatterlist_builder_retrieve_data() will be necessary to copy > + * the data written by the device back into @data. > + */ > + void *shadow_buffer = > + builder->shadow_buffer + builder->shadow_buffer_pos; > + struct page *page = virt_to_page(shadow_buffer); > + unsigned long offset = > + (((unsigned long)shadow_buffer) & ~PAGE_MASK); > + struct scatterlist *next_desc = > + &builder->descs[builder->cur_desc]; > + > + if (len > > + builder->shadow_buffer_size - builder->shadow_buffer_pos) > + return -ENOSPC; > + > + memcpy(shadow_buffer, data, len); > + memset(next_desc, 0, sizeof(*next_desc)); > + sg_set_page(next_desc, page, len, offset); > + builder->cur_desc++; > + builder->shadow_buffer_pos += len; > + } [Severity: Medium] When multiple descriptors are sub-allocated from builder->shadow_buffer (for example when always_use_shadow_buffer is true during virtio_media_send_r_ioctl() or virtio_media_send_wr_ioctl()), does advancing builder->shadow_buffer_pos by the unaligned len cause adjacent DMA_FROM_DEVICE and DMA_TO_DEVICE descriptors to share a CPU cache line? virtio_media_send_r_ioctl() places the 16-byte virtio_media_cmd_ioctl at offset 0, the 8-byte virtio_media_resp_ioctl at offset 16, and ioctl_data at offset 24. When virtqueue_add_sgs() maps each scatterlist entry separately for DMA, unaligned offsets within session->shadow_buf can trigger CONFIG_DMA_API_DEBUG overlapping cacheline warnings and corrupt adjacent response data during cache invalidation on non-coherent DMA platforms unless shadow_buffer_pos is aligned to dma_get_cache_alignment() / ARCH_DMA_MINALIGN. [ ... ] > +int scatterlist_builder_retrieve_data(struct scatterlist_builder *builder, > + size_t sg_index, void *data) > +{ > + void *shadow_buf = builder->shadow_buffer; > + struct scatterlist *sg; > + void *kaddr; > + > + /* We can only retrieve from the range of sgs currently set. */ > + if (sg_index >= builder->cur_sg) > + return -ERANGE; > + > + sg = builder->sgs[sg_index]; > + kaddr = pfn_to_kaddr(page_to_pfn(sg_page(sg))) + sg->offset; > + > + if (kaddr >= shadow_buf && > + kaddr < shadow_buf + builder->shadow_buffer_size) { > + if (kaddr + sg->length > > + shadow_buf + builder->shadow_buffer_size) > + return -EINVAL; > + > + memcpy(data, kaddr, sg->length); > + } > + > + return 0; > +} [Severity: Medium] On arm64 with CONFIG_KASAN_SW_TAGS or CONFIG_KASAN_HW_TAGS enabled, will the range check in scatterlist_builder_retrieve_data() fail to match shadow_buf? In virtio_media_session_alloc(), session->shadow_buf is allocated with kzalloc(VIRTIO_SHADOW_BUF_SIZE, GFP_KERNEL), which returns a pointer with a KASAN tag in bits 63:56. Here, kaddr is computed via pfn_to_kaddr(), which produces an untagged linear address with 0xff in bits 63:56. Comparing kaddr < shadow_buf + builder->shadow_buffer_size compares an 0xff-tagged address against a tagged pointer, which evaluates to false and silently skips copying the response payload out of the shadow buffer. Should kasan_reset_tag() (or page_address(sg_page(sg)) / sg_virt(sg)) be used when comparing and copying from shadow_buf? > + > +/** > + * scatterlist_builder_add_ioctl_cmd() - Add an ioctl command to the > descriptor > + * chain. > + * @builder: builder to use. > + * @session: session on behalf of which the ioctl command is added. > + * @ioctl_code: code of the ioctl to add (i.e. ``VIDIOC_*``). > + */ > +int scatterlist_builder_add_ioctl_cmd(struct scatterlist_builder *builder, > + struct virtio_media_session *session, > + u32 ioctl_code) > +{ > + struct virtio_media_cmd_ioctl *cmd_ioctl = &session->cmd.ioctl; > + > + cmd_ioctl->hdr.cmd = VIRTIO_MEDIA_CMD_IOCTL; > + cmd_ioctl->hdr.__reserved = 0; > + cmd_ioctl->session_id = session->id; > + cmd_ioctl->code = VIRTIO_MEDIA_IOCTL_CODE(ioctl_code); [Severity: Low] Since hdr.cmd, session_id, and code in struct virtio_media_cmd_ioctl are declared as __le32 in include/uapi/linux/virtio_media.h, should these assignments use cpu_to_le32() to avoid Sparse -Wbitwise warnings? [ ... ] > diff --git a/drivers/media/virtio/virtio_media_ioctls.c > b/drivers/media/virtio/virtio_media_ioctls.c > new file mode 100644 > index 0000000000000..656ecea8a44cc > --- /dev/null > +++ b/drivers/media/virtio/virtio_media_ioctls.c > @@ -0,0 +1,606 @@ [ ... ] > +static int virtio_media_querycap(struct file *file, void *fh, > + struct v4l2_capability *cap) > +{ > + struct video_device *video_dev = video_devdata(file); > + struct virtio_media *vv = to_virtio_media(video_dev); > + > + strscpy(cap->bus_info, "platform:virtio-media"); > + strscpy(cap->driver, VIRTIO_MEDIA_DEFAULT_DRIVER_NAME); > + > + virtio_cread_bytes(vv->virtio_dev, 8, cap->card, sizeof(cap->card)); [Severity: Medium] Does virtio_media_querycap() need to ensure cap->card is NUL-terminated after virtio_cread_bytes()? In include/uapi/linux/virtio_media.h, the comment for struct virtio_media_config states that if all 32 characters of card are used by the device, it does not need to be NUL-terminated: * @card: name of the device, a NUL-terminated UTF-8 string. It corresponds * with the card field of the struct v4l2_capability. If all the * characters of the field are used, it does not need to be * NUL-terminated. However, V4L2 requires struct v4l2_capability.card to always be NUL-terminated when returned to userspace by VIDIOC_QUERYCAP. > + > + cap->capabilities = video_dev->device_caps | V4L2_CAP_DEVICE_CAPS; > + cap->device_caps = video_dev->device_caps; > + > + return 0; > +} > + > +/* > + * s/g_input/output work with an unsigned int - recast this to a u32 so the > + * size is unambiguous. > + */ > + > +static int virtio_media_g_input(struct file *file, void *fh, unsigned int *i) > +{ > + u32 input; > + > + int ret = virtio_media_send_wr_ioctl(file_to_v4l2_fh(file), > + VIDIOC_G_INPUT, &input, > + sizeof(input), sizeof(input)); > + if (ret) > + return ret; > + > + *i = input; > + > + return 0; > +} > + > +static int virtio_media_s_input(struct file *file, void *fh, unsigned int i) > +{ > + u32 input = i; > + > + return virtio_media_send_wr_ioctl(file_to_v4l2_fh(file), > + VIDIOC_S_INPUT, &input, > + sizeof(input), sizeof(input)); > +} > + > +static int virtio_media_g_output(struct file *file, void *fh, unsigned int > *o) > +{ > + u32 output; > + > + int ret = virtio_media_send_wr_ioctl(file_to_v4l2_fh(file), > + VIDIOC_G_OUTPUT, &output, > + sizeof(output), sizeof(output)); [Severity: High] Should virtio_media_g_input() and virtio_media_g_output() call virtio_media_send_r_ioctl() instead of virtio_media_send_wr_ioctl()? Both VIDIOC_G_INPUT and VIDIOC_G_OUTPUT are defined with _IOR in include/uapi/linux/videodev2.h. Per the VIRTIO_MEDIA_CMD_IOCTL documentation in include/uapi/linux/virtio_media.h, _IOR ioctls should only be followed by a device-writable response descriptor. Calling virtio_media_send_wr_ioctl() attaches a device-readable command payload descriptor for an _IOR ioctl and sends 4 bytes of uninitialized stack memory from input and output to the host (or copies uninitialized bytes into session->shadow_buf via memcpy()). [ ... ] > +long virtio_media_device_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct video_device *video_dev = video_devdata(file); > + struct virtio_media *vv = to_virtio_media(video_dev); > + struct v4l2_fh *vfh = NULL; > + struct v4l2_standard standard; > + v4l2_std_id std_id = 0; > + int ret; > + > + if (test_bit(V4L2_FL_USES_V4L2_FH, &video_dev->flags)) > + vfh = file_to_v4l2_fh(file); > + > + mutex_lock(&vv->vlock); [Severity: Critical] What prevents a use-after-free of the session and vv->commandq if virtio_media_remove() runs while a file descriptor is open or an ioctl is waiting on vv->vlock? In drivers/media/virtio/virtio_media_driver.c, virtio_media_remove() does: video_unregister_device(&vv->video_dev); mutex_lock(&vv->vlock); virtio_reset_device(virtio_dev); while (virtqueue_detach_unused_buf(vv->commandq)) ; virtio_dev->config->del_vqs(virtio_dev); mutex_unlock(&vv->vlock); 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); } If a thread passes v4l2_ioctl()'s unlocked video_is_registered() check and waits on mutex_lock(&vv->vlock) in virtio_media_device_ioctl() while virtio_media_remove() runs, virtio_media_remove() deletes vv->commandq and frees all sessions (including s->command_sgs and s->shadow_buf). Once virtio_media_device_ioctl() acquires vv->vlock, it proceeds without checking video_is_registered(video_dev), dereferencing the freed vfh / session and submitting descriptors to the deleted vv->commandq. Furthermore, when userspace later closes the open file descriptor, v4l2_release() calls virtio_media_device_close(), which looks up the already freed session from file->private_data, accesses vv->commandq in virtio_media_session_close(), and calls virtio_media_session_remove() and virtio_media_session_free() a second time. > + > + /* > + * We need to handle a few ioctls manually because their results > + * rely on vfd->tvnorms, which is normally updated by the driver > + * as S_INPUT is called. Since we want to just pass these ioctls > + * through, we have to hijack them from here. > + */ > + switch (cmd) { > + case VIDIOC_S_STD: > + ret = copy_from_user(&std_id, (void __user *)arg, > + sizeof(std_id)); > + if (ret) { > + ret = -EFAULT; > + break; > + } > + ret = virtio_media_s_std(file, vfh, std_id); > + break; > + case VIDIOC_ENUMSTD: > + ret = copy_from_user(&standard, (void __user *)arg, > + sizeof(standard)); > + if (ret) { > + ret = -EFAULT; > + break; > + } > + ret = virtio_media_enumstd(file, vfh, &standard); > + if (ret) > + break; > + ret = copy_to_user((void __user *)arg, &standard, > + sizeof(standard)); > + if (ret) > + ret = -EFAULT; > + break; > + case VIDIOC_QUERYSTD: > + ret = virtio_media_querystd(file, vfh, &std_id); > + if (ret) > + break; > + ret = copy_to_user((void __user *)arg, &std_id, sizeof(std_id)); > + if (ret) > + ret = -EFAULT; > + break; > + default: > + ret = video_ioctl2(file, cmd, arg); > + break; > + } [Severity: High] By intercepting VIDIOC_S_STD, VIDIOC_ENUMSTD, and VIDIOC_QUERYSTD before video_ioctl2(), doesn't virtio_media_device_ioctl() bypass several V4L2 core checks and 32-bit compat handling? Specifically: 1. __video_do_ioctl() checks test_bit(_IOC_NR(cmd), video_dev->valid_ioctls) and calls v4l2_prio_check(video_dev->prio, vfh->prio) for VIDIOC_S_STD (which is marked INFO_FL_PRIO). Intercepting them here skips both device validity and priority checks. 2. For VIDIOC_ENUMSTD, v4l2_ioctls[] uses INFO_FL_CLEAR(v4l2_standard, index) so video_get_user() zeroes all output fields after index, whereas copy_from_user() here forwards unzeroed trailing fields from userspace to the host. 3. Under CONFIG_COMPAT on x86_64, 32-bit userspace issues VIDIOC_ENUMSTD32 because struct v4l2_standard32 differs in size and alignment. v4l2_compat_ioctl32() passes VIDIOC_ENUMSTD32 to fops->unlocked_ioctl; because the switch statement only matches native VIDIOC_ENUMSTD, VIDIOC_ENUMSTD32 falls through to video_ioctl2() -> v4l_enumstd(), where video_dev->tvnorms is 0, failing with -EINVAL. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
