| Message ID | 20221117053548.28158-1-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: configfs: Prevent double delete from purge_configs_funcs | expand |
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 >
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
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,
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,
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(-)