On Wed, Dec 14, 2022 at 03:42:10PM +0530, Udipto Goswami wrote:
> Hi Greg,
>
> On 12/14/22 1:53 PM, Greg Kroah-Hartman wrote:
> > On Wed, Dec 14, 2022 at 12:47:17PM +0530, Udipto Goswami wrote:
> > > Hi All,
> > >
> > > On 11/25/22 11:11 AM, 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@xxxxxxxxxxx>
> > > > ---
> > > > v3: Moved dequeue out of mutex to prevent deadlock
> > > >
> > > > 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..523a961b910b 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)) {
> > > > + /* dequeue before freeing ep0req */
> > > > + usb_ep_dequeue(ffs->gadget->ep0, ffs->ep0req);
> > > > + mutex_lock(&ffs->mutex);
> > > > 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);
> > > > }
> > > > }
> > >
> > > Gentle reminder for any comments/suggestions on this patch.
> >
> > It's the middle of the merge window, and you submitted a patch that has
> > obvious coding style issues, so there's nothing that we can do with it
> > no matter what...
>
> Apologies for this, I had ran checkpatch, I didn't get any error:
>
> total: 0 errors, 0 warnings, 23 lines checked
>
> 0001-usb-gadget-f_fs-Prevent-race-between-functionfs_unbi.patch has no
> obvious style problems and is ready for submission.
>
> was curious what's the coding style error you are referring here?
No blank line after the variable description and before your new if
statement.
thanks,
greg k-h
