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@xxxxxxxxxxx>
> ---
> 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
>
