Re: [PATCH v10 1/5] media: virtio: Add core driver and session management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] media: virtio: UAF, NULL deref, and deadlock in virtio_media_remove()
- [Critical] media: virtio: stack UAF and vq race in commandq_callback()
- [Critical] media: virtio: dangling stack cb_param on virtqueue_kick() failure
- [Critical] media: virtio: unvalidated status negation in virtio_media_kick_command()
- [High] media: virtio: missing reset and caps check in virtio_media_probe()
- [Low] media: virtio: missing le32 conversions on virtio_media protocol fields
--

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

media: virtio: Add core driver and session management

This commit adds the initial virtio-media driver core, build system
integration, and V4L2 session lifecycle management via VIRTIO_MEDIA_CMD_OPEN
and VIRTIO_MEDIA_CMD_CLOSE over the command virtqueue.

> diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> new file mode 100644
> index 0000000000000..10073d863b574
> --- /dev/null
> +++ b/drivers/media/virtio/virtio_media_driver.c
> @@ -0,0 +1,503 @@

[ ... ]

> +static int virtio_media_session_close(struct virtio_media *vv,
> +				      struct virtio_media_session *session)
> +{
> +	struct virtio_media_cmd_close *cmd_close = &session->cmd.close;
> +	struct scatterlist cmd_sg = {};
> +	struct scatterlist *sgs[1] = { &cmd_sg };
> +	int ret;
> +
> +	mutex_lock(&vv->vlock);
> +
> +	cmd_close->hdr.cmd = VIRTIO_MEDIA_CMD_CLOSE;
> +	cmd_close->hdr.__reserved = 0;
> +	cmd_close->session_id = session->id;
> +	cmd_close->__reserved = 0;

[Severity: Low]
In virtio_media_session_close(), virtio_media_device_open(), and
virtio_media_kick_command(), the protocol fields hdr.cmd, hdr.status, and
session_id are declared as __le32 in include/uapi/linux/virtio_media.h, but
they are read and written directly as host integers.

Should these accesses use cpu_to_le32() and le32_to_cpu() to avoid sparse
endianness warnings when building with C=1?

[ ... ]

> +struct virtio_media_cmd_callback_param {
> +	struct virtio_media *vv;
> +	bool done;
> +	size_t resp_len;
> +};
> +
> +/**
> + * commandq_callback() - Callback for the command queue.
> + * @queue: command virtqueue.
> + *
> + * This just wakes up the thread that was waiting on the command to complete.
> + */
> +static void commandq_callback(struct virtqueue *queue)
> +{
> +	unsigned int len;
> +	struct virtio_media_cmd_callback_param *param;
> +
> +process_bufs:
> +	while ((param = virtqueue_get_buf(queue, &len))) {
> +		param->resp_len = len;
> +		param->done = true;
> +		wake_up(&param->vv->wq);
> +	}
> +
> +	if (!virtqueue_enable_cb(queue)) {
> +		virtqueue_disable_cb(queue);
> +		goto process_bufs;
> +	}
> +}
> +
> +/**
> + * virtio_media_kick_command() - send a command to the commandq.
> + * @vv: virtio-media device in use.
> + * @sgs: descriptor chain to send.
> + * @out_sgs: number of device-readable descriptors in @sgs.
> + * @in_sgs: number of device-writable descriptors in @sgs.
> + * @resp_len: output parameter. Upon success, contains the size of the response
> + *            in bytes.
> + */
> +static int virtio_media_kick_command(struct virtio_media *vv,
> +				     struct scatterlist **sgs,
> +				     const size_t out_sgs, const size_t in_sgs,
> +				     size_t *resp_len)
> +{
> +	struct virtio_media_cmd_callback_param cb_param = {
> +		.vv = vv,
> +		.done = false,
> +		.resp_len = 0,
> +	};
> +	struct virtio_media_resp_header *resp_header;
> +	int ret = virtqueue_add_sgs(vv->commandq, sgs, out_sgs, in_sgs,
> +		  &cb_param, GFP_ATOMIC);
> +	if (ret) {
> +		v4l2_err(&vv->v4l2_dev,
> +			 "failed to add sgs to command virtqueue\n");
> +		return ret;
> +	}
> +
> +	if (!virtqueue_kick(vv->commandq)) {
> +		v4l2_err(&vv->v4l2_dev, "failed to kick command virtqueue\n");
> +		return -EINVAL;
> +	}

[Severity: Critical]
If virtqueue_kick() returns false here in virtio_media_kick_command(), the
stack-allocated cb_param token and the DMA-mapped caller scatterlists have
already been added to vv->commandq by virtqueue_add_sgs().

When virtio_media_kick_command() returns -EINVAL without detaching the unused
buffer or resetting the device, cb_param goes out of scope while still queued
in vv->commandq, and in the close path:

  virtio_media_device_close()
    virtio_media_session_close() -> fails with -EINVAL
    virtio_media_session_remove()
    virtio_media_session_free() -> kfree(session)

session->cmd.close is freed while still queued in the virtqueue. Could a
subsequent completion in commandq_callback() dereference the dangling stack
pointer param and access freed session memory?

> +
> +	/* Wait for the response. */
> +	wait_event(vv->wq, cb_param.done);

[Severity: Critical]
Can commandq_callback() race with virtio_media_kick_command() and trigger a
stack use-after-return or vring state corruption?

In virtio_media_kick_command(), cb_param lives on the stack, and wait_event()
checks cb_param.done locklessly in its fast path before taking the waitqueue
lock. In commandq_callback():

  param->resp_len = len;
  param->done = true;
  wake_up(&param->vv->wq);

As soon as param->done = true becomes visible on another CPU,
virtio_media_kick_command() can finish wait_event() and return from the
function before commandq_callback() evaluates &param->vv->wq on the next
line, dereferencing the popped stack frame (for example, queue->vdev->priv
could be used to reach vv instead of param->vv, or a struct completion could
be used). In addition, param->resp_len and param->done are written and read
without memory barriers (such as smp_store_release() and smp_load_acquire())
when wait_event() takes its lockless fast path.

Furthermore, callers serialize access to vv->commandq only with the mutex
vv->vlock, whereas commandq_callback() runs in interrupt context without a
spinlock. Once a woken caller returns, releases vv->vlock, and issues a new
virtqueue_add_sgs() and virtqueue_kick() (or virtio_media_remove() detaches
buffers), can that run concurrently with commandq_callback() still executing
virtqueue_get_buf() and virtqueue_enable_cb() on vv->commandq?

> +
> +	if (resp_len)
> +		*resp_len = cb_param.resp_len;
> +
> +	if (in_sgs > 0) {
> +		/*
> +		 * If we expect a response, make sure we have at least a
> +		 * response header - anything shorter is invalid.
> +		 */
> +		if (cb_param.resp_len < sizeof(*resp_header)) {
> +			v4l2_err(&vv->v4l2_dev,
> +				 "received response header is too short\n");
> +			return -EINVAL;
> +		}
> +
> +		resp_header = sg_virt(sgs[out_sgs]);
> +		if (resp_header->status)
> +			/* Host returns a positive error code. */
> +			return -resp_header->status;

[Severity: Critical]
What happens if the host writes an out-of-range value into
resp_header->status (which is a 32-bit unsigned __le32)?

If resp_header->status has bit 31 set (0x80000001..0xffffffff, such as a
negative errno cast to u32), negating it yields a positive int when returned
from virtio_media_kick_command(). Because virtio_media_send_command() only
checks:

  if (ret < 0)
          return ret;

it treats positive return values as success and returns 0, causing
virtio_media_device_open() to succeed using an uninitialized or stale
resp_open->session_id.

Conversely, if resp_header->status is in (MAX_ERRNO, 0x80000000],
virtio_media_kick_command() returns a negative value smaller than -MAX_ERRNO.
When propagated from virtio_media_device_open() through v4l2_open() and
do_dentry_open() to path_openat(), ERR_PTR(ret) will not satisfy IS_ERR(),
which can cause VFS open to install an invalid pointer into the process file
descriptor table. Should status values outside 1..MAX_ERRNO be mapped to a
fallback error such as -EINVAL or -EIO?

[ ... ]

> +static int virtio_media_probe(struct virtio_device *virtio_dev)
> +{

[ ... ]

> +	virtio_cread_le(virtio_dev, struct virtio_media_config, device_caps,
> +			&vd->device_caps);
> +	if (vd->device_caps & (V4L2_CAP_VIDEO_M2M | V4L2_CAP_VIDEO_M2M_MPLANE))
> +		vd->vfl_dir = VFL_DIR_M2M;
> +	else if (vd->device_caps &
> +		 (V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_VIDEO_OUTPUT_MPLANE))
> +		vd->vfl_dir = VFL_DIR_TX;
> +	else
> +		vd->vfl_dir = VFL_DIR_RX;
> +
> +	virtio_device_ready(virtio_dev);
> +
> +	__u32 device_type;
> +
> +	virtio_cread_le(virtio_dev, struct virtio_media_config, device_type,
> +			&device_type);
> +	if (device_type >= VFL_TYPE_MAX) {
> +		dev_err(&virtio_dev->dev, "Invalid device_type: %u",
> +			device_type);
> +		ret = -EINVAL;
> +		goto err_del_vqs;
> +	}
> +
> +	ret = video_register_device(vd, device_type, -1);
> +	if (ret)
> +		goto err_del_vqs;
> +
> +	return 0;
> +
> +err_del_vqs:
> +	virtio_dev->config->del_vqs(virtio_dev);
> +err_v4l2_device_unregister:
> +	v4l2_device_unregister(&vv->v4l2_dev);
> +err_free_virtio_media:
> +	kfree(vv);
> +	return ret;
> +}

[Severity: High]
In virtio_media_probe(), virtio_device_ready(virtio_dev) is called before
validating device_type and before video_register_device(). If either check
fails and jumps to err_del_vqs, virtio_dev->config->del_vqs(virtio_dev) and
kfree(vv) are called without calling virtio_reset_device(virtio_dev) first,
leaving VIRTIO_CONFIG_S_DRIVER_OK set while the virtqueues and driver state
are freed.

In addition, vd->device_caps is read from config space without checking that
it is non-zero; if device_type != VFL_TYPE_SUBDEV and vd->device_caps == 0,
__video_register_device() triggers a WARN_ON(!vdev->device_caps) and fails
into err_del_vqs.

Should vd->device_caps and device_type be validated before
virtio_device_ready(), and should err_del_vqs reset the virtio device before
deleting the virtqueues?

> +
> +static void virtio_media_remove(struct virtio_device *virtio_dev)
> +{
> +	struct virtio_media *vv = virtio_dev->priv;
> +	struct list_head *p, *n;
> +
> +	/* Remove the video device to stop accessing the commandq virtqueue */
> +	video_unregister_device(&vv->video_dev);
> +
> +	/*
> +	 * Acquire vlock to ensure no concurrent accesses to the commandq
> +	 * virtqueue
> +	 */
> +	mutex_lock(&vv->vlock);
> +
> +	/* Stop all virtqueue callbacks */
> +	virtio_reset_device(virtio_dev);
> +
> +	/*
> +	 * Only detach the unused buffers to allow internal virtqueue clean up.
> +	 * The actual buffers are freed in virtio_media_release()
> +	 */
> +	while (virtqueue_detach_unused_buf(vv->commandq))
> +		;
> +
> +	/* Delete the virtqueues */
> +	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);
> +	}
> +
> +	v4l2_device_put(&vv->v4l2_dev);
> +}

[Severity: Critical]
What happens in virtio_media_remove() if the virtio device is unbound or
hot-unplugged while userspace still has an open file descriptor (or an
in-flight open/close)?

First, video_unregister_device() only clears V4L2_FL_REGISTERED and unlinks
the device node; it does not close open struct file instances or wait for
in-flight file operations. In virtio_media_remove(), the loop over
vv->sessions (which also iterates without holding vv->sessions_lock) frees
every active session via virtio_media_session_free(), which calls:

  virtio_media_session_fh_del(session)
    v4l2_fh_del(&session->fh, session->file)
      filp->private_data = NULL;
  kfree(session);

and virtio_dev->config->del_vqs(virtio_dev) frees vv->commandq. When
userspace later closes its open file descriptor, v4l2_release() still invokes
virtio_media_device_close():

  virtio_media_device_close(file)
    session = fh_to_session(file->private_data); -> NULL (or freed session)
    virtio_media_session_close(vv, session)
      cmd_close->session_id = session->id; -> NULL dereference / UAF
      virtio_media_send_command(vv, ...) -> uses deleted vv->commandq (UAF)
    virtio_media_session_remove(vv, session);
    virtio_media_session_free(session); -> double free

Similarly, a concurrent virtio_media_device_open() or
virtio_media_device_close() waiting on mutex_lock(&vv->vlock) during
virtio_media_remove() will acquire vv->vlock after del_vqs() has already
destroyed vv->commandq and attempt to submit a command on the freed
virtqueue.

Second, if a command is in flight in virtio_media_kick_command() waiting in
uninterruptible wait_event(vv->wq, cb_param.done) while holding vv->vlock and
the device stops responding during hot-unplug, won't virtio_media_remove()
deadlock forever on mutex_lock(&vv->vlock) before it can reach
virtio_reset_device()?

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