Re: [PATCH] drm/virtio: check virtio_gpu_array_lock_resv() return in cursor update

[Dmitry Osipenko]

Hello,

On 5/10/26 08:30, Deepanshu Kartikey wrote:
> virtio_gpu_cursor_plane_update() calls virtio_gpu_array_lock_resv()
> but ignores its return value. The function can fail in two ways:
> 
>   - dma_resv_lock_interruptible() returns -ERESTARTSYS when a signal
>     is delivered while waiting for the reservation lock.
>   - dma_resv_reserve_fences() returns -ENOMEM if it fails to allocate
>     a fence slot; in this case lock_resv unlocks before returning.
> 
> In both cases the resv lock is not held on return. The cursor path
> proceeds to queue a fenced transfer command. The queue path then
> walks the object array and calls dma_resv_add_fence() on the cursor
> BO's reservation. dma_resv_add_fence() requires the resv lock to be
> held; with lockdep enabled the missing lock trips
> dma_resv_assert_held():
> 
>   WARNING: drivers/dma-buf/dma-resv.c:296 at dma_resv_add_fence+0x71e/0x840
>   Call Trace:
>    virtio_gpu_array_add_fence+0xcd/0x140
>    virtio_gpu_queue_ctrl_sgs
>    virtio_gpu_queue_fenced_ctrl_buffer+0x578/0xfb0
>    virtio_gpu_cursor_plane_update+0x411/0xbc0
>    drm_atomic_helper_commit_planes+0x497/0xf10
>    ...
>    drm_mode_cursor_ioctl+0xd4/0x110
>    drm_ioctl+0x5e6/0xc60
>    __x64_sys_ioctl+0x18e/0x210
> 
> Beyond the WARN, mutating the dma_resv fence list without the lock
> races with concurrent readers/writers and can corrupt the list.
> 
> Check the return value of virtio_gpu_array_lock_resv(). On failure,
> drop the references taken by virtio_gpu_array_add_obj() with
> virtio_gpu_array_put_free() (which does not unlock, matching the
> not-locked state) and return without queueing the command. A
> skipped cursor frame is harmless; the WARN and the underlying race
> are not.
> 
> The bug was reported by syzbot, triggered via fault injection
> (fail_nth) on the DRM_IOCTL_MODE_CURSOR path, which forces the
> -ENOMEM branch in dma_resv_reserve_fences().
> 
> Reported-by: syzbot+72bd3dd3a5d5f39a0271@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=72bd3dd3a5d5f39a0271
> Fixes: 5cfd31c5b3a3 ("drm/virtio: fix virtio_gpu_cursor_plane_update().")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> ---
>  drivers/gpu/drm/virtio/virtgpu_plane.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index a126d1b25f46..ca379b08b9ec 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -459,7 +459,10 @@ static void virtio_gpu_cursor_plane_update(struct drm_plane *plane,
>  		if (!objs)
>  			return;
>  		virtio_gpu_array_add_obj(objs, vgfb->base.obj[0]);
> -		virtio_gpu_array_lock_resv(objs);
> +		if (virtio_gpu_array_lock_resv(objs)) {
> +			virtio_gpu_array_put_free(objs);
> +			return;
> +		}
>  		virtio_gpu_cmd_transfer_to_host_2d
>  			(vgdev, 0,
>  			 plane->state->crtc_w,

Thanks for the patch. Atomic update shouldn't fail due to non-critical
errors like on a signal interrupt. Could you please move this code that
may fail in update() to .prepare/cleanup_fb() callbacks?

-- 
Best regards,
Dmitry