git: f12dd1d5f030 - main - uvideo: lock the mmap queue and read path

The branch main has been updated by bapt:

URL: https://cgit.FreeBSD.org/src/commit/?id=f12dd1d5f0303fe3bc5030293bda04b04995b72c

commit f12dd1d5f0303fe3bc5030293bda04b04995b72c
Author:     Baptiste Daroussin <bapt@FreeBSD.org>
AuthorDate: 2026-07-22 07:26:18 +0000
Commit:     Baptiste Daroussin <bapt@FreeBSD.org>
CommitDate: 2026-07-22 15:10:57 +0000

    uvideo: lock the mmap queue and read path
    
    qbuf(), dqbuf() and read() manipulated sc_mmap_q / sc_mmap_cur /
    sc_frames_ready without sc_mtx, racing with the USB transfer callbacks
    (producer) that run under the mutex.  This could corrupt the queue or
    trigger use-after-free.
    
    Take sc_mtx around qbuf(), use mtx_sleep() and protect the queue
    operations in dqbuf(), and use mtx_sleep() with a snapshot of sc_fsize
    in read().
    
    Also reject S_FMT and S_PARM with EBUSY while streaming: both
    re-negotiate the probe/commit controls with the device, which disrupts
    the active USB transfers (a second client opening the device would
    otherwise freeze the first one's stream).
---
 sys/dev/usb/video/uvideo.c | 72 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 13 deletions(-)

diff --git a/sys/dev/usb/video/uvideo.c b/sys/dev/usb/video/uvideo.c
index 9c6076bc76d0..708660adeafb 100644
--- a/sys/dev/usb/video/uvideo.c
+++ b/sys/dev/usb/video/uvideo.c
@@ -2893,7 +2893,7 @@ uvideo_cdev_read(struct cdev *dev, struct uio *uio, int ioflag)
 {
 	struct uvideo_softc *sc = dev->si_drv1;
 	usb_error_t error;
-	int ret;
+	int ret, fsize;
 
 	if (sc == NULL || sc->sc_dying)
 		return (ENXIO);
@@ -2942,23 +2942,31 @@ uvideo_cdev_read(struct cdev *dev, struct uio *uio, int ioflag)
 		return (EBUSY);
 
 	/* Wait for a frame */
+	mtx_lock(&sc->sc_mtx);
 	while (sc->sc_frames_ready == 0) {
-		if (ioflag & IO_NDELAY)
+		if (ioflag & IO_NDELAY) {
+			mtx_unlock(&sc->sc_mtx);
 			return (EWOULDBLOCK);
-		ret = tsleep(sc, PCATCH, "uvread", hz * 10);
-		if (ret != 0)
+		}
+		ret = mtx_sleep(sc, &sc->sc_mtx, PCATCH, "uvread", hz * 10);
+		if (ret != 0) {
+			mtx_unlock(&sc->sc_mtx);
 			return (ret);
-		if (sc->sc_dying)
+		}
+		if (sc->sc_dying) {
+			mtx_unlock(&sc->sc_mtx);
 			return (ENXIO);
+		}
 	}
 
 	sc->sc_frames_ready--;
+	fsize = sc->sc_fsize;
+	mtx_unlock(&sc->sc_mtx);
 
-	if (sc->sc_fsize == 0)
+	if (fsize == 0)
 		return (0);
 
-	return (uiomove(sc->sc_fbuffer, MIN(uio->uio_resid, sc->sc_fsize),
-	    uio));
+	return (uiomove(sc->sc_fbuffer, MIN(uio->uio_resid, fsize), uio));
 }
 
 static int
@@ -3367,6 +3375,16 @@ uvideo_s_fmt(struct uvideo_softc *sc, struct v4l2_format *fmt)
 	if (fmt->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
 		return (EINVAL);
 
+	/* Reject format changes while streaming: re-negotiating the probe
+	 * and commit controls with the device would disrupt the active USB
+	 * transfers.  V4L2 mandates EBUSY in this case. */
+	mtx_lock(&sc->sc_mtx);
+	if (sc->sc_streaming) {
+		mtx_unlock(&sc->sc_mtx);
+		return (EBUSY);
+	}
+	mtx_unlock(&sc->sc_mtx);
+
 	DPRINTFN(1, "s_fmt: requested %dx%d\n",
 	    fmt->fmt.pix.width, fmt->fmt.pix.height);
 
@@ -3448,6 +3466,15 @@ uvideo_s_parm(struct uvideo_softc *sc, struct v4l2_streamparm *parm)
 {
 	usb_error_t error;
 
+	/* Reject parameter changes while streaming for the same reason as
+	 * S_FMT: they re-negotiate with the device. */
+	mtx_lock(&sc->sc_mtx);
+	if (sc->sc_streaming) {
+		mtx_unlock(&sc->sc_mtx);
+		return (EBUSY);
+	}
+	mtx_unlock(&sc->sc_mtx);
+
 	if (parm->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		if (parm->parm.capture.timeperframe.numerator == 0 ||
 		    parm->parm.capture.timeperframe.denominator == 0)
@@ -3652,8 +3679,11 @@ uvideo_qbuf(struct uvideo_softc *sc, struct v4l2_buffer *qb)
 	    qb->index >= sc->sc_mmap_count)
 		return (EINVAL);
 
+	/* Serialize with the USB transfer callbacks (producer). */
+	mtx_lock(&sc->sc_mtx);
 	sc->sc_mmap[qb->index].v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
 	sc->sc_mmap[qb->index].v4l2_buf.flags |= V4L2_BUF_FLAG_QUEUED;
+	mtx_unlock(&sc->sc_mtx);
 
 	DPRINTFN(2, "buffer %d ready for queueing\n", qb->index);
 
@@ -3670,24 +3700,40 @@ uvideo_dqbuf(struct uvideo_softc *sc, struct v4l2_buffer *dqb)
 	    dqb->memory != V4L2_MEMORY_MMAP)
 		return (EINVAL);
 
-	if (STAILQ_EMPTY(&sc->sc_mmap_q)) {
-		error = tsleep(sc, PCATCH, "uvdqbuf", hz * 10);
-		if (error)
+	/*
+	 * Serialize with the USB transfer callbacks (producer) that insert
+	 * completed buffers into sc_mmap_q under sc_mtx.  Use mtx_sleep so
+	 * the wait and the queue inspection are atomic.
+	 */
+	mtx_lock(&sc->sc_mtx);
+	while (STAILQ_EMPTY(&sc->sc_mmap_q)) {
+		error = mtx_sleep(sc, &sc->sc_mtx, PCATCH, "uvdqbuf", hz * 10);
+		if (error != 0) {
+			mtx_unlock(&sc->sc_mtx);
 			return (EINVAL);
+		}
+		if (sc->sc_dying) {
+			mtx_unlock(&sc->sc_mtx);
+			return (ENXIO);
+		}
 	}
 
 	mmap = STAILQ_FIRST(&sc->sc_mmap_q);
-	if (mmap == NULL)
+	if (mmap == NULL) {
+		mtx_unlock(&sc->sc_mtx);
 		return (EINVAL);
+	}
 
 	bcopy(&mmap->v4l2_buf, dqb, sizeof(struct v4l2_buffer));
 
 	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
 	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
 
+	STAILQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
+	mtx_unlock(&sc->sc_mtx);
+
 	DPRINTFN(2, "frame dequeued from index %d\n",
 	    mmap->v4l2_buf.index);
-	STAILQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
 
 	return (0);
 }