[PATCH] drm/virtio: fix vbuf->objs double-free, fence NULL deref, and sgt error path

[PATCH] drm/virtio: fix vbuf->objs double-free, fence NULL deref, and sgt error path

[Hui Peng]

Fix three error-handling bugs in drivers/gpu/drm/virtio/:

1. In virtio_gpu_dequeue_ctrl_func() and vbuf cleanup (virtgpu_vq.c),
   clear entry->objs after freeing so objects are not double-released,
   and fix the vmalloc_to_sgt() error path.
2. In virtio_gpu_cleanup_submit() (virtgpu_submit.c), cancel and free
   out_fence->e when submit fails before drm_event_reserve_init()
   completes.

Fixes: dc5698e80cf7 ("Add virtio gpu driver.")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
index 32cb1e4aa425..84aeea58c700 100644
--- a/drivers/gpu/drm/virtio/virtgpu_submit.c
+++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
@@ -343,8 +343,14 @@ static void virtio_gpu_cleanup_submit(struct virtio_gpu_submit *submit)
 	if (submit->out_fence_fd >= 0)
 		put_unused_fd(submit->out_fence_fd);
 
-	if (submit->out_fence)
+	if (submit->out_fence) {
+		if (submit->out_fence->e && !submit->out_fence->f.seqno) {
+			drm_event_cancel_free(submit->vgdev->ddev,
+					      &submit->out_fence->e->base);
+			submit->out_fence->e = NULL;
+		}
 		dma_fence_put(&submit->out_fence->f);
+	}
 
 	if (submit->sync_file)
 		fput(submit->sync_file->file);
@@ -415,6 +421,9 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
 	if (err)
 		return err;
 
+	if (!exbuf->size)
+		return -EINVAL;
+
 	submit->buf = vmemdup_user(u64_to_user_ptr(exbuf->command), exbuf->size);
 	if (IS_ERR(submit->buf))
 		return PTR_ERR(submit->buf);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index c02c03c10d92..115156a680bc 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -202,6 +202,10 @@ static void *virtio_gpu_alloc_cmd_cb(struct virtio_gpu_device *vgdev,
 static void free_vbuf(struct virtio_gpu_device *vgdev,
 		      struct virtio_gpu_vbuffer *vbuf)
 {
+	if (vbuf->objs) {
+		virtio_gpu_array_put_free_delayed(vgdev, vbuf->objs);
+		vbuf->objs = NULL;
+	}
 	if (vbuf->resp_size > MAX_INLINE_RESP_SIZE)
 		kfree(vbuf->resp_buf);
 	kvfree(vbuf->data_buf);
@@ -282,8 +286,6 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
 	wake_up(&vgdev->ctrlq.ack_queue);
 
 	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
-		if (entry->objs)
-			virtio_gpu_array_put_free_delayed(vgdev, entry->objs);
 		list_del(&entry->list);
 		free_vbuf(vgdev, entry);
 	}
@@ -517,6 +519,7 @@ static int virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device *vgdev,
 			if (!sgt) {
 				if (fence && vbuf->objs)
 					virtio_gpu_array_unlock_resv(vbuf->objs);
+				free_vbuf(vgdev, vbuf);
 				return -ENOMEM;
 			}
 
@@ -1409,8 +1412,7 @@ virtio_gpu_cmd_resource_assign_uuid(struct virtio_gpu_device *vgdev,
 	cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
 
 	vbuf->objs = objs;
-	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
-	return 0;
+	return virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
 }
 
 static void virtio_gpu_cmd_resource_map_cb(struct virtio_gpu_device *vgdev,

Re: [PATCH] drm/virtio: fix vbuf->objs double-free, fence NULL deref, and sgt error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/virtio: double-free of vbuf->objs in virtio_gpu_reclaim_vbufs
--

commit 94995582490827616358d8c115ec5215d43b829f
Author: Hui Peng <benquike@gmail.com>
Subject: drm/virtio: fix vbuf->objs double-free, fence NULL deref, and sgt error path

This patch modifies virtio_gpu_dequeue_ctrl_func() and free_vbuf() to
consolidate vbuf->objs cleanup inside free_vbuf(). It also handles fence
cancellation and checks for exbuf->size in submit initialization.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -202,6 +202,10 @@ static void *virtio_gpu_alloc_cmd_cb(struct virtio_gpu_device *vgdev,
>  static void free_vbuf(struct virtio_gpu_device *vgdev,
>  		      struct virtio_gpu_vbuffer *vbuf)
>  {
> +	if (vbuf->objs) {
> +		virtio_gpu_array_put_free_delayed(vgdev, vbuf->objs);
> +		vbuf->objs = NULL;
> +	}

[Severity: High]
Does this change introduce a double-free when called from
virtio_gpu_reclaim_vbufs()?

Looking at virtio_gpu_reclaim_vbufs() in the same file, the loop manually
frees the objects but does not clear the pointer before calling the newly
updated free_vbuf():

	while ((vbuf = virtqueue_detach_unused_buf(vgdev->ctrlq.vq))) {
		if (vbuf->objs)
			virtio_gpu_array_put_free(vbuf->objs);
		if (vbuf->resp_cb_data)
			virtio_gpu_cleanup_object(vbuf->resp_cb_data);
		free_vbuf(vgdev, vbuf);
	}

Since free_vbuf() now also attempts to free vbuf->objs via
virtio_gpu_array_put_free_delayed(), and the pointer is not cleared in
virtio_gpu_reclaim_vbufs(), won't this trigger a double-free on device
teardown when there are unused control queue buffers with attached objects?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919223440.3884176-1-benquike@gmail.com?part=1