From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: sys/uvideo: add missed lock in uvideo_vs_start_isoc_ixfer
To: tech@openbsd.org, mglocker@openbsd.org
Date: Wed, 27 Nov 2024 10:24:25 +0100

On 26/11/24(Tue) 15:02, Kirill A. Korinsky wrote:
> On Tue, 26 Nov 2024 13:04:20 +0100,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > 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):
> > 
> 
> This seems quite better solution: to abort pipe when it is blocked inside
> usbd_transfer.

This is even required because the transfers cannot timeout.

ok mpi@

> 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 13:40:00 -0000
> @@ -1935,6 +1935,11 @@ uvideo_vs_close(struct uvideo_softc *sc)
>  {
>  	if (sc->sc_vs_cur->bulk_running == 1) {
>  		sc->sc_vs_cur->bulk_running = 0;
> +
> +		/* Bulk thread may sleep in usbd_transfer, abort it */
> +		if (sc->sc_vs_cur->pipeh)
> +			usbd_abort_pipe(sc->sc_vs_cur->pipeh);
> +
>  		usbd_ref_wait(sc->sc_udev);
>  	}
>  
> 
> -- 
> wbr, Kirill
>