Hi John,
On 11/3/22 4:59 PM, Udipto Goswami wrote:
Hi John
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 ?
-Udipto
