* [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs
@ 2022-11-17 5:35 Udipto Goswami
2022-11-18 16:34 ` John Keeping
0 siblings, 1 reply; 4+ messages in thread
From: Udipto Goswami @ 2022-11-17 5:35 UTC (permalink / raw)
To: Jack Pham, Alan Stern, Greg Kroah-Hartman, linux-usb
Cc: Pratham Pratap, Wesley Cheng, Udipto Goswami
Currently the function purge_configs_funcs isn't protected by any lock
So there is a potential race possible there id udc called composite_unbind
and say at the same time the configfs_composite_bind also got executed.
If bind fails and follows the error path, driver will end up calling the
purge_configfs_funcs again, resulting in double free from func_list.
Fix this by introducing mutex gi->lock to make sure serial execution
can be maintained. Also, usage of list_for_each_entry_safe is risky
here since the tmp variable is still valid for the next iteration, in
order to ensure list traversal is clean, use list_last_entry.
Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
---
drivers/usb/gadget/configfs.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index 3a6b4926193e..f1ac87906897 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
{
struct usb_configuration *c;
+ mutex_lock(&gi->lock);
list_for_each_entry(c, &gi->cdev.configs, list) {
- struct usb_function *f, *tmp;
+ struct usb_function *f;
struct config_usb_cfg *cfg;
cfg = container_of(c, struct config_usb_cfg, c);
-
- list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
-
+ while (!list_empty(&c->functions)) {
+ f = list_last_entry(&c->functions, struct usb_function, list);
list_move(&f->list, &cfg->func_list);
if (f->unbind) {
dev_dbg(&gi->cdev.gadget->dev,
@@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
c->highspeed = 0;
c->fullspeed = 0;
}
+ mutex_unlock(&gi->lock);
}
static int configfs_composite_bind(struct usb_gadget *gadget,
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs
2022-11-17 5:35 [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs Udipto Goswami
@ 2022-11-18 16:34 ` John Keeping
2022-11-20 7:10 ` Udipto Goswami
0 siblings, 1 reply; 4+ messages in thread
From: John Keeping @ 2022-11-18 16:34 UTC (permalink / raw)
To: Udipto Goswami
Cc: Jack Pham, Alan Stern, Greg Kroah-Hartman, linux-usb,
Pratham Pratap, Wesley Cheng
On Thu, Nov 17, 2022 at 11:05:48AM +0530, Udipto Goswami wrote:
> Currently the function purge_configs_funcs isn't protected by any lock
> So there is a potential race possible there id udc called composite_unbind
> and say at the same time the configfs_composite_bind also got executed.
> If bind fails and follows the error path, driver will end up calling the
> purge_configfs_funcs again, resulting in double free from func_list.
>
> Fix this by introducing mutex gi->lock to make sure serial execution
> can be maintained. Also, usage of list_for_each_entry_safe is risky
> here since the tmp variable is still valid for the next iteration, in
> order to ensure list traversal is clean, use list_last_entry.
I don't understand how this can happen.
purge_configs_funcs() is called from the .bind and .unbind
usb_gadget_driver hooks, and those should not be able to run at the same
time - if bind has not succeeded, how does unbinding make sense?
> Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> ---
> drivers/usb/gadget/configfs.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> index 3a6b4926193e..f1ac87906897 100644
> --- a/drivers/usb/gadget/configfs.c
> +++ b/drivers/usb/gadget/configfs.c
> @@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
> {
> struct usb_configuration *c;
>
> + mutex_lock(&gi->lock);
> list_for_each_entry(c, &gi->cdev.configs, list) {
> - struct usb_function *f, *tmp;
> + struct usb_function *f;
> struct config_usb_cfg *cfg;
>
> cfg = container_of(c, struct config_usb_cfg, c);
> -
> - list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
> -
> + while (!list_empty(&c->functions)) {
> + f = list_last_entry(&c->functions, struct usb_function, list);
> list_move(&f->list, &cfg->func_list);
> if (f->unbind) {
> dev_dbg(&gi->cdev.gadget->dev,
> @@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
> c->highspeed = 0;
> c->fullspeed = 0;
> }
> + mutex_unlock(&gi->lock);
> }
>
> static int configfs_composite_bind(struct usb_gadget *gadget,
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs
2022-11-18 16:34 ` John Keeping
@ 2022-11-20 7:10 ` Udipto Goswami
2022-11-20 17:45 ` John Keeping
0 siblings, 1 reply; 4+ messages in thread
From: Udipto Goswami @ 2022-11-20 7:10 UTC (permalink / raw)
To: John Keeping
Cc: Jack Pham, Alan Stern, Greg Kroah-Hartman, linux-usb,
Pratham Pratap, Wesley Cheng
Hi John,
On 11/18/22 10:04 PM, John Keeping wrote:
> On Thu, Nov 17, 2022 at 11:05:48AM +0530, Udipto Goswami wrote:
>> Currently the function purge_configs_funcs isn't protected by any lock
>> So there is a potential race possible there id udc called composite_unbind
>> and say at the same time the configfs_composite_bind also got executed.
>> If bind fails and follows the error path, driver will end up calling the
>> purge_configfs_funcs again, resulting in double free from func_list.
>>
>> Fix this by introducing mutex gi->lock to make sure serial execution
>> can be maintained. Also, usage of list_for_each_entry_safe is risky
>> here since the tmp variable is still valid for the next iteration, in
>> order to ensure list traversal is clean, use list_last_entry.
>
> I don't understand how this can happen.
>
> purge_configs_funcs() is called from the .bind and .unbind
> usb_gadget_driver hooks, and those should not be able to run at the same
> time - if bind has not succeeded, how does unbinding make sense?
Consider this,
we are going out of the composition i.e unbinding it,
lets say we have 7 functions and we were only at removing the 2 one
where in the meantime the bind process got called, the udc driver
doesn't make sure of this, depends on the controller when they call the
usb_add_gadget probably.
For dwc3 controller specifically, the mode will still be device so from
core.c will end up calling gadget_init() which in turn will call
usb_add_gadget which further does to check_pending_gadget_drivers,
eventually calling bind.
So, i don't see any serializing mechanism here which would stop the
execution of make the controller wait until the unbind is completed
right. Please correct me if i'm wrong.
>
>> Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
>> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
>> ---
>> drivers/usb/gadget/configfs.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 3a6b4926193e..f1ac87906897 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
>> {
>> struct usb_configuration *c;
>>
>> + mutex_lock(&gi->lock);
>> list_for_each_entry(c, &gi->cdev.configs, list) {
>> - struct usb_function *f, *tmp;
>> + struct usb_function *f;
>> struct config_usb_cfg *cfg;
>>
>> cfg = container_of(c, struct config_usb_cfg, c);
>> -
>> - list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
>> -
>> + while (!list_empty(&c->functions)) {
>> + f = list_last_entry(&c->functions, struct usb_function, list);
>> list_move(&f->list, &cfg->func_list);
>> if (f->unbind) {
>> dev_dbg(&gi->cdev.gadget->dev,
>> @@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
>> c->highspeed = 0;
>> c->fullspeed = 0;
>> }
>> + mutex_unlock(&gi->lock);
>> }
>>
>> static int configfs_composite_bind(struct usb_gadget *gadget,
>> --
>> 2.17.1
>>
Thanks,
-Udipto
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs
2022-11-20 7:10 ` Udipto Goswami
@ 2022-11-20 17:45 ` John Keeping
0 siblings, 0 replies; 4+ messages in thread
From: John Keeping @ 2022-11-20 17:45 UTC (permalink / raw)
To: Udipto Goswami
Cc: Jack Pham, Alan Stern, Greg Kroah-Hartman, linux-usb,
Pratham Pratap, Wesley Cheng
On Sun, Nov 20, 2022 at 12:40:54PM +0530, Udipto Goswami wrote:
> On 11/18/22 10:04 PM, John Keeping wrote:
> > On Thu, Nov 17, 2022 at 11:05:48AM +0530, Udipto Goswami wrote:
> > > Currently the function purge_configs_funcs isn't protected by any lock
> > > So there is a potential race possible there id udc called composite_unbind
> > > and say at the same time the configfs_composite_bind also got executed.
> > > If bind fails and follows the error path, driver will end up calling the
> > > purge_configfs_funcs again, resulting in double free from func_list.
> > >
> > > Fix this by introducing mutex gi->lock to make sure serial execution
> > > can be maintained. Also, usage of list_for_each_entry_safe is risky
> > > here since the tmp variable is still valid for the next iteration, in
> > > order to ensure list traversal is clean, use list_last_entry.
> >
> > I don't understand how this can happen.
> >
> > purge_configs_funcs() is called from the .bind and .unbind
> > usb_gadget_driver hooks, and those should not be able to run at the same
> > time - if bind has not succeeded, how does unbinding make sense?
>
> Consider this,
> we are going out of the composition i.e unbinding it,
> lets say we have 7 functions and we were only at removing the 2 one where in
> the meantime the bind process got called, the udc driver doesn't make sure
> of this, depends on the controller when they call the usb_add_gadget
> probably.
>
> For dwc3 controller specifically, the mode will still be device so from
> core.c will end up calling gadget_init() which in turn will call
> usb_add_gadget which further does to check_pending_gadget_drivers,
> eventually calling bind.
I can't find functions called gadget_init() or
check_pending_gadget_drivers() in the kernel.
> So, i don't see any serializing mechanism here which would stop the
> execution of make the controller wait until the unbind is completed right.
> Please correct me if i'm wrong.
The comments in configfs_composite_bind() and
configfs_composite_unbind() indicate that gi->lock is held by the caller
for both these functions.
I checked that this is the case for configfs gadgets, so it's possible
that one of the legacy paths has a bug where it is not holding this
lock, but adding the extra lock calls in the patch below will deadlock
with configfs.
> >
> > > Fixes: 6cd0fe913879 ("usb: gadget: configfs: Preserve function ordering after bind failure")
> > > Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > ---
> > > drivers/usb/gadget/configfs.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
> > > index 3a6b4926193e..f1ac87906897 100644
> > > --- a/drivers/usb/gadget/configfs.c
> > > +++ b/drivers/usb/gadget/configfs.c
> > > @@ -1246,14 +1246,14 @@ static void purge_configs_funcs(struct gadget_info *gi)
> > > {
> > > struct usb_configuration *c;
> > > + mutex_lock(&gi->lock);
> > > list_for_each_entry(c, &gi->cdev.configs, list) {
> > > - struct usb_function *f, *tmp;
> > > + struct usb_function *f;
> > > struct config_usb_cfg *cfg;
> > > cfg = container_of(c, struct config_usb_cfg, c);
> > > -
> > > - list_for_each_entry_safe_reverse(f, tmp, &c->functions, list) {
> > > -
> > > + while (!list_empty(&c->functions)) {
> > > + f = list_last_entry(&c->functions, struct usb_function, list);
> > > list_move(&f->list, &cfg->func_list);
> > > if (f->unbind) {
> > > dev_dbg(&gi->cdev.gadget->dev,
> > > @@ -1269,6 +1269,7 @@ static void purge_configs_funcs(struct gadget_info *gi)
> > > c->highspeed = 0;
> > > c->fullspeed = 0;
> > > }
> > > + mutex_unlock(&gi->lock);
> > > }
> > > static int configfs_composite_bind(struct usb_gadget *gadget,
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-11-20 17:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-17 5:35 [PATCH] usb: gadget: configfs: Prevent double delete from purge_configs_funcs Udipto Goswami
2022-11-18 16:34 ` John Keeping
2022-11-20 7:10 ` Udipto Goswami
2022-11-20 17:45 ` John Keeping
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.