Re: [PATCH v10 2/5] media: virtio: Add V4L2 ioctl forwarding infrastructure

[sashiko-bot@kernel.org]

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 <gnurou@gmail.com>

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/20261029153038.3800747-1-briandaniels@google.com?part=2