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 <[email protected]> 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...@smail.nju.edu.cn?part=1
