linux-kernel - RE: Re: Re: Re: Subject: [PATCH v1] USB:Core: BugFix: Proper handling of Race Condition when two USB class drivers try to call init_usb

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 14 Feb 2017 10:39:07 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Ajay Kaher <ajay.kaher@...sung.com>
cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        AMAN DEEP <aman.deep@...sung.com>,
        HEMANSHU SRIVASTAVA <hemanshu.s@...sung.com>
Subject: RE: Re: Re: Re: Subject: [PATCH v1] USB:Core: BugFix: Proper handling
 of Race Condition when two USB class drivers try to call init_usb_class
 simultaneously

On Thu, 2 Feb 2017, Ajay Kaher wrote:

> >>>> At boot time, probe function of multiple connected devices
> >>>> (proprietary devices) execute simultaneously.
> >>>
> >>> What exactly do you mean here?  How can probe happen "simultaneously"?
> >>> The USB core prevents this, right?
> >> 
> >> I have observed two scenarios to call probe function:
> >> 
> >> Scenario #1: Driver inserted and attaching USB Device:
> >> Yes, you are right, two probes at same time is not happening
> >> in this scenario.
> >> 
> >> Scenario #2: USB Device attached and inserting Driver:
> >> In this case probe has been called in context of insmod,
> >> refer following code flow:
> >> init -> usb_register_driver -> driver_register -> bus_add_driver ->
> >> driver_attach -> bus_for_each_dev -> __driver_attach ->
> >> driver_probe_device -> usb_probe_interface -> probe -> usb_register_dev
> >> 
> >> I have observed the crash in Scenario #2, as two probes executes at
> >> same time in this scenario. And init_usb_class_mutex lock require to
> >> prevent race condition.
> > 
> > What about the fact that in __driver_attach() we call device_lock() so
> > that probe never gets called at the same time for the same device?
> 
> Devices are different.
> 
> > Or are you saying that you can load multiple USB modules at the same
> > time?  If so, how is insmod running on multiple cpus at the same time?
> > I thought we had a global lock there to prevent that from happening
> > (i.e. only one module can be loaded at a time.)  Or is that what has
> > recently changed?
> 
> Yes, we can load multiple USB modules at the same time using insmod.
> Tested on ARM Architecture with Linux kernel 4.1.10, that we can have 
> multiple insmod on multiple cpus at same time. Also reviewed load_module and 
> do_init_module functions and couldn't find any global lock.
> 
> > 
> > What is causing your modules to be loaded from userspace?  What type of
> > device is this happening for?  And why haven't we seen this before?
> > What kernel versions have you had a problem with this?
> 
> Earlier we execute insmod in foreground as "insmod aaa.ko ; insmod bbb.ko"
> and that's why insmod executed sequentially. Now we modified to execute in 
> parallel/background as "insmod aaa.ko & ; insmod bbb.ko &". 
> 
> > And what for what drivers specifically?
> 
> This problem observed for drivers in which usb_register_dev called from their
> probe function, and there are many such drivers.
> 
> As per my opinion, usb_class structure is global and allocated + initialized
> in usb_register_dev->init_usb_class function. Also as per scenario #2
> concurrency is possible, so protection using init_usb_class_mutex lock requires.
> Don't you think so?
> 
> >>>> And because of the following code path race condition happens:
> >>>> probe->usb_register_dev->init_usb_class
> >>>
> >>> Why is this just showing up now, and hasn't been an issue for the decade
> >>> or so this code has been around?  What changed?
> >>>
> >>>> Tested with these changes, and problem has been solved.
> >>>
> >>> What changes?
> >> 
> >> Tested with my patch (i.e. locking with init_usb_class_mutex).
> > 
> > I don't see a patch here :(
> 
> Sorry, appending the patch again with this mail.
>  
> thanks,
>  
> ajay kaher
> 
> 
> Signed-off-by: Ajay Kaher

I think Ajay's argument is correct and a patch is needed.  But this
patch misses the race between init_usb_class() and release_usb_class().  

The basic problem is that reference counting doesn't work when you try
to use the same global pointer (usb_class) to refer to multiple
generations of a dynamically allocated entity.  We had the same sort of
problem many years ago with the usb_interface structure (and we
ultimately fixed it by creating a separate usb_interface_cache
structure).

The best approach here would be to forget about all the reference
counting.  Get rid of usb_class entirely, and create the "usbmisc"
class structure just once, when usbcore initializes.  Or, if you
prefer, use a mutex to protect a routine that allocates the class
structure dynamically, just once.  Either way, don't deallocate it
until usbcore is unloaded.

Alan Stern