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@xxxxxxxxxxx>
> ---
> 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.
