Re: [PATCH] drm/virtio: fix NULL pointer dereference on fence allocation failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Erroneous release of file descriptor 0 on early error returns due to 0-initialized out_fence_fd.
- [High] Leak of DRM event memory and event space quota on early submission failure.
- [High] Premature reset of in_syncobjs on error paths breaks ioctl restart and destroys dependencies.
--

commit 1f57c5e182bb2310a6b5538ff7f993fd7e45dee5
Author: Peiyang He <peiyang_he@smail.nju.edu.cn>
drm/virtio: fix NULL pointer dereference on fence allocation failure

This commit adds a missing NULL check after allocating a fence in
virtio_gpu_init_submit() to prevent a NULL pointer dereference under
memory pressure.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c b/drivers/gpu/drm/virtio/virtgpu_submit.c
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -389,10 +389,13 @@ static int virtio_gpu_init_submit(struct virtio_gpu_submit *submit,
>  	if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
>  	    exbuf->num_out_syncobjs ||
>  	    exbuf->num_bo_handles ||
> -	    drm_fence_event)
> +	    drm_fence_event) {
>  		out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> -	else
> +		if (!out_fence)
> +			return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but returning early here
exposes a pre-existing issue with file descriptor handling.

Because virtio_gpu_init_submit() initializes the submit struct using
memset(submit, 0, sizeof(*submit)), submit->out_fence_fd is left as 0.
If we return -ENOMEM here, the caller eventually invokes
virtio_gpu_cleanup_submit(), which checks:

    if (submit->out_fence_fd >= 0)
        put_unused_fd(submit->out_fence_fd);

Since 0 is >= 0, this will erroneously release file descriptor 0
(typically stdin) back to the kernel, potentially allowing subsequent
open() calls to hijack the file descriptor. Could out_fence_fd be
initialized to -1 before taking this early return?

[Severity: High]
This is a pre-existing issue, but does taking an error path (such as
this new -ENOMEM return) break system call restarts?

When the submission ioctl fails, the error path calls
virtio_gpu_cleanup_submit(). This function unconditionally calls
virtio_gpu_reset_syncobjs(), replacing all input syncobj fences with
NULL. When the kernel automatically restarts the ioctl (e.g. after
-ERESTARTSYS is returned during a signal interruption), the dependencies
are already destroyed, causing drm_syncobj_find_fence() to return
-EINVAL and breaking userspace synchronization state.

> +	} else {
>  		out_fence = NULL;
> +	}
>  
>  	if (drm_fence_event) {
>  		err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);

[Severity: High]
This is a pre-existing issue, but how does the code handle event cleanup
if an error occurs later in the submission process?

If virtio_gpu_fence_event_create() succeeds, it allocates the event and
deducts from file_priv->event_space. If a subsequent initialization step
fails, virtio_gpu_cleanup_submit() drops the fence reference via
dma_fence_put(&submit->out_fence->f).

However, because virtio_gpu_fence_ops lacks a .release callback,
dma_fence_release() simply frees the fence without ever calling
drm_event_cancel_free(), leaking both the event memory and the client's
event space quota.

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