Re: [PATCH v9 1/4] media: virtio: Add skeleton virtio-media driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] virtio-media: use-after-free when device is unbound with open file handles
- [High] virtio-media: architecture-dependent types in UAPI protocol break cross-arch ABI
- [High] virtio-media: teardown order inversion destroys virtqueues before unregistering device
- [High] virtio-media: untrusted hardware config mapped directly to internal kernel enum
- [High] virtio-media: use of errno as struct member in UAPI header breaks userspace compilation
- [Medium] virtio-media: missing 64-bit alignment padding in struct virtio_media_cmd_mmap
- [Medium] virtio-media: video device exposed to userspace before virtio DRIVER_OK
--

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

media: virtio: Add skeleton virtio-media driver

This commit adds a skeleton driver for virtualizing media devices using
the V4L2 protocol over virtio. It defines the UAPI protocol headers,
internal driver structures, and basic device probing and removal hooks.

> diff --git a/drivers/media/virtio/virtio_media_driver.c b/drivers/media/virtio/virtio_media_driver.c
> index 0000000000000..25f2ceaa6c480
> --- /dev/null
> +++ b/drivers/media/virtio/virtio_media_driver.c
[ ... ]
> +static int virtio_media_probe(struct virtio_device *virtio_dev)
> +{
[ ... ]
> +	vd = &vv->video_dev;
> +	vd->v4l2_dev = &vv->v4l2_dev;
> +	vd->vfl_type = VFL_TYPE_VIDEO;
> +	vd->fops = &virtio_media_fops;
> +	vd->release = video_device_release_empty;
> +	strscpy(vd->name, "virtio-media", sizeof(vd->name));
> +
> +	video_set_drvdata(vd, vv);
> +
> +	vd->device_caps = virtio_cread32(virtio_dev, 0);
> +	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;
> +
> +	ret = video_register_device(vd, virtio_cread32(virtio_dev, 4), 0);

[Severity: High]
Is it safe to pass the hardware config value directly into an internal
kernel enum?

video_register_device() expects an enum vfl_devnode_type as its second
argument. Using virtio_cread32(virtio_dev, 4) directly leaks the untrusted
hardware config space value into an unstable Linux-internal enum. If offset 4
was intended to be the node number (nr), the arguments are swapped.

> +	if (ret)
> +		goto err_register_device;
> +
> +	virtio_device_ready(virtio_dev);

[Severity: Medium]
Could exposing the video device before signaling DRIVER_OK violate the
virtio specification?

video_register_device() exposes the /dev/videoX node to userspace.
A userspace process can open the device and issue commands (which kicks the
virtqueues) before the virtio driver has signaled the DRIVER_OK status, which
might cause the host to ignore commands or fault the guest.

> +	return 0;
> +
> +err_register_device:
> +	virtio_dev->config->del_vqs(virtio_dev);
> +err_find_vqs:
> +	v4l2_device_unregister(&vv->v4l2_dev);
> +	return ret;
> +}
> +
> +static void virtio_media_remove(struct virtio_device *virtio_dev)
> +{
> +	struct virtio_media *vv = virtio_dev->priv;
> +
> +	virtio_reset_device(virtio_dev);
> +	v4l2_device_unregister(&vv->v4l2_dev);
> +	virtio_dev->config->del_vqs(virtio_dev);
> +	video_unregister_device(&vv->video_dev);

[Severity: High]
Does this cleanup order risk a use-after-free or null pointer dereference?

Destroying the virtqueues via del_vqs() before calling
video_unregister_device() leaves a window where a concurrent userspace
process can open the device or issue an ioctl, attempting to access the
already freed virtqueues.

[Severity: High]
Can this lead to a use-after-free when the device is unbound?

virtio_media_probe() allocates vv using devm_kzalloc() and sets
vd->release to video_device_release_empty.

During removal, virtio_media_remove() unregisters the device and returns,
allowing devres to immediately free vv. If a userspace process still has
the /dev/videoX node open, a subsequent ioctl or close will access the freed
video_device embedded in vv. The V4L2 subsystem requires reference-counted
memory management via the release callback for dynamically allocated
devices.

> diff --git a/include/uapi/linux/virtio_media.h b/include/uapi/linux/virtio_media.h
> index 0000000000000..ac3d75c752d92
> --- /dev/null
> +++ b/include/uapi/linux/virtio_media.h
[ ... ]
> +struct virtio_media_cmd_mmap {
> +	struct virtio_media_cmd_header hdr;
> +	__le32 session_id;
> +	__le32 flags;
> +	__le32 offset;
> +};

[Severity: Medium]
Is this structure missing a 4-byte padding field to align to 64 bits?

It consists of an 8-byte header followed by three 4-byte fields, totaling 20
bytes. The compiler will not implicitly pad it to 24 bytes, which contradicts
the design of ensuring 64-bit alignment for virtio-media protocol structures.

[ ... ]
> +struct virtio_media_event_error {
> +	struct virtio_media_event_header hdr;
> +	__le32 errno;

[Severity: High]
Will this break userspace compilation?

In POSIX environments, errno is usually defined as a macro in <errno.h>.
Using it as a struct member name in a UAPI header will cause the preprocessor
to expand it into an invalid identifier for programs including both headers.

> +	__le32 __reserved;
> +};
[ ... ]
> +struct virtio_media_event_dqbuf {
> +	struct virtio_media_event_header hdr;
> +	struct v4l2_buffer buffer;
> +	struct v4l2_plane planes[VIRTIO_MEDIA_MAX_PLANES];
> +};
> +
> +/**
> + * struct virtio_media_event_event - V4L2 event.
> + * @hdr: header for the event.
> + * @event: description of the event that occurred.
> + *
> + * This event signals that a V4L2 event has been emitted for a session.
> + */
> +struct virtio_media_event_event {
> +	struct virtio_media_event_header hdr;
> +	struct v4l2_event event;
> +};

[Severity: High]
Are cross-architecture ABI guarantees broken by these embedded structures?

struct v4l2_buffer and struct v4l2_event contain pointers and
architecture-dependent types (like struct timespec and unsigned long userptr).

The sizes and layouts of these structures will differ between 32-bit and
64-bit platforms, meaning a 64-bit host cannot safely interpret messages from
a 32-bit guest.

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