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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Signed-off-by: TaoXue <xuetao09@xxxxxxxxxx>
---
  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);



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux