* [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
@ 2022-11-25 5:41 Udipto Goswami
2022-12-14 7:17 ` Udipto Goswami
0 siblings, 1 reply; 6+ messages in thread
From: Udipto Goswami @ 2022-11-25 5:41 UTC (permalink / raw)
To: John Keeping, Greg Kroah-Hartman, linux-usb
Cc: Jack Pham, Pratham Pratap, Wesley Cheng, Udipto Goswami
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@quicinc.com>
---
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);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
2022-11-25 5:41 [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait Udipto Goswami
@ 2022-12-14 7:17 ` Udipto Goswami
2022-12-14 8:23 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Udipto Goswami @ 2022-12-14 7:17 UTC (permalink / raw)
To: John Keeping, Greg Kroah-Hartman, linux-usb
Cc: Jack Pham, Pratham Pratap, Wesley Cheng
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@quicinc.com>
> ---
> 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.
Thanks,
-Udipto
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
2022-12-14 7:17 ` Udipto Goswami
@ 2022-12-14 8:23 ` Greg Kroah-Hartman
2022-12-14 10:12 ` Udipto Goswami
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-12-14 8:23 UTC (permalink / raw)
To: Udipto Goswami
Cc: John Keeping, linux-usb, Jack Pham, Pratham Pratap, Wesley Cheng
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@quicinc.com>
> > ---
> > 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...
Also, you are not explaining how this has been tested, or if it really
solves the problem for you or not, and it seems you are mixing two
different things into the same patch.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
2022-12-14 8:23 ` Greg Kroah-Hartman
@ 2022-12-14 10:12 ` Udipto Goswami
2022-12-14 10:22 ` Greg Kroah-Hartman
0 siblings, 1 reply; 6+ messages in thread
From: Udipto Goswami @ 2022-12-14 10:12 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: John Keeping, linux-usb, Jack Pham, Pratham Pratap, Wesley Cheng
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@quicinc.com>
>>> ---
>>> 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?
>
> Also, you are not explaining how this has been tested, or if it really
> solves the problem for you or not, and it seems you are mixing two
> different things into the same patch.
Yah it does solves the problem of the race condition.
For testing this i was running USB Plug-in Plug-out along with fast
composition switch.
Thanks for pointing it out I'll make a a separate patch for ep0_dequeue.
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
2022-12-14 10:12 ` Udipto Goswami
@ 2022-12-14 10:22 ` Greg Kroah-Hartman
2022-12-14 11:06 ` Udipto Goswami
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2022-12-14 10:22 UTC (permalink / raw)
To: Udipto Goswami
Cc: John Keeping, linux-usb, Jack Pham, Pratham Pratap, Wesley Cheng
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@quicinc.com>
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait
2022-12-14 10:22 ` Greg Kroah-Hartman
@ 2022-12-14 11:06 ` Udipto Goswami
0 siblings, 0 replies; 6+ messages in thread
From: Udipto Goswami @ 2022-12-14 11:06 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: John Keeping, linux-usb, Jack Pham, Pratham Pratap, Wesley Cheng
On 12/14/22 3:52 PM, Greg Kroah-Hartman wrote:
> 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@quicinc.com>
>>>>> ---
>>>>> 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.
Aah, Thanks for pointing out I'll split the change into two and push v4
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-12-14 11:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-25 5:41 [v3] usb: gadget: f_fs: Prevent race between functionfs_unbind & ffs_ep0_queue_wait Udipto Goswami
2022-12-14 7:17 ` Udipto Goswami
2022-12-14 8:23 ` Greg Kroah-Hartman
2022-12-14 10:12 ` Udipto Goswami
2022-12-14 10:22 ` Greg Kroah-Hartman
2022-12-14 11:06 ` Udipto Goswami
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox