linux-kernel - Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit error

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:   Tue, 20 Nov 2018 08:18:18 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>,
        stern@...land.harvard.edu
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit
 error

On Mo, 2018-11-19 at 16:52 +0100, Nicolas Saenz Julienne wrote:
>  
> > >  /* USB 2.0 spec Section 11.24.2.3 */
> > > @@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub,
> > > enum hub_quiescing_type type)
> > >  	}
> > >  
> > >  	/* Stop hub_wq and related activity */
> > > +	del_timer_sync(&hub->irq_urb_retry);
> > 
> > That is a race condition. You kill the timer here, but the URB may
> > still be in flight. And if it fails, it will restart the error
> > handler. You have to introduce a flag or poison the URB.
> 
> I see, wouldn't checking "hub->quiescing" in hub_irq()'s submit error
> path do the work? Something the likes of this:
> 
> @@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
>   		return;
> 
>  	status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> -	if (status != 0 && status != -ENODEV && status != -EPERM)
> +	if (status != 0 && status != -ENODEV && status != -EPERM &&
> +	    status != -ESHUTDOWN && !hub->quiescing) {

The problem with doing such things is that the interrupt and the
disconnect can run on two different CPUs. Thus if you use a flag
you need to make sure they don't race and you need to get
memory ordering right.

Doable, but not easy.
There is a reason URBs can be poisoned.

	Regards
		Oliver