linux-kernel - Re: [PATCH v3] usb: core: hub: hub_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5720D1F8.6030809@linux.intel.com>
Date:	Wed, 27 Apr 2016 17:51:36 +0300
From:	Mathias Nyman <mathias.nyman@...ux.intel.com>
To:	Chris Bainbridge <chris.bainbridge@...il.com>,
	gregkh@...uxfoundation.org
CC:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: core: hub: hub_port_init lock controller instead
 of bus

On 25.04.2016 15:48, Chris Bainbridge wrote:
> The XHCI controller presents two USB buses to the system - one for USB2
> and one for USB3. The hub init code (hub_port_init) is reentrant but
> only locks one bus per thread, leading to a race condition failure when
> two threads attempt to simultaneously initialise a USB2 and USB3 device:
>
> [    8.034843] xhci_hcd 0000:00:14.0: Timeout while waiting for setup device command
> [   13.183701] usb 3-3: device descriptor read/all, error -110
>
> On a test system this failure occurred on 6% of all boots.
>
> The call traces at the point of failure are:
>
> Call Trace:
>   [<ffffffff81b9bab7>] schedule+0x37/0x90
>   [<ffffffff817da7cd>] usb_kill_urb+0x8d/0xd0
>   [<ffffffff8111e5e0>] ? wake_up_atomic_t+0x30/0x30
>   [<ffffffff817dafbe>] usb_start_wait_urb+0xbe/0x150
>   [<ffffffff817db10c>] usb_control_msg+0xbc/0xf0
>   [<ffffffff817d07de>] hub_port_init+0x51e/0xb70
>   [<ffffffff817d4697>] hub_event+0x817/0x1570
>   [<ffffffff810f3e6f>] process_one_work+0x1ff/0x620
>   [<ffffffff810f3dcf>] ? process_one_work+0x15f/0x620
>   [<ffffffff810f4684>] worker_thread+0x64/0x4b0
>   [<ffffffff810f4620>] ? rescuer_thread+0x390/0x390
>   [<ffffffff810fa7f5>] kthread+0x105/0x120
>   [<ffffffff810fa6f0>] ? kthread_create_on_node+0x200/0x200
>   [<ffffffff81ba183f>] ret_from_fork+0x3f/0x70
>   [<ffffffff810fa6f0>] ? kthread_create_on_node+0x200/0x200
>
> Call Trace:
>   [<ffffffff817fd36d>] xhci_setup_device+0x53d/0xa40
>   [<ffffffff817fd87e>] xhci_address_device+0xe/0x10
>   [<ffffffff817d047f>] hub_port_init+0x1bf/0xb70
>   [<ffffffff811247ed>] ? trace_hardirqs_on+0xd/0x10
>   [<ffffffff817d4697>] hub_event+0x817/0x1570
>   [<ffffffff810f3e6f>] process_one_work+0x1ff/0x620
>   [<ffffffff810f3dcf>] ? process_one_work+0x15f/0x620
>   [<ffffffff810f4684>] worker_thread+0x64/0x4b0
>   [<ffffffff810f4620>] ? rescuer_thread+0x390/0x390
>   [<ffffffff810fa7f5>] kthread+0x105/0x120
>   [<ffffffff810fa6f0>] ? kthread_create_on_node+0x200/0x200
>   [<ffffffff81ba183f>] ret_from_fork+0x3f/0x70
>   [<ffffffff810fa6f0>] ? kthread_create_on_node+0x200/0x200
>
> Which results from the two call chains:
>
> hub_port_init
>   usb_get_device_descriptor
>    usb_get_descriptor
>     usb_control_msg
>      usb_internal_control_msg
>       usb_start_wait_urb
>        usb_submit_urb / wait_for_completion_timeout / usb_kill_urb
>
> hub_port_init
>   hub_set_address
>    xhci_address_device
>     xhci_setup_device
>
> Mathias Nyman explains the current behaviour violates the XHCI spec:
>
>   hub_port_reset() will end up moving the corresponding xhci device slot
>   to default state.
>
>   As hub_port_reset() is called several times in hub_port_init() it
>   sounds reasonable that we could end up with two threads having their
>   xhci device slots in default state at the same time, which according to
>   xhci 4.5.3 specs still is a big no no:
>
>   "Note: Software shall not transition more than one Device Slot to the
>    Default State at a time"
>
>   So both threads fail at their next task after this.
>   One fails to read the descriptor, and the other fails addressing the
>   device.
>
> Fix this in hub_port_init by locking the USB controller (instead of an
> individual bus) to prevent simultaneous initialisation of both buses.
>
> Fixes: 638139eb95d2 ("usb: hub: allow to process more usb hub events in parallel")
> Link: https://lkml.org/lkml/2016/2/8/312
> Link: https://lkml.org/lkml/2016/2/4/748
> Signed-off-by: Chris Bainbridge <chris.bainbridge@...il.com>
> ---

Acked-by: Mathias Nyman <mathias.nyman@...ux.intel.com>