[PATCH] drm/virtio: fail init on display-info timeout

[PATCH] drm/virtio: fail init on display-info timeout

[Pengpeng Hou]

virtio_gpu_init() sends GET_DISPLAY_INFO when scanouts are present and
waits for display_info_pending to clear. If the response never arrives,
the wait result is ignored and probe still succeeds.

Return -ETIMEDOUT on display-info timeout. Because this happens after
virtio_device_ready(), reset the device and tear down modesetting before
using the existing vbuf and virtqueue cleanup path.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/gpu/drm/virtio/virtgpu_kms.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index cfde9f573df6..31209bea97ae 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -262,11 +262,19 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 			virtio_gpu_cmd_get_edids(vgdev);
 		virtio_gpu_cmd_get_display_info(vgdev);
 		virtio_gpu_notify(vgdev);
-		wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
-				   5 * HZ);
+		if (!wait_event_timeout(vgdev->resp_wq,
+					!vgdev->display_info_pending,
+					5 * HZ)) {
+			DRM_ERROR("timed out waiting for display info\n");
+			ret = -ETIMEDOUT;
+			goto err_ready;
+		}
 	}
 	return 0;
 
+err_ready:
+	virtio_reset_device(vgdev->vdev);
+	virtio_gpu_modeset_fini(vgdev);
 err_scanouts:
 	virtio_gpu_free_vbufs(vgdev);
 err_vbufs:
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] drm/virtio: fail init on display-info timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Pending virtqueue buffers and dynamically allocated response structures are leaked on the timeout error path, causing a kmem_cache_destroy() warning.
- [High] Missing cancellation of workqueues in the timeout error path leads to use-after-free, NULL pointer dereferences, and data races.
--

commit 81c055cf6da869ee835f7a7f8d32208780ea1dd0
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/virtio: fail init on display-info timeout

virtio_gpu_init() sends GET_DISPLAY_INFO when scanouts are present and
waits for display_info_pending to clear. If the response never arrives,
the wait result is ignored and probe still succeeds.

Return -ETIMEDOUT on display-info timeout. Because this happens after
virtio_device_ready(), reset the device and tear down modesetting before
using the existing vbuf and virtqueue cleanup path.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index cfde9f573df6e..31209bea97ae1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -262,11 +262,19 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
>  			virtio_gpu_cmd_get_edids(vgdev);
>  		virtio_gpu_cmd_get_display_info(vgdev);
>  		virtio_gpu_notify(vgdev);
> -		wait_event_timeout(vgdev->resp_wq, !vgdev->display_info_pending,
> -				   5 * HZ);
> +		if (!wait_event_timeout(vgdev->resp_wq,
> +					!vgdev->display_info_pending,
> +					5 * HZ)) {
> +			DRM_ERROR("timed out waiting for display info\n");
> +			ret = -ETIMEDOUT;
> +			goto err_ready;
> +		}
>  	}
>  	return 0;
>  
> +err_ready:
> +	virtio_reset_device(vgdev->vdev);
> +	virtio_gpu_modeset_fini(vgdev);
>  err_scanouts:
>  	virtio_gpu_free_vbufs(vgdev);

[Severity: High]
Does this error path leak the pending virtqueue buffers and dynamically
allocated response structures?

The driver queues commands via virtio_gpu_cmd_get_display_info() and
virtio_gpu_cmd_get_edids() which allocate vbuf objects from the vgdev->vbufs
cache, as well as resp_buf via kzalloc_obj().

If wait_event_timeout() expires, the error path resets the device and
immediately calls virtio_gpu_free_vbufs(), which destroys the cache. Because
the commands are still in the virtqueue (and never detached/freed), will this
trigger a 'Slab cache still has objects' warning from kmem_cache_destroy() and
permanently leak the dynamically allocated resp_buf pointers?

[Severity: High]
Does the cleanup path miss cancelling the virtqueue workqueues before the
device is destroyed?

Since virtio_device_ready() enables interrupts, if a virtqueue interrupt
fires just as the timeout expires, virtio_gpu_ctrl_ack() could schedule
vgdev->ctrlq.dequeue_work.

This timeout error path resets the device but doesn't cancel the asynchronously
scheduled workqueues.

Since the function returns -ETIMEDOUT, the caller (virtio_gpu_probe()) will
free the vgdev structure via drmm managed release. When the scheduled work
later executes, could it cause a use-after-free when accessing the freed
vgdev, or a NULL pointer dereference when attempting to call
kmem_cache_free(vgdev->vbufs, vbuf) where vgdev->vbufs was set to NULL by the
error path?

>  err_vbufs:
>  	vgdev->vdev->config->del_vqs(vgdev->vdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625030202.82590-1-pengpeng@iscas.ac.cn?part=1