From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751659AbdLIWiv (ORCPT ); Sat, 9 Dec 2017 17:38:51 -0500 Received: from s18231873.onlinehome-server.info ([217.160.179.168]:54458 "EHLO godking.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751343AbdLIWit (ORCPT ); Sat, 9 Dec 2017 17:38:49 -0500 From: Alexander Kappner Subject: Re: [PATCH] usb-core: Fix potential null pointer dereference in xhci-debugfs.c To: Mathias Nyman , 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> <3c3a199a-8c73-41c8-d105-33199bfb92dc@linux.intel.com> Message-ID: Date: Sat, 9 Dec 2017 14:38:44 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.3.0 MIME-Version: 1.0 In-Reply-To: <3c3a199a-8c73-41c8-d105-33199bfb92dc@linux.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mathias, thanks for the patch! The system now resumes cleanly from hibernate even with usbmuxd doing its thing. Tested-by: Alexander Kappner While testing this I hit some other issues with xhci-debugfs.c but I'll write these up in a separate bug. On 12/08/2017 09:01 AM, Mathias Nyman wrote: > 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 > > From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: usb-core: Fix potential null pointer dereference in xhci-debugfs.c From: Alexander Kappner Message-Id: Date: Sat, 9 Dec 2017 14:38:44 -0800 To: Mathias Nyman , Mathias Nyman , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Lu Baolu List-ID: SGkgTWF0aGlhcywKCnRoYW5rcyBmb3IgdGhlIHBhdGNoISBUaGUgc3lzdGVtIG5vdyByZXN1bWVz IGNsZWFubHkgZnJvbSBoaWJlcm5hdGUgZXZlbiAKd2l0aCB1c2JtdXhkIGRvaW5nIGl0cyB0aGlu Zy4KClRlc3RlZC1ieTogQWxleGFuZGVyIEthcHBuZXIgPGFna0Bnb2RraW5nLm5ldD4KCldoaWxl IHRlc3RpbmcgdGhpcyBJIGhpdCBzb21lIG90aGVyIGlzc3VlcyB3aXRoIHhoY2ktZGVidWdmcy5j IGJ1dCBJJ2xsIAp3cml0ZSB0aGVzZSB1cCBpbiBhIHNlcGFyYXRlIGJ1Zy4KCk9uIDEyLzA4LzIw MTcgMDk6MDEgQU0sIE1hdGhpYXMgTnltYW4gd3JvdGU6Cj4gT24gMDguMTIuMjAxNyAxMzowNiwg QWxleGFuZGVyIEthcHBuZXIgd3JvdGU6Cj4+IEhpLAo+Pgo+Pj4gSSB0aGluayB3ZSBuZWVkIHRv IGRpZyBhIGJpdCBkZWVwZXIuIEl0J3MgZ29vZCB0byBjaGVjayBpZiBzcHJpdiBpcwo+Pj4gdmFs aWQKPj4+IGJ1dCB0aGVyZSBhcmUgcHJvYmFibHkgb3RoZXIgcmVhc29ucyB0aGFuIGt6YWxsb2Mg ZmFpbGluZy4KPj4KPj4gSSBhZ3JlZSAtLSB0aGlzIHNtYWxsIGFsbG9jYXRpb24gaXMgIHVubGlr ZWx5IHRvIGZhaWwgaW4gcHJhY3RpY2UuCj4+IEFsc28sIHdoaWxlIG15IHBhdGNoIHByZXZlbnRz IHRoZSBrZXJuZWwgb29wcywgaXQgYWxzbyBwcmV2ZW50cyB0aGUKPj4gZGVidWdmcyBlbnRyaWVz IGZyb20gYmVpbmcgY3JlYXRlZC4KPj4KPj4gSSd2ZSBiZWVuIGRlYnVnZ2luZyB0aGlzIG1vcmUg dHJ5aW5nIHRvIGNvbWUgdXAgd2l0aCBhIGJldHRlcgo+PiBzb2x1dGlvbiwgYnV0IEkgbWlnaHQg bmVlZCBzb21lIGd1aWRhbmNlIGFzIEknbSBub3QgdG9vIGZhbWlsaWFyIHdpdGgKPj4gdGhlIFVT QiBzdWJzeXN0ZW0uIFRoZSBpbW1lZGlhdGUgY2F1c2Ugb2YgdGhlIGNyYXNoIHdhcyB1c2JtdXhk Cj4+IHNlbmRpbmcgYSBVU0JERVZGU19TRVRDT05GSUdVUkFUSU9OIGlvY3RsIHRvIGEgZGV2aWNl LCB3aGljaCBfb25seSBpZgo+PiBpdCBmYWlsc18gY2FsbHMgdXNiX2hjZF9hbGxvY19iYW5kd2lk dGggdG8gdHJ5IGFuZCByZXNldCB0aGUgZGV2aWNlLAo+PiB3aGljaCBpbiB0dXJuIGNhbGxzIHho Y2lfZGVidWdmc19jcmVhdGVfZW5kcG9pbnQuIFRoZSBpb2N0bCBoYW5kbGVyCj4+IGFjcXVpcmVz IGEgZGV2aWNlLXNwZWNpZmljIGxvY2sgdmlhIHVzYl9sb2NrX2RldmljZS4KPj4KPj4gV2hlbiB0 aGUgc3lzdGVtIHJlc3VtZWQgZnJvbSBoaWJlcm5hdGUsIHhoY2lfcmVzdW1lIHdhcyBjYWxsZWQu IFRoaXMKPj4gaW4gdHVybiBjYWxsZWQgeGhjaV9tZW1fY2xlYW51cCB0byBkZWFsbG9jYXRlIHRo ZSBkZXZpY2Ugc3RydWN0dXJlcywKPj4gd2hpY2ggaW5jbHVkZSBzZXR0aW5nIHRoZSBkZWJ1Z2Zz X3ByaXZhdGUgcG9pbnRlciB0byBOVUxMICAodmlhCj4+IHhoY2lfZnJlZV92aXJ0X2RldmljZXNf ZGVwdGhfZmlyc3QpLiBJdCB0aHVzIHNlZW1zIGxpa2VseSB0aGF0IHRoZQo+PiBpb2N0bCBpcyBz b21laG93IHJhY2luZyB3aXRoIHRoZSBoaWJlcm5hdGUuIFRoZSBjYWxsIHRvIHhoY2lfcmVzdW1l Cj4+IGlzIHByb3RlY3RlZCBieSBhIGhvc3QtY29udHJvbGxlciBzcGVjaWZpYyBsb2NrICh4aGNp LT5sb2NrKSBidXQgaXQKPj4gZG9lc24ndCBhdHRlbXB0IHRvIHRha2UgdGhlIHVzYl9sb2NrX2Rl dmljZSBkZXZpY2Utc3BlY2lmaWMgbG9jay4KPj4KPj4gTm93IG15IHN1c3BpY2lvbiBpcyB0aGF0 IHhoY2lfcmVzdW1lIGZyZWVkIGFuZCB6ZXJvZWQgdGhlIGRldmljZQo+PiBzdHJ1Y3R1cmVzIHdo aWxlIHJhY2luZyB3aXRoIHRoZSBpb2N0bCBoYW5kbGVyLiBJIGNhbid0IHNlZW0gdG8gZmluZAo+ PiBhbnkgZXhjbHVzaW9uIG1lY2hhbmlzbSB0aGF0IHdvdWxkIHByZXZlbnQgeGhjaV9yZXN1bWUg ZnJvbSByYWNpbmcKPj4gd2l0aCB0aGUgVVNCREVWRlNfU0VUQ09ORklHVVJBVElPTiBpb2N0bCAo b3IgYW55IG90aGVyIGlvY3RsLCBmb3IKPj4gdGhhdCBtYXR0ZXIpLiBBbSBJIG1pc3Npbmcgc29t ZXRoaW5nPyBJZiBub3QsIGlzIHRoZXJlIGFueSByZWFzb24gd2h5Cj4+IGFuIGlvY3RsIG1pZ2h0 IG5lZWQgdG8gZXhlY3V0ZSBpbiBwYXJhbGxlbCB3aXRoIHRoZSB4aGNpX3Jlc3VtZT8gSWYKPj4g bm90LCBjYW4gd2UganVzdCBkbyBhIGJ1c3kgd2FpdCBpbiB4aGNpX3Jlc3VtZSB1bnRpbCBhbGwg cGVuZGluZwo+PiBpb2N0bHMgaGF2ZSByZXR1cm5lZD8KPgo+IEknbSBub3Qgc3VyZSwgYnV0IGlm IEkgcmVjYWxsIGNvcnJlY3RseSB0aGVuIHBvd2VyIG1hbmFnZW1lbnQgaXMgc3VwcG9zZWQKPiB0 byBtYWtlIHN1cmUgYSBkcml2ZXIgZG9lc24ndCBhY2Nlc3MgdXNiIGRldmljZXMgd2hpbGUgdGhl IGhvc3QgCj4gY29udHJvbGxlcgo+IGlzIHN0aWxsIHJlc3VtaW5nLgo+Cj4gVGhlIG9kZCB0aGlu ZyBoZXJlIGlzIHRoYXQKPiB4aGNpX2RlYnVnZnNfcmVtb3ZlX3Nsb3QoeGhjaSwgc2xvdF9pZCks IGFuZAo+IHhoY2lfZnJlZV92aXJ0X2RldmljZSh4aGNpLCBzbG90X2lkKSBhcmUgY2FsbGVkIHRv Z2V0aGVyIHdoZW4KPiB4aGNpX21lbV9jbGVhbnVwKCkgY2FsbHMgeGhjaV9mcmVlX3ZpcnRfZGV2 aWNlc19kZXB0aF9maXJzdCgpCj4KPiBUaGF0IG1lYW5zIGJvdGggdGhlIHhoY2lfdmlydF9kZXZp Y2UgKmRldiBhbmQgZGV2LT5kZWJ1Z2ZzX3ByaXZhdGUKPiBzaG91bGQgYm90aCBiZSBmcmVlZCBh bmQgeGhjaS0+ZGV2c1tzbG90X2lkXSBzZXQgTlVMTCBmb3IgdGhhdCAKPiB2aXJ0X2RldmljZS4K Pgo+IHNvIHhoY2lfYWRkX2VuZHBvaW50KCkgc2hvdWxkIGZhaWwgYSBsb3QgZWFybGllciBiZWNh dXNlIHRoZSAKPiB4aGNpLT5kZXZzW3Nsb3RfaWRdCj4gc2hvdWxkIGJlIGEgbnVsbCBwb2ludGVy IGFzIHdlbGwuCj4KPiBBbGxvY2F0aW9uIGlzIGFsc28gZG9uZSB0b2dldGhlciBpbiB4aGNpX2Fs bG9jX2RldigpCj4KPiBMb29raW5nIGF0IGl0IG1vcmUgY2xvc2VseSB0aGVyZSBpcyBhY3R1YWxs eSB0aGUgLmZyZWVfZGV2IGNhbGxiYWNrIHRoYXQKPiBmaXJzdCBmcmVlcyB0aGUgZGV2LT5kZWJ1 Z3NfcHJpdmF0ZSBidXQgdGhlIHZpcnRfZGV2IGlzIG9ubHkgZnJlZWQKPiBjb25kaXRpb25hbGx5 IGxhdGVyCj4KPiBBdHRhY2hlZCBhIHBhdGNoIHRoYXQgZnJlZXMgdGhlbSB0b2dldGhlciwgY2Fu IHlvdSB0cnkgaXQgb3V0Pwo+Cj4gSWYgaXQgZG9lc24ndCBoZWxwIHdlIG5lZWQgdG8gYWRkIHNv bWUgZWxhYm9yYXRlIHRyYWNpbmcKPgo+IFRoYW5rcwo+IC1NYXRoaWFzCj4KPgotLS0KVG8gdW5z dWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxpbnV4 LXVzYiIgaW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5v cmcKTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9yZy9tYWpvcmRv bW8taW5mby5odG1sCg==