From 64287585dcf7d61bd0c1d73c2c8ffffd1b8ee062 Mon Sep 17 00:00:00 2001 From: Ajay Agarwal Date: Tue, 8 Jan 2019 11:10:18 +0530 Subject: [PATCH] usb: misc: diag_ipc_bridge: Move dev cleanup to delete function Currently the driver does the diag_briddge_dev's members' cleanup in disconnect function. This can lead to race between read/write and disconnect functions where the read/write mutex is being destroyed when it is in locked state. Also, the read/write function can be called after disconnect leading to mutex_lock warning on a destroyed mutex. Finally, since the close function can be called after disconnect, it can lead to null pointer dereference from dev->ifc since it is being assigned null in disconnect. Also, there can be a use-after-free if the interface structure is used after disconnect function has been called and core has freed the intf. Fix this by moving the dev member cleanup from disconnect to the delete function. This will ensure that mutex and dev->ifc exists when the diag core can still queue read/write and call close. Also do a get and put of interface from probe and delete respectively to prevent the use-after-free issue. Change-Id: I1a1fa4440560b0c0b77880fb3f5a37c3c24c7e67 Signed-off-by: Ajay Agarwal --- drivers/usb/misc/diag_ipc_bridge.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/usb/misc/diag_ipc_bridge.c b/drivers/usb/misc/diag_ipc_bridge.c index 94b5775cd954..f369f69da6a1 100644 --- a/drivers/usb/misc/diag_ipc_bridge.c +++ b/drivers/usb/misc/diag_ipc_bridge.c @@ -131,9 +131,12 @@ static int ipc_bridge_open(struct platform_device *pdev) static void diag_bridge_delete(struct kref *kref) { struct diag_bridge *dev = container_of(kref, struct diag_bridge, kref); + struct usb_interface *ifc = dev->ifc; int id = dev->id; dev_dbg(&dev->ifc->dev, "%s\n", __func__); + usb_set_intfdata(ifc, NULL); + usb_put_intf(ifc); usb_put_dev(dev->udev); __dev[id] = 0; kfree(dev); @@ -308,6 +311,8 @@ int diag_bridge_read(int id, char *data, int size) goto free_error; } + usb_autopm_put_interface(dev->ifc); + if (id == IPC_BRIDGE) { wait_for_completion(&dev->read_done); ret = dev->read_result; @@ -615,7 +620,7 @@ diag_bridge_probe(struct usb_interface *ifc, const struct usb_device_id *id) dev->id = devid; dev->udev = usb_get_dev(interface_to_usbdev(ifc)); - dev->ifc = ifc; + dev->ifc = usb_get_intf(ifc); kref_init(&dev->kref); mutex_init(&dev->ifc_mutex); mutex_init(&dev->read_mutex); @@ -705,13 +710,7 @@ static void diag_bridge_disconnect(struct usb_interface *ifc) platform_device_unregister(dev->pdev); diag_bridge_debugfs_cleanup(); - mutex_lock(&dev->ifc_mutex); - dev->ifc = NULL; - mutex_unlock(&dev->ifc_mutex); - usb_set_intfdata(ifc, NULL); - mutex_destroy(&dev->write_mutex); - mutex_destroy(&dev->read_mutex); - mutex_destroy(&dev->ifc_mutex); + dev->err = -ENODEV; kref_put(&dev->kref, diag_bridge_delete); }