Messages in this thread | | | Subject | Re: [PATCH v2 2/5] usb: xhci: Fix potential memory leak in xhci_disable_slot() | From | Mathias Nyman <> | Date | Wed, 9 Aug 2017 13:09:47 +0300 |
| |
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
| |