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. > req->zero = len < le16_to_cpu(ffs->ev.setup.wLength); > > spin_unlock_irq(&ffs->ev.waitq.lock); > @@ -1892,11 +1899,13 @@ static void functionfs_unbind(struct ffs_data *ffs) > ENTER(); > > if (!WARN_ON(!ffs->gadget)) { > + spin_lock_irq(&ffs->ev.waitq.lock); > usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req); > ffs->ep0req = NULL; > ffs->gadget = NULL; > clear_bit(FFS_FL_BOUND, &ffs->flags); > ffs_data_put(ffs); > + spin_unlock_irq(&ffs->ev.waitq.lock); ffs may have been freed in ffs_data_put() so accessing the lock here is unsafe.