From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: sys/uvideo: add missed lock in uvideo_vs_start_isoc_ixfer
To: tech@openbsd.org, mglocker@openbsd.org
Date: Tue, 26 Nov 2024 13:04:20 +0100

On Tue, 26 Nov 2024 12:56:10 +0100,
Martin Pieuchot <mpi@grenadille.net> wrote:
> 
> On 26/11/24(Tue) 12:35, Kirill A. Korinsky wrote:
> > On Tue, 26 Nov 2024 11:21:14 +0100,
> > Martin Pieuchot <mpi@grenadille.net> wrote:
> > > 
> > > On 25/11/24(Mon) 20:58, Kirill A. Korinsky wrote:
> > > > 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.
> > > 
> > > What do you mean by locks?
> > > 
> > > > When close had happened, it may lead to crash if non-bulk decided to
> > > > re-setup usbd_transfer in parallel thread.
> > > 
> > > I don't understand what you say.  To me the issue seem to come from the
> > > fact that uvideo_vs_close() is called twice, possibly because the first
> > > one sleeps.
> > > 
> > > > Here is the thread https://marc.info/?t=173228582400007&r=1&w=2 where I
> > > > encountered this crash, and this patch fixes it.
> > > 
> > > The diff below prevent the crash by clearing pointers before going to
> > > sleep.  Which means the "second" close won't trigger a user-after-free.
> > > 
> > > I'm not sure the order of operations below is correct.  usbd_ref_wait()
> > > is possibly where the thread being killed is waiting.  You can confirm
> > > that by killing ffplay and looking on which channel it hangs.
> > > 
> > > Then comes the question, why does the bulk thread doesn't stop?  Is it
> > > stuck?  Where is it sleeping?
> > >
> > 
> > Seems my wording wasn't good. I mean that I use usbd_ref_wait as lock to
> > block the second call of close.
> 
> I don't understand what you mean.
> 
> > I also was wrong that it goes to non-bulk code path, and you're right. The
> > code is blocked at usbd_transfer inside uvideo_vs_start_bulk_thread.
> 
> That's interesting.  So the bulk thread is waiting for the transfer to
> timeout before it can check `bulk_running' and finally decrement the
> refcounter?  Is it so?
> 
> In that case, does calling usbd_abort_pipe() before usbd_ref_wait()
> help?
> 
> > When I add a very short timeout (1s), I can't crash system because anymore,
> > but for longer timeout (10s) I still can do it.
> 
> Are you talking about the usbd_ref_wait() timeout?  If so, that confirms
> the double-free due to a double close.
> 
> I wonder if we should not have a mechanism a layer above to prevent
> double close()...
> 
> 

I had reverted all my changes and tested with 1000 (1s) and 10000 (10s):

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	26 Nov 2024 10:50:08 -0000
@@ -2020,9 +2020,11 @@ uvideo_vs_start_bulk_thread(void *arg)
 		    sc->sc_vs_cur->bxfer.buf,
 		    size,
 		    USBD_NO_COPY | USBD_SHORT_XFER_OK | USBD_SYNCHRONOUS,
-		    0,
+		    1000,
 		    NULL);
 		error = usbd_transfer(sc->sc_vs_cur->bxfer.xfer);
+		if (error == USBD_TIMEOUT)
+			continue;
 		if (error != USBD_NORMAL_COMPLETION) {
 			DPRINTF(1, "%s: error in bulk xfer: %s!\n",
 			    DEVNAME(sc), usbd_errstr(error));


-- 
wbr, Kirill