From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753034AbdLHQ7R (ORCPT ); Fri, 8 Dec 2017 11:59:17 -0500 Received: from mga01.intel.com ([192.55.52.88]:18813 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752530AbdLHQ7O (ORCPT ); Fri, 8 Dec 2017 11:59:14 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,378,1508828400"; d="scan'208,223";a="10249618" Subject: Re: [PATCH] usb-core: Fix potential null pointer dereference in xhci-debugfs.c To: Alexander Kappner , Mathias Nyman , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Lu Baolu References: <1512638774-6837-1-git-send-email-agk@godking.net> <98502b84-1a6e-fd18-f290-ab90f0082e55@godking.net> From: Mathias Nyman Message-ID: <3c3a199a-8c73-41c8-d105-33199bfb92dc@linux.intel.com> Date: Fri, 8 Dec 2017 19:01:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <98502b84-1a6e-fd18-f290-ab90f0082e55@godking.net> Content-Type: multipart/mixed; boundary="------------0402EB926771CE563FE2864F" Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is a multi-part message in MIME format. --------------0402EB926771CE563FE2864F Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 08.12.2017 13:06, Alexander Kappner wrote: > Hi, > >> I think we need to dig a bit deeper. It's good to check if spriv is >> valid >> but there are probably other reasons than kzalloc failing. > > I agree -- this small allocation is unlikely to fail in practice. > Also, while my patch prevents the kernel oops, it also prevents the > debugfs entries from being created. > > I've been debugging this more trying to come up with a better > solution, but I might need some guidance as I'm not too familiar with > the USB subsystem. The immediate cause of the crash was usbmuxd > sending a USBDEVFS_SETCONFIGURATION ioctl to a device, which _only if > it fails_ calls usb_hcd_alloc_bandwidth to try and reset the device, > which in turn calls xhci_debugfs_create_endpoint. The ioctl handler > acquires a device-specific lock via usb_lock_device. > > When the system resumed from hibernate, xhci_resume was called. This > in turn called xhci_mem_cleanup to deallocate the device structures, > which include setting the debugfs_private pointer to NULL (via > xhci_free_virt_devices_depth_first). It thus seems likely that the > ioctl is somehow racing with the hibernate. The call to xhci_resume > is protected by a host-controller specific lock (xhci->lock) but it > doesn't attempt to take the usb_lock_device device-specific lock. > > Now my suspicion is that xhci_resume freed and zeroed the device > structures while racing with the ioctl handler. I can't seem to find > any exclusion mechanism that would prevent xhci_resume from racing > with the USBDEVFS_SETCONFIGURATION ioctl (or any other ioctl, for > that matter). Am I missing something? If not, is there any reason why > an ioctl might need to execute in parallel with the xhci_resume? If > not, can we just do a busy wait in xhci_resume until all pending > ioctls have returned? I'm not sure, but if I recall correctly then power management is supposed to make sure a driver doesn't access usb devices while the host controller is still resuming. The odd thing here is that xhci_debugfs_remove_slot(xhci, slot_id), and xhci_free_virt_device(xhci, slot_id) are called together when xhci_mem_cleanup() calls xhci_free_virt_devices_depth_first() That means both the xhci_virt_device *dev and dev->debugfs_private should both be freed and xhci->devs[slot_id] set NULL for that virt_device. so xhci_add_endpoint() should fail a lot earlier because the xhci->devs[slot_id] should be a null pointer as well. Allocation is also done together in xhci_alloc_dev() Looking at it more closely there is actually the .free_dev callback that first frees the dev->debugs_private but the virt_dev is only freed conditionally later Attached a patch that frees them together, can you try it out? If it doesn't help we need to add some elaborate tracing Thanks -Mathias --------------0402EB926771CE563FE2864F Content-Type: text/x-patch; name="0001-xhci-Only-free-xhci_virt_device-debugfs_private-if-d.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-xhci-Only-free-xhci_virt_device-debugfs_private-if-d.pa"; filename*1="tch" >>From 811aad86287e78879b7e7f28e0c266bcd2a2f73e Mon Sep 17 00:00:00 2001 From: Mathias Nyman Date: Fri, 8 Dec 2017 18:50:18 +0200 Subject: [PATCH] xhci: Only free xhci_virt_device->debugfs_private if device is freed freeing device is conditional, we otherwise might end up with a xhci virt_device that doesnt have a valid debugs_private pointer. For testing only Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 2424d30..da6dbe3 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -3525,8 +3525,6 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) struct xhci_slot_ctx *slot_ctx; int i, ret; - xhci_debugfs_remove_slot(xhci, udev->slot_id); - #ifndef CONFIG_USB_DEFAULT_PERSIST /* * We called pm_runtime_get_noresume when the device was attached. @@ -3555,8 +3553,10 @@ static void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev) } ret = xhci_disable_slot(xhci, udev->slot_id); - if (ret) + if (ret) { + xhci_debugfs_remove_slot(xhci, udev->slot_id); xhci_free_virt_device(xhci, udev->slot_id); + } } int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id) -- 2.7.4 --------------0402EB926771CE563FE2864F--