From a3073dde63380142e0555d922b0abf9535f528de Mon Sep 17 00:00:00 2001 From: Ajay Agarwal Date: Thu, 24 Jan 2019 14:21:47 +0530 Subject: [PATCH 1/2] usb: misc: diag_ipc_bridge: Patch debug statements Remove pr_fmt declaration and add function name and new line functionality to the debug prints instead. Add more debug logs where necessary. Change-Id: Iedf27473174eeae1c8032c133250a190978d38e5 Signed-off-by: Ajay Agarwal --- drivers/usb/misc/diag_ipc_bridge.c | 90 +++++++++++++++++------------- 1 file changed, 52 insertions(+), 38 deletions(-) diff --git a/drivers/usb/misc/diag_ipc_bridge.c b/drivers/usb/misc/diag_ipc_bridge.c index a487f7c458ed..94b5775cd954 100644 --- a/drivers/usb/misc/diag_ipc_bridge.c +++ b/drivers/usb/misc/diag_ipc_bridge.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2011-2015, 2018, The Linux Foundation. All rights reserved. +/* Copyright (c) 2011-2015, 2018-2019, The Linux Foundation. All rights reserved. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 and @@ -10,9 +10,6 @@ * GNU General Public License for more details. */ -/* add additional information to our printk's */ -#define pr_fmt(fmt) "%s: " fmt "\n", __func__ - #include #include #include @@ -81,31 +78,32 @@ int diag_bridge_open(int id, struct diag_bridge_ops *ops) struct diag_bridge *dev; if (id < 0 || id >= MAX_BRIDGE_DEVS) { - pr_err("Invalid device ID"); + pr_err("%s: Invalid device ID\n", __func__); return -ENODEV; } dev = __dev[id]; if (!dev) { - pr_err("dev is null"); + pr_err("%s: dev is null\n", __func__); return -ENODEV; } if (dev->ops) { - pr_err("bridge already opened"); + pr_err("%s: bridge already opened\n", __func__); return -EALREADY; } mutex_lock(&dev->ifc_mutex); if (dev->opened) { mutex_unlock(&dev->ifc_mutex); - pr_err("Bridge already opened"); + pr_err("%s: Bridge already opened\n", __func__); return -EBUSY; } dev->opened = true; mutex_unlock(&dev->ifc_mutex); + dev_dbg(&dev->ifc->dev, "%s\n", __func__); dev->ops = ops; dev->err = 0; @@ -135,6 +133,7 @@ static void diag_bridge_delete(struct kref *kref) struct diag_bridge *dev = container_of(kref, struct diag_bridge, kref); int id = dev->id; + dev_dbg(&dev->ifc->dev, "%s\n", __func__); usb_put_dev(dev->udev); __dev[id] = 0; kfree(dev); @@ -145,32 +144,32 @@ void diag_bridge_close(int id) struct diag_bridge *dev; if (id < 0 || id >= MAX_BRIDGE_DEVS) { - pr_err("Invalid device ID"); + pr_err("%s: Invalid device ID\n", __func__); return; } dev = __dev[id]; if (!dev) { - pr_err("dev is null"); + pr_err("%s: dev is null\n", __func__); return; } if (id == DIAG_BRIDGE && !dev->ops) { - pr_err("can't close bridge that was not open"); + pr_err("%s: can't close bridge that was not open\n", __func__); return; } mutex_lock(&dev->ifc_mutex); if (!dev->opened) { mutex_unlock(&dev->ifc_mutex); - pr_err("Bridge not opened"); + pr_err("%s: Bridge not opened\n", __func__); return; } dev->opened = false; mutex_unlock(&dev->ifc_mutex); - dev_dbg(&dev->ifc->dev, "%s:\n", __func__); + dev_dbg(&dev->ifc->dev, "%s\n", __func__); usb_kill_anchored_urbs(&dev->submitted); dev->ops = 0; @@ -230,32 +229,33 @@ int diag_bridge_read(int id, char *data, int size) int ret; if (id < 0 || id >= MAX_BRIDGE_DEVS) { - pr_err("Invalid device ID"); + pr_err("%s: Invalid device ID\n", __func__); return -ENODEV; } - pr_debug("reading %d bytes", size); + pr_debug("%s: reading %d bytes\n", __func__, size); dev = __dev[id]; if (!dev) { - pr_err("device is disconnected"); + pr_err("%s: device is disconnected\n", __func__); return -ENODEV; } mutex_lock(&dev->read_mutex); if (!dev->ifc) { + pr_err("%s: device is disconnected\n", __func__); ret = -ENODEV; goto error; } if (id == DIAG_BRIDGE && !dev->ops) { - pr_err("bridge is not open"); + pr_err("%s: bridge is not open\n", __func__); ret = -ENODEV; goto error; } if (!size) { - dev_dbg(&dev->ifc->dev, "invalid size:%d\n", size); + dev_err(&dev->ifc->dev, "invalid size:%d\n", size); dev->drop_count++; ret = -EINVAL; goto error; @@ -263,6 +263,8 @@ int diag_bridge_read(int id, char *data, int size) /* if there was a previous unrecoverable error, just quit */ if (id == DIAG_BRIDGE && dev->err) { + pr_err("%s: EPROTO error occurred, or device disconnected\n", + __func__); ret = -ENODEV; goto error; } @@ -271,14 +273,15 @@ int diag_bridge_read(int id, char *data, int size) urb = usb_alloc_urb(0, GFP_KERNEL); if (!urb) { - dev_dbg(&dev->ifc->dev, "unable to allocate urb\n"); + dev_err(&dev->ifc->dev, "unable to allocate urb\n"); ret = -ENOMEM; goto put_error; } ret = usb_autopm_get_interface(dev->ifc); if (ret < 0 && ret != -EAGAIN && ret != -EACCES) { - pr_err_ratelimited("read: autopm_get failed:%d", ret); + pr_err_ratelimited("%s: read: autopm_get failed:%d\n", + __func__, ret); goto free_error; } @@ -297,7 +300,8 @@ int diag_bridge_read(int id, char *data, int size) ret = usb_submit_urb(urb, GFP_KERNEL); if (ret) { - pr_err_ratelimited("submitting urb failed err:%d", ret); + pr_err_ratelimited("%s: submitting urb failed err:%d\n", + __func__, ret); dev->pending_reads--; usb_unanchor_urb(urb); usb_autopm_put_interface(dev->ifc); @@ -344,7 +348,7 @@ static void diag_bridge_write_cb(struct urb *urb) struct diag_bridge *dev = urb->context; struct diag_bridge_ops *cbs = dev->ops; - dev_dbg(&dev->ifc->dev, "%s:\n", __func__); + dev_dbg(&dev->ifc->dev, "%s\n", __func__); usb_autopm_put_interface_async(dev->ifc); @@ -381,26 +385,27 @@ int diag_bridge_write(int id, char *data, int size) int ret; if (id < 0 || id >= MAX_BRIDGE_DEVS) { - pr_err("Invalid device ID"); + pr_err("%s: Invalid device ID\n", __func__); return -ENODEV; } - pr_debug("writing %d bytes", size); + pr_debug("%s: writing %d bytes\n", __func__, size); dev = __dev[id]; if (!dev) { - pr_err("device is disconnected"); + pr_err("%s: device is disconnected\n", __func__); return -ENODEV; } mutex_lock(&dev->write_mutex); if (!dev->ifc) { + pr_err("%s: device is disconnected\n", __func__); ret = -ENODEV; goto error; } if (id == DIAG_BRIDGE && !dev->ops) { - pr_err("bridge is not open"); + pr_err("%s: bridge is not open\n", __func__); ret = -ENODEV; goto error; } @@ -413,6 +418,8 @@ int diag_bridge_write(int id, char *data, int size) /* if there was a previous unrecoverable error, just quit */ if (id == DIAG_BRIDGE && dev->err) { + pr_err("%s: EPROTO error occurred, or device disconnected\n", + __func__); ret = -ENODEV; goto error; } @@ -428,7 +435,8 @@ int diag_bridge_write(int id, char *data, int size) ret = usb_autopm_get_interface(dev->ifc); if (ret < 0 && ret != -EAGAIN && ret != -EACCES) { - pr_err_ratelimited("write: autopm_get failed:%d", ret); + pr_err_ratelimited("%s: write: autopm_get failed:%d\n", + __func__, ret); goto free_error; } @@ -441,7 +449,8 @@ int diag_bridge_write(int id, char *data, int size) ret = usb_submit_urb(urb, GFP_KERNEL); if (ret) { - pr_err_ratelimited("submitting urb failed err:%d", ret); + pr_err_ratelimited("%s: submitting urb failed err:%d\n", + __func__, ret); dev->pending_writes--; usb_unanchor_urb(urb); usb_autopm_put_interface(dev->ifc); @@ -584,15 +593,17 @@ diag_bridge_probe(struct usb_interface *ifc, const struct usb_device_id *id) struct usb_endpoint_descriptor *ep_desc; int i, devid, ret = -ENOMEM; - pr_debug("id:%lu", id->driver_info); + pr_debug("%s: id:%lu\n", __func__, id->driver_info); devid = id->driver_info & 0xFF; - if (devid < 0 || devid >= MAX_BRIDGE_DEVS) + if (devid < 0 || devid >= MAX_BRIDGE_DEVS) { + pr_err("%s: Invalid device ID\n", __func__); return -ENODEV; + } /* already probed? */ if (__dev[devid]) { - pr_err("Diag device already probed"); + pr_err("%s: Diag device already probed\n", __func__); return -ENODEV; } @@ -629,7 +640,8 @@ diag_bridge_probe(struct usb_interface *ifc, const struct usb_device_id *id) } if (!(dev->in_epAddr && dev->out_epAddr)) { - pr_err("could not find bulk in and bulk out endpoints"); + pr_err("%s: could not find bulk in and bulk out endpoints\n", + __func__); ret = -ENODEV; goto error; } @@ -640,14 +652,16 @@ diag_bridge_probe(struct usb_interface *ifc, const struct usb_device_id *id) dev->pdev = platform_device_register_simple("diag_bridge", devid, NULL, 0); if (IS_ERR(dev->pdev)) { - pr_err("unable to allocate platform device"); + pr_err("%s: unable to allocate platform device\n", + __func__); ret = PTR_ERR(dev->pdev); goto error; } } else { dev->pdev = platform_device_alloc("ipc_bridge", -1); if (!dev->pdev) { - pr_err("unable to allocate platform device"); + pr_err("%s: unable to allocate platform device\n", + __func__); ret = -ENOMEM; goto error; } @@ -655,13 +669,13 @@ diag_bridge_probe(struct usb_interface *ifc, const struct usb_device_id *id) ret = platform_device_add_data(dev->pdev, &ipc_bridge_pdata, sizeof(struct ipc_bridge_platform_data)); if (ret) { - pr_err("fail to add pdata"); + pr_err("%s: fail to add pdata\n", __func__); goto put_pdev; } ret = platform_device_add(dev->pdev); if (ret) { - pr_err("fail to add pdev"); + pr_err("%s: fail to add pdev\n", __func__); goto put_pdev; } } @@ -687,7 +701,7 @@ static void diag_bridge_disconnect(struct usb_interface *ifc) { struct diag_bridge *dev = usb_get_intfdata(ifc); - dev_dbg(&dev->ifc->dev, "%s:\n", __func__); + dev_dbg(&dev->ifc->dev, "%s\n", __func__); platform_device_unregister(dev->pdev); diag_bridge_debugfs_cleanup(); @@ -815,7 +829,7 @@ static int __init diag_bridge_init(void) ret = usb_register(&diag_bridge_driver); if (ret) { - pr_err("unable to register diag driver"); + pr_err("%s: unable to register diag driver\n", __func__); return ret; } From 64287585dcf7d61bd0c1d73c2c8ffffd1b8ee062 Mon Sep 17 00:00:00 2001 From: Ajay Agarwal Date: Tue, 8 Jan 2019 11:10:18 +0530 Subject: [PATCH 2/2] 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); }