lkml.org 
[lkml]   [2017]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot()
From
Date
On 27.07.2017 05:21, Lu Baolu wrote:
> xhci_disable_slot() allows the invoker to pass a command pointer
> as paramenter. Otherwise, it will allocate one. This will cause
> memory leak when a command structure was allocated inside of this
> function while queuing command trb fails. Another problem comes up
> when the invoker passed a command pointer, but xhci_disable_slot()
> frees it when it detects a dead host.
>
> This patch fixes these two problems by removing the command parameter
> from xhci_disable_slot().
>
> Fixes: f9e609b82479 ("usb: xhci: Add helper function xhci_disable_slot().")
> Cc: Guoqing Zhang <guoqing.zhang@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/usb/host/xhci-hub.c | 2 +-
> drivers/usb/host/xhci.c | 30 +++++++++---------------------
> drivers/usb/host/xhci.h | 3 +--
> 3 files changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 00721e8..c862d53 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -612,7 +612,7 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci,
> xhci_dbg(xhci, "Disable all slots\n");
> spin_unlock_irqrestore(&xhci->lock, *flags);
> for (i = 1; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
> - retval = xhci_disable_slot(xhci, NULL, i);
> + retval = xhci_disable_slot(xhci, i);
> if (retval)
> xhci_err(xhci, "Failed to disable slot %d, %d. Enter test mode anyway\n",
> i, retval);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e69073f..cb2461a 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -3519,11 +3519,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> struct xhci_virt_device *virt_dev;
> struct xhci_slot_ctx *slot_ctx;
> int i, ret;
> - struct xhci_command *command;
> -
> - command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
> - if (!command)
> - return;
>
> #ifndef CONFIG_USB_DEFAULT_PERSIST
> /*
> @@ -3539,10 +3534,8 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> /* If the host is halted due to driver unload, we still need to free the
> * device.
> */
> - if (ret <= 0 && ret != -ENODEV) {
> - kfree(command);
> + if (ret <= 0 && ret != -ENODEV)
> return;
> - }
>
> virt_dev = xhci->devs[udev->slot_id];
> slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->out_ctx);
> @@ -3554,22 +3547,21 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
> del_timer_sync(&virt_dev->eps[i].stop_cmd_timer);
> }
>
> - xhci_disable_slot(xhci, command, udev->slot_id);
> + xhci_disable_slot(xhci, udev->slot_id);
> /*
> * Event command completion handler will free any data structures
> * associated with the slot. XXX Can free sleep?
> */
> }
>
> -int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
> - u32 slot_id)
> +int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id)
> {
> + struct xhci_command *command;
> unsigned long flags;
> u32 state;
> int ret = 0;
>
> - if (!command)
> - command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
> + command = xhci_alloc_command(xhci, false, false, GFP_KERNEL);
> if (!command)
> return -ENOMEM;
>
> @@ -3588,7 +3580,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, struct xhci_command *command,
> slot_id);
> if (ret) {
> spin_unlock_irqrestore(&xhci->lock, flags);
> - xhci_dbg(xhci, "FIXME: allocate a command ring segment\n");
> + kfree(command);
> return ret;
> }
> xhci_ring_cmd_db(xhci);
> @@ -3663,6 +3655,8 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> return 0;
> }
>
> + xhci_free_command(xhci, command);
> +
> if ((xhci->quirks & XHCI_EP_LIMIT_QUIRK)) {
> spin_lock_irqsave(&xhci->lock, flags);
> ret = xhci_reserve_host_control_ep_resources(xhci);
> @@ -3698,18 +3692,12 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
> pm_runtime_get_noresume(hcd->self.controller);
> #endif
>
> -
> - xhci_free_command(xhci, command);
> /* Is this a LS or FS device under a HS hub? */
> /* Hub or peripherial? */
> return 1;
>
> disable_slot:
> - /* Disable slot, if we can do it without mem alloc */
> - kfree(command->completion);
> - command->completion = NULL;
> - command->status = 0;
> - return xhci_disable_slot(xhci, command, udev->slot_id);
> + return xhci_disable_slot(xhci, udev->slot_id);

avoiding allocating a new command after failing to allocate a virt dev was
probably the reason why xhci_disable_slot() supported a command
as a parameter in the first place.

But this is such a small corner case, and your patch fixes two more likely issues,
and simplifies the code a lot so I think It's well worth it

-Mathias

\
 
 \ /
  Last update: 2017-08-09 12:07    [W:0.132 / U:0.560 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site