On Fri, Nov 04, 2022 at 03:44:09PM +0530, Udipto Goswami wrote: > On 11/3/22 4:59 PM, Udipto Goswami wrote: > > On 11/3/22 4:22 PM, John Keeping wrote: > > > On Thu, Nov 03, 2022 at 03:57:02PM +0530, Udipto Goswami wrote: > > > > On 11/3/22 3:00 PM, John Keeping wrote: > > > > > On Thu, Nov 03, 2022 at 01:08:21PM +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 introducing a NULL check before any req operation. > > > > > > Also to ensure the serialization, perform the ep0req ops inside > > > > > > spinlock &ffs->ev.waitq.lock. > > > > > > > > > > > > Fixes: ddf8abd25994 ("USB: f_fs: the FunctionFS driver") > > > > > > Signed-off-by: Udipto Goswami <quic_ugoswami@xxxxxxxxxxx> > > > > > > --- > > > > > > drivers/usb/gadget/function/f_fs.c | 9 +++++++++ > > > > > > 1 file changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c > > > > > > b/drivers/usb/gadget/function/f_fs.c > > > > > > index 73dc10a77cde..39980b2bf285 100644 > > > > > > --- a/drivers/usb/gadget/function/f_fs.c > > > > > > +++ b/drivers/usb/gadget/function/f_fs.c > > > > > > @@ -279,6 +279,13 @@ 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; > > > > > > + /* > > > > > > + * Even if ep0req is freed won't be a problem since the local > > > > > > + * copy of the request will be used here. > > > > > > + */ > > > > > > > > > > This doesn't sound right - if we set ep0req to NULL then we've called > > > > > usb_ep_free_request() on it so the request is not longer valid. > > > > > > > > Yes I agree as soon as we spin_unlock it the functionfs_unbind > > > > will execute > > > > and free_up the req, so performing and ep_queue after that even > > > > if it is a > > > > local copy could be fatal. > > > > > > > > ret = usb_ep_queue(ffs->gadget->ep0, req, GFP_ATOMIC); > > > > if (unlikely(ret < 0)) > > > > return ret; > > > > > > > > spin_unlock_irq(&ffs->ev.waitq.lock); > > > > We can move the spin_unlock after to queue operation perhaps ? > > > > > > I don't think it's that simple. The documentation for > > > usb_ep_free_request() says: > > > > > > * Caller guarantees the request is not queued, and that it will > > > * no longer be requeued (or otherwise used). > > > > > > so some extra synchronisation is required here. > > > > > > By the time we get to functionfs_unbind() everything should be disabled > > > by ffs_func_disable() and ffs_func_unbind() has drained the workqueue, > > > but none of that applies to ep0. > > > > > > I think ffs_unbind() needs to dequeue the ep0 request. > > > > > > In addition to that, I think we need a new ep0 status variable in struct > > > ffs_data so that req is not accessed after wait_for_completion() in > > > __ffs_ep0_queue_wait() and that status is set in ffs_ep0_complete(). > > > > > > With the spin_unlock_irq() moved to immediately before > > > wait_for_completion() in __ffs_ep0_queue_wait() it looks like everything > > > is then safe. > > > > Thanks for the suggestions, let me try implementing it. > > > Just curious because i saw __ffs_ep0_queue_wait will only be called from > ffs_ep0_read & ffs_ep0_write, both using a mutex_lock(&ffs->mutex) > > So if we protect the functionfs_unbind with this mutex, it will be better > right? > > @@ -1889,12 +1889,13 @@ static int functionfs_bind(struct ffs_data *ffs, > struct usb_composite_dev *cdev) > static void functionfs_unbind(struct ffs_data *ffs) > { > ENTER(); > > if (!WARN_ON(!ffs->gadget)) { > + ffs_mutex_lock(&ffs->mutex, file->f_flags & O_NONBLOCK); > 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); > ffs_data_put(ffs); > } > } > > Perhaps we don't have to take care of the the serialization in that case > since it will exit the function __ffs_ep0_queue_wait only after everything > is done and hence functionfs_unbind will get the control after the > ep0_write/read has completed? > > What do you think ? The documentation does say it protects ep0req so this might make sense. But I think you need to ensure ep0req is dequeued before locking the mutex in order to avoid a deadlock as nothing else is going to complete an outstanding request at this point.