[PATCH] USB: gadget: Fix use-after-free during usb config switch

[PATCH] USB: gadget: Fix use-after-free during usb config switch

[jiantao zhang]

In the process of switching USB config from rndis to other config,
if the hardware does not support the ->pullup callback, or the
hardware encounters a low probability fault, both of them may cause
the ->pullup callback to fail, which will then cause a system panic
(use after free).

The gadget drivers sometimes need to be unloaded regardless of the
hardware's behavior.

Analysis as follows:
=======================================================================
(1) write /config/usb_gadget/g1/UDC "none" (init.usb.configfs.rc:2)

gether_disconnect+0x2c/0x1f8 (dev->port_usb = NULL)
rndis_disable+0x4c/0x74
composite_disconnect+0x74/0xb0
configfs_composite_disconnect+0x60/0x7c
usb_gadget_disconnect+0x70/0x124
usb_gadget_unregister_driver+0xc8/0x1d8
gadget_dev_desc_UDC_store+0xec/0x1e4

In function usb_gadget_disconnect(),The ->disconnect() callback will
not be called when gadget->ops->pullup() return an error, therefore,
pointer dev->port will not be set to NULL. If pointer dev->port_usb
is not null, it will cause an exception of use-after-free in step3.

(2) rm /config/usb_gadget/g1/configs/b.1/f1 (init.usb.configfs.rc:8)
(f1 -> ../../../../usb_gadget/g1/functions/rndis.gs4)

rndis_deregister+0x28/0x54 (kfree(params))
rndis_free+0x44/0x7c (kfree(rndis))
usb_put_function+0x14/0x1c
config_usb_cfg_unlink+0xc4/0xe0
configfs_unlink+0x124/0x1c8
vfs_unlink+0x114/0x1dc

(3) rmdir /config/usb_gadget/g1/functions/rndis.gs4
(init.usb.configfs.rc:11)

Call trace:
panic+0x1fc/0x3d0
die+0x29c/0x2a8
do_page_fault+0xa8/0x46c
do_mem_abort+0x3c/0xac
el1_sync_handler+0x40/0x78
0xffffff801138f880 (params->resp_avail is an illegal func pointer)
rndis_close+0x28/0x34 (->rndis_indicate_status_msg->params->resp_avail)
eth_stop+0x74/0x110 (if dev->port_usb != NULL, call rndis_close)
__dev_close_many+0x134/0x194
dev_close_many+0x48/0x194
rollback_registered_many+0x118/0x814
unregister_netdevice_queue+0xe0/0x168
unregister_netdev+0x20/0x30
gether_cleanup+0x1c/0x38
rndis_free_inst+0x2c/0x58
rndis_attr_release+0xc/0x14
kref_put+0x74/0xb8
config_item_put+0x14/0x1c
configfs_rmdir+0x314/0x374

In step3,function pointer params->resp_avail() is a wild pointer
becase pointer params has been freed in step2.

Free mem stack(in step2):
usb_put_function -> rndis_free -> rndis_deregister -> kfree(params)

use-after-free stack(in step3):
eth_stop -> rndis_close -> rndis_signal_disconnect ->
rndis_indicate_status_msg -> params->resp_avail()

In function eth_stop(), if pointer dev->port_usb is NULL, function
rndis_close() will not be called.
If gadget->ops->pullup() return an error in step1,dev->port_usb will
not be set to null. So, a panic will be caused in step3.
=======================================================================
Fixes:<0a55187a1ec8c> (USB: gadget core: Issue ->disconnect()
callback from usb_gadget_disconnect())
Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
Signed-off-by: TaoXue <xuetao09@huawei.com>
---
drivers/usb/gadget/udc/core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index c63c0c2cf649..bf9878e1a72a 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -734,13 +734,13 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
}
ret = gadget->ops->pullup(gadget, 0);
- if (!ret) {
+ if (!ret)
gadget->connected = 0;
- mutex_lock(&udc_lock);
- if (gadget->udc->driver)
- gadget->udc->driver->disconnect(gadget);
- mutex_unlock(&udc_lock);
- }
+
+ mutex_lock(&udc_lock);
+ if (gadget->udc->driver)
+ gadget->udc->driver->disconnect(gadget);
+ mutex_unlock(&udc_lock);
out:
trace_usb_gadget_disconnect(gadget, ret);

-- 
2.17.1

Re: [PATCH] USB: gadget: Fix use-after-free during usb config switch

[gregkh]

On Thu, Nov 10, 2022 at 09:20:17PM +0800, jiantao zhang wrote:
> In the process of switching USB config from rndis to other config,
> if the hardware does not support the ->pullup callback, or the
> hardware encounters a low probability fault, both of them may cause
> the ->pullup callback to fail, which will then cause a system panic
> (use after free).
> 
> The gadget drivers sometimes need to be unloaded regardless of the
> hardware's behavior.
> 
> Analysis as follows:
> =======================================================================
> (1) write /config/usb_gadget/g1/UDC "none" (init.usb.configfs.rc:2)
> 
> gether_disconnect+0x2c/0x1f8 (dev->port_usb = NULL)
> rndis_disable+0x4c/0x74
> composite_disconnect+0x74/0xb0
> configfs_composite_disconnect+0x60/0x7c
> usb_gadget_disconnect+0x70/0x124
> usb_gadget_unregister_driver+0xc8/0x1d8
> gadget_dev_desc_UDC_store+0xec/0x1e4
> 
> In function usb_gadget_disconnect(),The ->disconnect() callback will
> not be called when gadget->ops->pullup() return an error, therefore,
> pointer dev->port will not be set to NULL. If pointer dev->port_usb
> is not null, it will cause an exception of use-after-free in step3.
> 
> (2) rm /config/usb_gadget/g1/configs/b.1/f1 (init.usb.configfs.rc:8)
> (f1 -> ../../../../usb_gadget/g1/functions/rndis.gs4)
> 
> rndis_deregister+0x28/0x54 (kfree(params))
> rndis_free+0x44/0x7c (kfree(rndis))
> usb_put_function+0x14/0x1c
> config_usb_cfg_unlink+0xc4/0xe0
> configfs_unlink+0x124/0x1c8
> vfs_unlink+0x114/0x1dc
> 
> (3) rmdir /config/usb_gadget/g1/functions/rndis.gs4
> (init.usb.configfs.rc:11)
> 
> Call trace:
> panic+0x1fc/0x3d0
> die+0x29c/0x2a8
> do_page_fault+0xa8/0x46c
> do_mem_abort+0x3c/0xac
> el1_sync_handler+0x40/0x78
> 0xffffff801138f880 (params->resp_avail is an illegal func pointer)
> rndis_close+0x28/0x34 (->rndis_indicate_status_msg->params->resp_avail)
> eth_stop+0x74/0x110 (if dev->port_usb != NULL, call rndis_close)
> __dev_close_many+0x134/0x194
> dev_close_many+0x48/0x194
> rollback_registered_many+0x118/0x814
> unregister_netdevice_queue+0xe0/0x168
> unregister_netdev+0x20/0x30
> gether_cleanup+0x1c/0x38
> rndis_free_inst+0x2c/0x58
> rndis_attr_release+0xc/0x14
> kref_put+0x74/0xb8
> config_item_put+0x14/0x1c
> configfs_rmdir+0x314/0x374
> 
> In step3,function pointer params->resp_avail() is a wild pointer
> becase pointer params has been freed in step2.
> 
> Free mem stack(in step2):
> usb_put_function -> rndis_free -> rndis_deregister -> kfree(params)
> 
> use-after-free stack(in step3):
> eth_stop -> rndis_close -> rndis_signal_disconnect ->
> rndis_indicate_status_msg -> params->resp_avail()
> 
> In function eth_stop(), if pointer dev->port_usb is NULL, function
> rndis_close() will not be called.
> If gadget->ops->pullup() return an error in step1,dev->port_usb will
> not be set to null. So, a panic will be caused in step3.
> =======================================================================
> Fixes:<0a55187a1ec8c> (USB: gadget core: Issue ->disconnect()
> callback from usb_gadget_disconnect())
> Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> Signed-off-by: TaoXue <xuetao09@huawei.com>
> ---
> drivers/usb/gadget/udc/core.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index c63c0c2cf649..bf9878e1a72a 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -734,13 +734,13 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> }
> ret = gadget->ops->pullup(gadget, 0);
> - if (!ret) {
> + if (!ret)
> gadget->connected = 0;
> - mutex_lock(&udc_lock);
> - if (gadget->udc->driver)
> - gadget->udc->driver->disconnect(gadget);
> - mutex_unlock(&udc_lock);
> - }
> +
> + mutex_lock(&udc_lock);
> + if (gadget->udc->driver)
> + gadget->udc->driver->disconnect(gadget);
> + mutex_unlock(&udc_lock);
> out:
> trace_usb_gadget_disconnect(gadget, ret);
> 
> -- 
> 2.17.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch is malformed (tabs converted to spaces, linewrapped, etc.)
  and can not be applied.  Please read the file,
  Documentation/email-clients.txt in order to fix this.

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/SubmittingPatches for what needs to be done
  here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH] USB: gadget: Fix use-after-free during usb config switch

[jiantao zhang]

> In the process of switching USB config from rndis to other config,
> if function gadget->ops->pullup() return an error,it will inevitably
> cause a system panic(use after free).
>
> Analysis as follows:
> =====================================================================
> (1) write /config/usb_gadget/g1/UDC "none"   (init.usb.configfs.rc:2)
>
> gether_disconnect+0x2c/0x1f8           (dev->port_usb = NULL)
> rndis_disable+0x4c/0x74
> composite_disconnect+0x74/0xb0
> configfs_composite_disconnect+0x60/0x7c
> usb_gadget_disconnect+0x70/0x124
> usb_gadget_unregister_driver+0xc8/0x1d8
> gadget_dev_desc_UDC_store+0xec/0x1e4
>
> In general, pointer dev->port will be set to null.
> In function usb_gadget_disconnect(),gadget->udc->driver->disconnect()
> will not be called when gadget->ops->pullup() return an error, therefore,
> pointer dev->port will not be set to NULL.
> If pointer dev->port_usb is not null, it will cause an exception of using
> the freed memory in step3.
>
> (2) rm /config/usb_gadget/g1/configs/b.1/f1    (init.usb.configfs.rc:8)
>      (f1 -> ../../../../usb_gadget/g1/functions/rndis.gs4)
>
> rndis_deregister+0x28/0x54        (kfree(params))
> rndis_free+0x44/0x7c              (kfree(rndis))
> usb_put_function+0x14/0x1c
> config_usb_cfg_unlink+0xc4/0xe0
> configfs_unlink+0x124/0x1c8
> vfs_unlink+0x114/0x1dc
>
> (3) rmdir /config/usb_gadget/g1/functions/rndis.gs4
>      (init.usb.configfs.rc:11)
>
> Call trace:
> panic+0x1fc/0x3d0
> die+0x29c/0x2a8
> die_kernel_fault+0x60/0x90
> do_page_fault+0xa8/0x46c
> do_mem_abort+0x3c/0xac
> el1_abort+0x3c/0x58
> el1_sync_handler+0x40/0x78
> el1_sync+0x84/0x140
> 0xffffff801138f880    (params->resp_avail is an illegal function pointer)
> rndis_close+0x28/0x34 (->rndis_indicate_status_msg-> params->resp_avail())
> eth_stop+0x74/0x110   (if dev->port_usb != NULL, call rndis_close)
> __dev_close_many+0x134/0x194
> dev_close_many+0x48/0x194
> rollback_registered_many+0x118/0x814
> unregister_netdevice_queue+0xe0/0x168
> unregister_netdev+0x20/0x30
> gether_cleanup+0x1c/0x38
> rndis_free_inst+0x2c/0x58
> usb_put_function_instance+0x20/0x34
> rndis_attr_release+0xc/0x14
> kref_put+0x74/0xb8
> config_item_put+0x14/0x1c
> configfs_rmdir+0x314/0x374
> vfs_rmdir+0xa4/0x15c
>
> In step3,function pointer params->resp_avail() is a wild pointer
> becase pointer params has been freed in step2.
>
>      Free mem stack(in step2):
>      usb_put_function -> rndis_free -> rndis_deregister -> kfree(params)
>
>      use-after-free stack(in step3):
>      eth_stop -> rndis_close -> rndis_signal_disconnect ->
>      rndis_indicate_status_msg -> params->resp_avail()
>
> In function eth_stop(), if pointer dev->port_usb is NULL, function
> rndis_close() will not be called.
>
> If gadget->ops->pullup() return an error in step),dev->port_usb will
> not be set to null. So, a panic will be caused in step3.
> =======================================================================
>
> Through above analysis, i think gadget->udc->driver->disconnect() need
> to be called even if gadget->udc->driver->disconnect() return an error.
>
> This reverts commit 0a55187a1ec8c03d0619e7ce41d10fdc39cff036
>
> I think this change is a code optimization, not a solution to specific
> problem. And i think this problem is caused by this change,revert it can
> solve this problem.
>
> Of course, my understanding may be inaccurate.There may be a better
> solution to this problem. If possible, please give some suggestions,
> thanks.
>
> Fixes:<0a55187a1ec8c> (USB: gadget core: Issue ->disconnect()
> callback from usb_gadget_disconnect())
>
> Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> Signed-off-by: TaoXue <xuetao09@huawei.com>
> ---
>   drivers/usb/gadget/udc/core.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index c63c0c2cf649..b502b2ac4824 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -707,9 +707,6 @@ EXPORT_SYMBOL_GPL(usb_gadget_connect);
>    * as a disconnect (when a VBUS session is active).  Not all systems
>    * support software pullup controls.
>    *
> - * Following a successful disconnect, invoke the ->disconnect() callback
> - * for the current gadget driver so that UDC drivers don't need to.
> - *
>    * Returns zero on success, else negative errno.
>    */
>   int usb_gadget_disconnect(struct usb_gadget *gadget)
> @@ -734,13 +731,8 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
>   	}
>   
>   	ret = gadget->ops->pullup(gadget, 0);
> -	if (!ret) {
> +	if (!ret)
>   		gadget->connected = 0;
> -		mutex_lock(&udc_lock);
> -		if (gadget->udc->driver)
> -			gadget->udc->driver->disconnect(gadget);
> -		mutex_unlock(&udc_lock);
> -	}
>   
>   out:
>   	trace_usb_gadget_disconnect(gadget, ret);
> @@ -1532,6 +1524,11 @@ static void gadget_unbind_driver(struct device *dev)
>   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>   
>   	usb_gadget_disconnect(gadget);
> +
> +	mutex_lock(&udc_lock);
> +	udc->driver->disconnect(udc->gadget);
> +	mutex_unlock(&udc_lock);
> +
>   	usb_gadget_disable_async_callbacks(udc);
>   	if (gadget->irq)
>   		synchronize_irq(gadget->irq);
> @@ -1626,6 +1623,11 @@ static ssize_t soft_connect_store(struct device *dev,
>   		usb_gadget_connect(udc->gadget);
>   	} else if (sysfs_streq(buf, "disconnect")) {
>   		usb_gadget_disconnect(udc->gadget);
> +
> +		mutex_lock(&udc_lock);
> +		udc->driver->disconnect(udc->gadget);
> +		mutex_unlock(&udc_lock);
> +
>   		usb_gadget_udc_stop(udc);
>   	} else {
>   		dev_err(dev, "unsupported command '%s'\n", buf);

Re: [PATCH] USB: gadget: Fix use-after-free during usb config switch

[stern]

Was this supposed to be an actual patch submission?  It doesn't look 
like it, because each line of the email begins with "> ".

On Wed, Nov 09, 2022 at 10:11:51PM +0800, jiantao zhang wrote:
> > In the process of switching USB config from rndis to other config,
> > if function gadget->ops->pullup() return an error,it will inevitably
> > cause a system panic(use after free).
> > 
> > Analysis as follows:
> > =====================================================================
> > (1) write /config/usb_gadget/g1/UDC "none"   (init.usb.configfs.rc:2)
> > 
> > gether_disconnect+0x2c/0x1f8           (dev->port_usb = NULL)
> > rndis_disable+0x4c/0x74
> > composite_disconnect+0x74/0xb0
> > configfs_composite_disconnect+0x60/0x7c
> > usb_gadget_disconnect+0x70/0x124
> > usb_gadget_unregister_driver+0xc8/0x1d8
> > gadget_dev_desc_UDC_store+0xec/0x1e4
> > 
> > In general, pointer dev->port will be set to null.
> > In function usb_gadget_disconnect(),gadget->udc->driver->disconnect()
> > will not be called when gadget->ops->pullup() return an error, therefore,
> > pointer dev->port will not be set to NULL.
> > If pointer dev->port_usb is not null, it will cause an exception of using
> > the freed memory in step3.

Good point.

...
> > Through above analysis, i think gadget->udc->driver->disconnect() need
> > to be called even if gadget->udc->driver->disconnect() return an error.

You mean "even if gadget->ops->pullup() returns an error".  That seems 
reasonable.  The only reason for the ->pullup callback to fail is if the 
hardware doesn't support it, but gadget drivers sometimes need to be 
unloaded regardless of the hardware's capabilities.

> > This reverts commit 0a55187a1ec8c03d0619e7ce41d10fdc39cff036

But this is not reasonable.  You should change the code so that it does 
what you want: Make it call driver->disconnect() even if ops->pullup() 
returns an error.  Don't revert an entire commit just because of one 
side effect.

> > I think this change is a code optimization, not a solution to specific
> > problem. And i think this problem is caused by this change,revert it can
> > solve this problem.

That commit was not just a code optimization.  It did fix a problem: 
Some UDCs would not call driver->disconnect() because they expected the 
core to make the call for them.

> > Of course, my understanding may be inaccurate.There may be a better
> > solution to this problem. If possible, please give some suggestions,
> > thanks.

Simply move the lines which grab the mutex and do the callback after 
the "if" statement, so that they will run regardless of what 
ops->pullup() returns.

Alan Stern

> > Fixes:<0a55187a1ec8c> (USB: gadget core: Issue ->disconnect()
> > callback from usb_gadget_disconnect())
> > 
> > Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> > Signed-off-by: TaoXue <xuetao09@huawei.com>
> > ---
> >   drivers/usb/gadget/udc/core.c | 20 +++++++++++---------
> >   1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index c63c0c2cf649..b502b2ac4824 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -707,9 +707,6 @@ EXPORT_SYMBOL_GPL(usb_gadget_connect);
> >    * as a disconnect (when a VBUS session is active).  Not all systems
> >    * support software pullup controls.
> >    *
> > - * Following a successful disconnect, invoke the ->disconnect() callback
> > - * for the current gadget driver so that UDC drivers don't need to.
> > - *
> >    * Returns zero on success, else negative errno.
> >    */
> >   int usb_gadget_disconnect(struct usb_gadget *gadget)
> > @@ -734,13 +731,8 @@ int usb_gadget_disconnect(struct usb_gadget *gadget)
> >   	}
> >   	ret = gadget->ops->pullup(gadget, 0);
> > -	if (!ret) {
> > +	if (!ret)
> >   		gadget->connected = 0;
> > -		mutex_lock(&udc_lock);
> > -		if (gadget->udc->driver)
> > -			gadget->udc->driver->disconnect(gadget);
> > -		mutex_unlock(&udc_lock);
> > -	}
> >   out:
> >   	trace_usb_gadget_disconnect(gadget, ret);
> > @@ -1532,6 +1524,11 @@ static void gadget_unbind_driver(struct device *dev)
> >   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >   	usb_gadget_disconnect(gadget);
> > +
> > +	mutex_lock(&udc_lock);
> > +	udc->driver->disconnect(udc->gadget);
> > +	mutex_unlock(&udc_lock);
> > +
> >   	usb_gadget_disable_async_callbacks(udc);
> >   	if (gadget->irq)
> >   		synchronize_irq(gadget->irq);
> > @@ -1626,6 +1623,11 @@ static ssize_t soft_connect_store(struct device *dev,
> >   		usb_gadget_connect(udc->gadget);
> >   	} else if (sysfs_streq(buf, "disconnect")) {
> >   		usb_gadget_disconnect(udc->gadget);
> > +
> > +		mutex_lock(&udc_lock);
> > +		udc->driver->disconnect(udc->gadget);
> > +		mutex_unlock(&udc_lock);
> > +
> >   		usb_gadget_udc_stop(udc);
> >   	} else {
> >   		dev_err(dev, "unsupported command '%s'\n", buf);