From patchwork Fri May 11 16:16:30 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mathias Nyman X-Patchwork-Id: 10394653 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A14D8601A0 for ; Fri, 11 May 2018 16:14:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4F20928F46 for ; Fri, 11 May 2018 16:14:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 43D7A28F4C; Fri, 11 May 2018 16:14:13 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A045628F46 for ; Fri, 11 May 2018 16:14:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751478AbeEKQOL (ORCPT ); Fri, 11 May 2018 12:14:11 -0400 Received: from mga02.intel.com ([134.134.136.20]:64313 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751398AbeEKQOL (ORCPT ); Fri, 11 May 2018 12:14:11 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 May 2018 09:14:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,389,1520924400"; d="scan'208";a="50097297" Received: from mattu-haswell.fi.intel.com (HELO [10.237.72.164]) ([10.237.72.164]) by orsmga003.jf.intel.com with ESMTP; 11 May 2018 09:14:09 -0700 Subject: Re: usbcore: NULL pointer dereference after detaching USB disk with linux 4.17 To: Jordan Glover Cc: "linux-usb@vger.kernel.org" , Alan Stern References: <7793ce1f-727a-f903-a84b-30c51ebe1e4e@linux.intel.com> <31rlqjJ8GD2M7ENwuc-uxGdhI3fhnVxe7-P78Y8QTXOUw7ONVVxHOJYlUwEoSqWUZReEhMvqdUvqipLHH5qcYl3n7au8mhKl7d9Xg0tsXbY=@protonmail.ch> From: Mathias Nyman Message-ID: <27403844-e1be-a987-323d-584163e21d76@linux.intel.com> Date: Fri, 11 May 2018 19:16:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <31rlqjJ8GD2M7ENwuc-uxGdhI3fhnVxe7-P78Y8QTXOUw7ONVVxHOJYlUwEoSqWUZReEhMvqdUvqipLHH5qcYl3n7au8mhKl7d9Xg0tsXbY=@protonmail.ch> Content-Language: en-US Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 11.05.2018 15:11, Jordan Glover wrote: > On May 11, 2018 1:46 PM, Mathias Nyman wrote: > >> Hi >> >> On 10.05.2018 14:49, Jordan Glover wrote: >> >>> Hello, >>> >>> Detaching plugged external usb disk with: "udisksctl power-off --block-device " causes NULL pointer dereference and kernel hang. Tested with 4.17-rc4 on Manjaro Linux config and my own custom config with two different usb disks. It doesn't happen with 4.16.x. Below are logs registered with my own kernel config: >> >> I'm able to reproduce this. >> >>> udisksd[1375]: Successfully sent SCSI command SYNCHRONIZE CACHE to /dev/sda >>> >>> udisksd[1375]: Successfully sent SCSI command START STOP UNIT to /dev/sda >>> >>> kernel: sd 0:0:0:0: [sda] Synchronizing SCSI cache >>> >>> kernel: sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result: hostbyte=DID_ERROR driverbyte=DRIVER_OK >>> >>> upowerd[1387]: unhandled action 'unbind' on /sys/devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.0 >>> >>> laptop udisksd[1375]: Powered off /dev/sda - successfully wrote to sysfs path /sys/devices/pci0000:00/0000:00:14.0/usb2/2-3/remove >>> >>> kernel: usb 2-3: USB disconnect, device number 2 >>> >>> kernel: BUG: unable to handle kernel NULL pointer dereference at 000000000000001c >> >>> kernel: RIP: 0010:xhci_hub_control+0x1ee5/0x1ff0 [xhci_hcd] >> >> looks like xhci issue, triggered by speed = xhci->devs[i]->udev->speed in >> >> xhci_find_slot_id_by_port() >> >> xhci->devs[i]->udev seems to be NULL, probably because of commit 44a182b9d177 >> >> ("xhci: Fix use-after-free in xhci_free_virt_device") >> >> That patch itself fixes another regression, I'll see igf there is a better solution >> >> Thanks >> >> -Mathias > > Uh, it's a relief. I was afraid being on my own with this. Looking forward for fix. Thank you. > Ok, I think I found the reason. Below details are mostly for myself to remember what's going on. Some testcode found last. It's a combination of USB3 devices lacking a real "disabled" link state, udisksctl power-off using the "remove" sysfs entry causing a logical disconnect, and xhci free_dev being async, not really freeing before returning. udisksctl power-off uses the "remove" sysfs entry, setting removed bit,calls logical_disaconnect and sets USB3 device to U3, and kicks hub thread. Hub thread calls usb_disconnect, and end by calling xhci_free_dev callback. xhci_free_dev returns before freeing anything. It queues xhci slot disabling commands, which when complete will free the slot (set xhci->devs[i] to NULL). Before xhci->devs[i] is set to NULL the hub thread notices the port is enabled, (U3) and will try to disable it once again, setting it to U3 again. xhci->devs[i]->udev will be NULL in 4.17-rc4 here, but we only check xhci->devs[i] before setting U3, and hence trigger the NULL pointer dereference. xhci->devs[i]->udev is a better, earlier indicator to check if xhci slot is about to go be disabled and freed, and simply checking it as well makes sense. Some usb core change in how we handle the whole thing might make sense, but to fix this, I think that just adding the below code should be enough for 4.17 -Mathias --- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 72ebbc9..32cd52c 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -354,7 +354,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci, slot_id = 0; for (i = 0; i < MAX_HC_SLOTS; i++) { - if (!xhci->devs[i]) + if (!xhci->devs[i] || !xhci->devs[i]->udev) continue; speed = xhci->devs[i]->udev->speed; if (((speed >= USB_SPEED_SUPER) == (hcd->speed >= HCD_USB3))