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: Tue, 26 Nov 2024 12:56:10 +0100

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()...