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 <[email protected]> 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/[email protected]?part=1
