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