From: Kirill A. Korinsky <kirill@korins.ky>
Subject: sys/uvideo: add missed lock in uvideo_vs_start_isoc_ixfer
To: OpenBSD tech <tech@openbsd.org>
Cc: Marcus Glocker <mglocker@openbsd.org>
Date: Mon, 25 Nov 2024 20:58:33 +0100

tech@,

uvideo supports two modes of webcam reading, bulk and non-bulk. In bulk
mode it wraps by locks usbd_transfer to prevent call usbd_close_pipe on
used pipe, and in case of non-bulk, no lock here.

When close had happened, it may lead to crash if non-bulk decided to
re-setup usbd_transfer in parallel thread.

Here is the thread https://marc.info/?t=173228582400007&r=1&w=2 where I
encountered this crash, and this patch fixes it.

Feedback? Ok?

Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -r1.222 uvideo.c
--- sys/dev/usb/uvideo.c	1 Sep 2024 03:09:00 -0000	1.222
+++ sys/dev/usb/uvideo.c	25 Nov 2024 19:13:11 -0000
@@ -1933,15 +1933,18 @@ uvideo_vs_open(struct uvideo_softc *sc)
 void
 uvideo_vs_close(struct uvideo_softc *sc)
 {
-	if (sc->sc_vs_cur->bulk_running == 1) {
+	struct usbd_pipe	*pipeh;
+
+	if (sc->sc_vs_cur->bulk_running == 1)
 		sc->sc_vs_cur->bulk_running = 0;
-		usbd_ref_wait(sc->sc_udev);
-	}
 
-	if (sc->sc_vs_cur->pipeh) {
-		usbd_close_pipe(sc->sc_vs_cur->pipeh);
-		sc->sc_vs_cur->pipeh = NULL;
-	}
+	pipeh = sc->sc_vs_cur->pipeh;
+	sc->sc_vs_cur->pipeh = NULL;
+
+	usbd_ref_wait(sc->sc_udev);
+
+	if (pipeh)
+		usbd_close_pipe(pipeh);
 
 	/*
 	 * Some devices need time to shutdown before we switch back to
@@ -2063,6 +2066,11 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
 	for (i = 0; i < sc->sc_nframes; i++)
 		ixfer->size[i] = sc->sc_vs_cur->psize;
 
+	usbd_ref_incr(sc->sc_udev);
+
+	if (sc->sc_vs_cur->pipeh == NULL)
+		goto skip;
+
 	usbd_setup_isoc_xfer(
 	    ixfer->xfer,
 	    sc->sc_vs_cur->pipeh,
@@ -2077,6 +2085,9 @@ uvideo_vs_start_isoc_ixfer(struct uvideo
 		DPRINTF(1, "%s: usbd_transfer error=%s!\n",
 		    DEVNAME(sc), usbd_errstr(error));
 	}
+
+skip:
+	usbd_ref_decr(sc->sc_udev);
 }
 
 void


-- 
wbr, Kirill