Re: [v2] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait

[John Keeping <john@metanate.com>]

On Mon, Nov 21, 2022 at 09:52:43AM +0530, Udipto Goswami wrote:
> Hi John
> 
> On 11/20/22 11:18 PM, John Keeping wrote:
> > On Sun, Nov 20, 2022 at 12:23:50PM +0530, Udipto Goswami wrote:
> > > On 11/18/22 9:49 PM, John Keeping wrote:
> > > > On Wed, Nov 16, 2022 at 04:49:55PM +0530, Udipto Goswami wrote:
> > > > > While performing fast composition switch, there is a possibility that the
> > > > > process of ffs_ep0_write/ffs_ep0_read get into a race condition
> > > > > due to ep0req being freed up from functionfs_unbind.
> > > > > 
> > > > > Consider the scenario that the ffs_ep0_write calls the ffs_ep0_queue_wait
> > > > > by taking a lock &ffs->ev.waitq.lock. However, the functionfs_unbind isn't
> > > > > bounded so it can go ahead and mark the ep0req to NULL, and since there
> > > > > is no NULL check in ffs_ep0_queue_wait we will end up in use-after-free.
> > > > > 
> > > > > Fix this by making a serialized execution between the two functions using
> > > > > a mutex_lock(ffs->mutex). Also, dequeue the ep0req to ensure that no
> > > > > other function can use it after the free operation.
> > > > > 
> > > > > Fixes: ddf8abd25994 ("USB: f_fs: the FunctionFS driver")
> > > > > Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > > > ---
> > > > > v2: Replaces spinlock with mutex & added dequeue operation in unbind.
> > > > > 
> > > > >    drivers/usb/gadget/function/f_fs.c | 7 +++++++
> > > > >    1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > > > index 73dc10a77cde..1439449df39a 100644
> > > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > > @@ -279,6 +279,9 @@ static int __ffs_ep0_queue_wait(struct ffs_data *ffs, char *data, size_t len)
> > > > >    	struct usb_request *req = ffs->ep0req;
> > > > >    	int ret;
> > > > > +	if (!req)
> > > > > +		return -EINVAL;
> > > > > +
> > > > >    	req->zero     = len < le16_to_cpu(ffs->ev.setup.wLength);
> > > > >    	spin_unlock_irq(&ffs->ev.waitq.lock);
> > > > > @@ -1892,10 +1895,14 @@ static void functionfs_unbind(struct ffs_data *ffs)
> > > > >    	ENTER();
> > > > >    	if (!WARN_ON(!ffs->gadget)) {
> > > > > +		mutex_lock(&ffs->mutex);
> > > > > +		/* dequeue before freeing ep0req */
> > > > > +		usb_ep_dequeue(ffs->gadget->ep0, ffs->ep0req);
> > > > >    		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
> > > > >    		ffs->ep0req = NULL;
> > > > >    		ffs->gadget = NULL;
> > > > >    		clear_bit(FFS_FL_BOUND, &ffs->flags);
> > > > > +		mutex_unlock(&ffs->mutex);
> > > > 
> > > > There's now a deadlock here if some other thread is waiting in
> > > > __ffs_ep0_queue_wait() on ep0req_completion.
> > > > 
> > > > You need to dequeue before taking the lock.
> > > That's a control request right, will it be async?
> > > 
> > > Anyway I see only 2 possible threads ep0_read/ep0_write who calls
> > > ep0_queue_wait and waits for the completion of ep0req and both
> > > ep0_read/write are prptected by the mutex lock so i guess execution won't
> > > reach there right ?
> > > Say functionfs_unbind ran first then ep0_read/write had to wait will the
> > > functionfs_unbind is completed so ep_dequeue will ran, will get completed,
> > > further free the request, mark in NULL. now ep0_read/write will have ep0req
> > > as NULL so bail out.
> > > 
> > > Is reverse then functionfs_unbind will wait will the ep0_read/write is
> > > completed.
> > 
> > What guarantee is there that the transfer completes?
> > 
> > If there is such a guarantee, then the request will not be queued, so
> > why is usb_ep_dequeue() necessary?
> 
> I Agree that we cannot say that for sure, but we see that
> wait_for_completion in the ep0_queue_wait is also inside mutex which was
> acquired in ep0_read/write right?

Correct.

> I Though of maintaining the uniformity for the approaches.

What uniformity?  If one process is blocked waiting for completion and
another process wants to cancel the operation, then the cancel
(usb_eq_dequeue()) must run concurrently with the wait, otherwise the
blocked process will never wake up.