Re: [RFT PATCH] Input: sur40 - fix DMA handling in video capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: use-after-free of sur40 state on disconnect when V4L2 fd is open
- [Medium] usb: invalid GFP flags (0) passed to usb_sg_init in sleepable context
--

commit f65bffa4ada0f7963abf3d4813cdc5924218586d
Author: Karl Mehltretter <kmehltretter@gmail.com>

Input: sur40 - fix DMA handling in video capture

This commit fixes video capture failures by transitioning from vb2_dma_sg to
vb2_vmalloc. It receives each frame into a driver-owned, page-backed
scatterlist and copies it through the vb2 mapping, while synchronizing
CPU writes to imported dma-bufs.

> diff --git a/drivers/input/touchscreen/sur40.c b/drivers/input/touchscreen/sur40.c
> index 09d8c5f8d09fc..72e3f0fa1f4cb 100644
> --- a/drivers/input/touchscreen/sur40.c
> +++ b/drivers/input/touchscreen/sur40.c
[ ... ]
> @@ -579,11 +584,17 @@ static void sur40_process_video(struct sur40_state *sur40)
>  
>  	dev_dbg(sur40->dev, "header acquired\n");
>  
> -	sgt = vb2_dma_sg_plane_desc(&new_buf->vb.vb2_buf, 0);
> +	vaddr = vb2_plane_vaddr(&new_buf->vb.vb2_buf, 0);
> +	if (!vaddr)
> +		goto err_poll;
>  
> +	/*
> +	 * vb2_plane_vaddr() may return a vmalloc or vmap address. Receive
> +	 * into page-backed memory so the USB core can map it for DMA.
> +	 */
>  	result = usb_sg_init(&sgr, sur40->usbdev,
>  		usb_rcvbulkpipe(sur40->usbdev, VIDEO_ENDPOINT), 0,
> -		sgt->sgl, sgt->nents, sur40->pix_fmt.sizeimage, 0);
> +		sur40->video_sgl, sur40->video_nents, size, 0);

[Severity: Medium]
Is it correct to pass 0 as the mem_flags argument to usb_sg_init() here?

Passing 0 implies no reclaim and no wait (equivalent to GFP_NOWAIT), which
restricts usb_alloc_urb() and internal kmalloc operations. This can increase
the probability of spurious -ENOMEM failures under memory pressure.

Since sur40_process_video() executes inside the input poller worker thread
(sur40_poll) which runs in process context and is allowed to sleep (it already
calls usb_bulk_msg() and usb_sg_wait()), shouldn't this use GFP_KERNEL instead?

[ ... ]
> @@ -831,6 +883,7 @@ static void sur40_disconnect(struct usb_interface *interface)
>  	video_unregister_device(&sur40->vdev);
>  	v4l2_device_unregister(&sur40->v4l2);
>  
> +	sgl_free(sur40->video_sgl);
>  	kfree(sur40->bulk_in_buffer);
>  	kfree(sur40);

[Severity: High]
Could freeing the sur40 structure unconditionally here lead to a use-after-free
if userspace holds the V4L2 file descriptor open during device unplug?

When the application later closes the file, the V4L2 core invokes
vb2_fop_release(), which dereferences vdev->queue and attempts to acquire
vdev->queue->lock. Because vdev is embedded in the freed sur40 structure,
this results in a direct use-after-free.

While the commit message acknowledges this bug and defers to a pending patch
series for the disconnect fixes, the current patch adds new cleanup to this
vulnerable path. The referenced series is absent from the provided tree,
leaving the vulnerability exposed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007054430.40155-1-kmehltretter@gmail.com?part=1