linux-kernel - Re: Should xhci_irq() call usb_hc

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]

Date:   Mon, 12 Dec 2016 12:48:48 +0200
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Mathias Nyman <mathias.nyman@...el.com>
Cc:     Alan Stern <stern@...land.harvard.edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: Should xhci_irq() call usb_hc_died()?

On 12.12.2016 10:43, Felipe Balbi wrote:
>
> Hi,
>
> Bjorn Helgaas <helgaas@...nel.org> writes:
>> Hi Mathias,
>>
>> ehci_irq(), ohci_irq(), fotg210_irq(), and oxu210_hcd_irq() contain code
>> equivalent to this:
>>
>>    status = ehci_readl(...);
>>    if (status == ~(u32) 0) {
>>      ...
>>      usb_hc_died(hcd);
>>      ...
>>      return IRQ_HANDLED;
>>    }
>>
>> xhci_irq() has a similar check, but does not call usb_hc_died():
>>
>>    status = readl(...);
>>    if (status = 0xffffffff) {
>>      ...
>>      return IRQ_HANDLED;
>>    }
>>
>> Should xhci_irq() also call usb_hc_died()?  Maybe there's some reason
>> for it to be different than the others, but it wasn't obvious to this
>> casual observer :)

It probably should, I'm not aware of any reason why not, and a quick look at the
logs didn't reveal anything.

Currently we are calling usb_hcd_died() in a couple of timeout cases if we read
0xffffffff from the pci registers, So eventually usb_hc_died() will be called.

I'll take a look at this in more detail

>
> you might just have fixed several bugs in dealing with a dead HC :-)
>
> Can you provide a patch? (well, unless Mathias has a strong reason not
> to call usb_hc_died(), of course).

I don't think this is the worst case, there are a couple of other reasons such as
normal pci remove case we halt the host and reset the hardware after first HCD (USB2)
is removed, with all the secondary HCD (USB3) sand all its devices still connected,

Or then the abnormal case where HC disappears, we may time out while giving back a
killed URB, and  may end up never returning it. USB core waits with the roothub device
lock held for the URB, and we try to tear down xhci, which also requires the roothub
device lock at some point -> deadlock.

I'm am looking at these, but I need to make sure i fix it properly and not cause even
more issues.

-Mathias