Re: [PATCH] drm/virtio: fix memory leak of fence event on execbuffer failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The return value of virtio_gpu_fence_alloc() is not checked for NULL, leading to a NULL pointer dereference on memory allocation failure.
- [High] Silent command submission failures ignore error returns, bypassing error cleanup and leaking memory, GEM object references, and the fence event.
--

commit 93a0baf42622f4c32d5d66c718e042ee250da557
Author: Peiyang He <peiyang_he@smail.nju.edu.cn>
drm/virtio: fix memory leak of fence event on execbuffer failure

This patch fixes a memory leak in the virtio GPU driver by cancelling
and freeing the DRM event on the execbuffer error path before dropping
the out-fence. It ensures the fence's event pointer is cleared after
cancellation to prevent dangling pointers.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index 32cb1e4aa4258..734b1e976a75c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -538,6 +538,10 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
>  	virtio_gpu_process_post_deps(&submit);
>  	virtio_gpu_complete_submit(&submit);

[Severity: High]
This isn't a bug introduced by this patch, but does this code silently absorb
submission errors and leak resources?

In the current code just above this hunk, we have:

    virtio_gpu_submit(&submit);
    ...
    virtio_gpu_complete_submit(&submit);
cleanup:

If the underlying queue functions fail (e.g., vmalloc_to_sgt() returning
-ENOMEM in virtio_gpu_queue_fenced_ctrl_buffer()), they return an error code:

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_queue_fenced_ctrl_buffer() {
    ...
        sgt = vmalloc_to_sgt(vbuf->data_buf, vbuf->data_size, &sg_ents);
        if (!sgt) {
            if (fence && vbuf->objs)
                virtio_gpu_array_unlock_resv(vbuf->objs);
            return -ENOMEM;
        }
    ...
}

However, the wrapper functions like virtio_gpu_cmd_submit() return void,
dropping this error:

drivers/gpu/drm/virtio/virtgpu_vq.c:virtio_gpu_cmd_submit() {
    ...
    virtio_gpu_queue_fenced_ctrl_buffer(vgdev, vbuf, fence);
}

Because the error is dropped, virtio_gpu_execbuffer_ioctl() proceeds as if
successful, calls virtio_gpu_complete_submit() to clear pointers, and
skips the error cleanup path.

Does this permanently leak GEM object references, memory buffers, and the
fence event when command submission fails under memory pressure?


[Severity: High]
This is another pre-existing issue, but is it possible for
virtio_gpu_fence_alloc() to fail and cause a NULL pointer dereference
earlier in the submission path?

Looking at virtio_gpu_init_submit() called earlier in this ioctl, it does
not check the return value of virtio_gpu_fence_alloc():

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_init_submit() {
    ...
        out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
    else
        out_fence = NULL;

    if (drm_fence_event) {
        err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
    ...
}

And in virtio_gpu_fence_event_create():

drivers/gpu/drm/virtio/virtgpu_submit.c:virtio_gpu_fence_event_create() {
    ...
    fence->e = e;
    ...
}

If kzalloc_obj() inside virtio_gpu_fence_alloc() fails due to memory pressure,
it returns NULL, which is then unconditionally dereferenced.

Can unprivileged userspace trigger this by invoking the execbuffer ioctl
under constrained memory conditions?

>  cleanup:
> +	if (ret && submit.out_fence && submit.out_fence->e) {
> +		drm_event_cancel_free(dev, &submit.out_fence->e->base);
> +		submit.out_fence->e = NULL;
> +	}
>  	virtio_gpu_cleanup_submit(&submit);
>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/D320EAB5680C1411+20260908121323.2405044-1-peiyang_he@smail.nju.edu.cn?part=1