Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider: - [Critical] virtio: stack use-after-free in virtio_media_kick_command() on timeout - [High] virtio: use-after-free of video_device on file operations after unbind - [High] virtio: null pointer dereference in virtio_media_device_close() after error event - [High] virtio: memory leak and UAF on session close failure - [High] virtio: workqueue use-after-free due to wrong teardown order - [High] virtio: data race and DMA corruption on command timeout - [High] virtio: virtqueue use-after-free from concurrent file operations during remove - [High] virtio: ABI mismatch due to architecture-dependent v4l2_buffer in virtio payload - [Medium] virtio: kick and device registration before DRIVER_OK - [Medium] virtio: host session resource leak on local allocation failure - [Medium] virtio: list corruption via duplicate DQBUF events - [Medium] virtio: DMA mapping leak from pending eventq buffers --
commit 14a36656b931432d8f2ab367b1f02ad0cec5fe5f Author: Alexandre Courbot <[email protected]> media: virtio: Add session management This patch adds session management to the virtio-media driver. A session is created when the /dev/videoX device is opened, and destroyed when it is closed. > diff --git a/drivers/media/virtio/virtio_media_driver.c > b/drivers/media/virtio/virtio_media_driver.c > index 25f2ceaa6c480..ef8065e344929 100644 > --- a/drivers/media/virtio/virtio_media_driver.c > +++ b/drivers/media/virtio/virtio_media_driver.c [ ... ] > +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; [Severity: High] When virtio_media_process_events() receives VIRTIO_MEDIA_EVT_ERROR, it calls virtio_media_session_close() which frees the session and sets the file's private_data to NULL. If userspace later closes the file, virtio_media_device_close() will pass the resulting NULL session pointer here. Could this dereference a null pointer if the session was already closed due to an error event? > + cmd_close->session_id = session->id; > + > + sg_set_buf(&cmd_sg, cmd_close, sizeof(*cmd_close)); > + sg_mark_end(&cmd_sg); > + > + ret = virtio_media_send_command(vv, sgs, 1, 0, 0, NULL); > + mutex_unlock(&vv->vlock); > + if (ret < 0) > + return ret; [Severity: High] If virtio_media_send_command() fails (for example due to a timeout), this error path returns early without calling virtio_media_session_free(). The VFS will still destroy the file structure, leaving a dangling pointer in the un-freed session object that will be accessed during driver removal when it iterates over vv->sessions. Does this leak the session memory and cause a use-after-free later? > + > + virtio_media_session_free(vv, session); > + > + return 0; > +} [ ... ] > +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->done = true; > + param->resp_len = len; [Severity: Critical] If wait_event_timeout() in virtio_media_kick_command() expires, the function returns and the stack frame containing cb_param is popped. When the host eventually completes the command, the callback will overwrite whatever is now at that stack location. Can this write to stale stack memory? > + wake_up(¶m->vv->wq); > + } [ ... ] > +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, > + }; [ ... ] > + /* Wait for the response. */ > + ret = wait_event_timeout(vv->wq, cb_param.done, 5 * HZ); > + if (ret == 0) { > + v4l2_err(&vv->v4l2_dev, > + "timed out waiting for response to command\n"); > + return -ETIMEDOUT; [Severity: Critical] Because cb_param is allocated on the stack, returning here destroys the stack frame. If the host completes the command later, commandq_callback() will write to the param->done and param->resp_len fields, which now point to popped stack memory. Is it safe to return here without aborting the command on the host? > + } [ ... ] > +static void > +virtio_media_process_dqbuf_event(struct virtio_media *vv, > + struct virtio_media_session *session, > + struct virtio_media_event_dqbuf *dqbuf_evt) [Severity: High] The virtio-media protocol uses struct v4l2_buffer within its event payloads. This structure contains a struct timeval, whose size varies between 32-bit and 64-bit architectures. A 32-bit guest running on a 64-bit host will encounter misaligned fields following the timestamp, breaking the virtio ABI. Does casting the event payload to struct v4l2_buffer create an ABI mismatch? > +{ > + struct virtio_media_buffer *dqbuf; > + const enum v4l2_buf_type queue_type = dqbuf_evt->buffer.type; [ ... ] > + /* Set the DONE flag as the buffer is waiting to be dequeued. */ > + dqbuf->buffer.flags |= V4L2_BUF_FLAG_DONE; > + > + mutex_lock(&session->queues_lock); > + list_add_tail(&dqbuf->list, &queue->pending_dqbufs); [Severity: Medium] Since the list_add_tail() is unconditional, receiving a duplicate event will link the node while it is already linked. What happens if the host sends duplicate DQBUF events for the same buffer index? > + queue->queued_bufs -= 1; > + mutex_unlock(&session->queues_lock); [ ... ] > +static int virtio_media_device_open(struct file *file) > +{ > + struct video_device *video_dev = video_devdata(file); > + struct virtio_media *vv = to_virtio_media(video_dev); > + struct virtio_media_cmd_open *cmd_open = &vv->cmd.open; > + struct virtio_media_resp_open *resp_open = &vv->resp.open; [Severity: High] The command and response buffers are shared in the virtio_media structure. If a command times out, virtio_media_send_command() drops the lock and returns. A subsequent open() will acquire the lock and overwrite these buffers while the host might concurrently read or write them for the timed-out command, corrupting DMA memory. Are these shared device-level buffers protected from concurrent modification on timeout? > + struct scatterlist cmd_sg = {}, resp_sg = {}; [ ... ] > + ret = virtio_media_send_command(vv, sgs, 1, 1, sizeof(*resp_open), > + NULL); > + session_id = resp_open->session_id; > + mutex_unlock(&vv->vlock); > + if (ret < 0) > + return ret; > + > + session = virtio_media_session_alloc(vv, session_id, file); > + if (IS_ERR(session)) > + return PTR_ERR(session); [Severity: Medium] If virtio_media_session_alloc() fails, the driver returns an error to VFS, but the VIRTIO_MEDIA_CMD_OPEN command has already been successfully executed on the host. Because no CLOSE command is sent in the error path, this leaks the session resources on the virtio host device. Will this leave a dangling session on the host on allocation failure? > + > + file->private_data = &session->fh; > + > + return 0; > +} [ ... ] > +static int virtio_media_probe(struct virtio_device *virtio_dev) > +{ > + struct device *dev = &virtio_dev->dev; > + struct virtqueue *vqs[2]; > + static struct virtqueue_info vq_info[2] = { > + { > + .name = "command", > + .callback = commandq_callback, > + }, > + { > + .name = "event", > + .callback = eventq_callback, > + }, > + }; > + struct virtio_media *vv; > + struct video_device *vd; > + int i; > + int ret; > + > + vv = devm_kzalloc(dev, sizeof(*vv), GFP_KERNEL); [Severity: High] By allocating vv with devres and using video_device_release_empty, the memory is freed immediately upon device unbind. If userspace has an open file descriptor, later file operations or closing the file will trigger VFS callbacks that access the freed video_device embedded in vv. Does this break the expected V4L2 refcounting mechanism? > + if (!vv) > + return -ENOMEM; [ ... ] > + 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; [ ... ] > + ret = video_register_device(vd, virtio_cread32(virtio_dev, 4), 0); > + if (ret) > + goto err_register_device; > + > + for (i = 0; i < VIRTIO_MEDIA_NUM_EVENT_BUFS; i++) { > + void *ebuf = vv->event_buffer + > + virtio_media_event_aligned_size * i; > + > + ret = virtio_media_send_event_buffer(vv, ebuf); > + if (ret) > + goto err_send_event_buffer; > + } > + > + virtio_device_ready(virtio_dev); [Severity: Medium] virtio_media_send_event_buffer() kicks the event virtqueue, and video_register_device() makes the device visible to userspace, both before virtio_device_ready() sets DRIVER_OK. The Virtio specification forbids sending buffer available notifications before DRIVER_OK. Is it permitted to expose the device to userspace and kick virtqueues before setting DRIVER_OK? > + > + return 0; [ ... ] > +static void virtio_media_remove(struct virtio_device *virtio_dev) > +{ > + struct virtio_media *vv = virtio_dev->priv; > + struct list_head *p, *n; > + > + cancel_work_sync(&vv->eventq_work); > + virtio_reset_device(virtio_dev); [Severity: High] Because cancel_work_sync() is called before virtio_reset_device(), the device is still active and can generate event interrupts. If an interrupt fires, eventq_callback() will schedule the work again. When the driver subsequently frees vv, the pending work will execute and access freed memory. Can the workqueue be rescheduled after cancellation? > + v4l2_device_unregister(&vv->v4l2_dev); > + virtio_dev->config->del_vqs(virtio_dev); [Severity: High] del_vqs() is called here without acquiring vv->vlock or waiting for concurrent file operations to finish. A concurrent ioctl or close operation could execute virtio_media_send_command() and attempt to add descriptors to the deleted vv->commandq. Can concurrent file operations access the virtqueues after they are deleted? [Severity: Medium] During probe, buffers are mapped and added to the event queue. Calling del_vqs() without first calling virtqueue_detach_unused_buf() leaves these pending buffers unretrieved, leaking the DMA mappings and consuming IOMMU space. Does deleting the virtqueues here leak DMA mappings? > + video_unregister_device(&vv->video_dev); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
