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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi John,

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 ?


  	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.
maybe we can move it before data_put ?
   		clear_bit(FFS_FL_BOUND, &ffs->flags);
 +		spin_unlock_irq(&ffs->ev.waitq.lock);
   		ffs_data_put(ffs);

the intention here is to protect the ep0req only since the ep0_queue_wait is also doing the assignments after taking the lock.

Thanks,
-Udipto



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux