Hi John,
On 11/4/22 5:19 PM, John Keeping wrote:
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.
Got it, thanks for clarification, i'll make sure to dequeue it to avoid
any deadlocks in v2.
-Udipto
