linux-kernel - Re: FW: RE: Re: Subject: [PATCH v3] 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 1 Mar 2017 10:15:44 -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: FW: RE: Re: Subject: [PATCH v3] USB:Core: BugFix: Proper handling
 of Race Condition when two USB class drivers try to call init_usb_class
 simultaneously

On Wed, 1 Mar 2017, Ajay Kaher wrote:

> > On Mon, 22 Feb 2017, Ajay Kaher wrote:
> > 
> >> On Mon, 20 Feb 2017, Ajay Kaher wrote:
> >> 
> >>> Alan, as per my understanding I have shifted the lock from
> >>> release_usb_class() to destroy_usb_class() in patch v3. 
> >>> If it is not right, please explain in detail which race condition
> >>> I have missed and also share your suggestions.
> >>> 
> >> 
> >> Have you considered what would happen if destroy_usb_class() ran, but 
> >> some other CPU was still holding a reference to usb_class?  And what if 
> >> the last reference gets dropped later on, while init_usb_class() is 
> >> running?
> > 
> > Access of usb_class->kref is only from either init_usb_class()
> > or destroy_usb_class(), and both these functions are now protected
> > with Mutex Locking in patch v3, so there is no chance of race condition
> > as per above scenarios.
> > 
> >> Maybe that's not possible here, but it is possible in general for 
> >> refcounted objects.  So yes, this code is probably okay, but it isn't 
> >> good form.
> > 
> > As per my understanding, I found to be one of the best possible solution
> > for this problem and this solutiuon don't have any side effect.
> 
> Alan, I had shared modified Patch v3 as per your inputs to prevent
> the race condition during simultaneously calling of init_usb_class().
> If you think there is scope to improve the patch, please share your inputs.

Under the circumstances, your patch is acceptable.

If you really want to make the point crystal clear, you could replace 
usb_class->kref with an ordinary integer counter.  Then it would be 
obvious that there are no references other than the ones taken by 
init_usb_class() and released by destroy_usb_class().

Alan Stern

> thanks,
> ajay kaher
> 
> 
> Signed-off-by: Ajay Kaher
> 
> ---
> 
>  drivers/usb/core/file.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index 822ced9..a12d184 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -27,6 +27,7 @@
>  #define MAX_USB_MINORS 256
>  static const struct file_operations *usb_minors[MAX_USB_MINORS];
>  static DECLARE_RWSEM(minor_rwsem);
> +static DEFINE_MUTEX(init_usb_class_mutex);
> 
>  static int usb_open(struct inode *inode, struct file *file)
>  {
> @@ -109,8 +110,10 @@ static void release_usb_class(struct kref *kref)
> 
>  static void destroy_usb_class(void)
>  {
> +       mutex_lock(&init_usb_class_mutex);
>         if (usb_class)
>                 kref_put(&usb_class->kref, release_usb_class);
> +       mutex_unlock(&init_usb_class_mutex);
>  }
> 
>  int usb_major_init(void)
> @@ -171,7 +174,10 @@ int usb_register_dev(struct usb_interface *intf,
>         if (intf->minor >= 0)
>                 return -EADDRINUSE;
> 
> +       mutex_lock(&init_usb_class_mutex);
>         retval = init_usb_class();
> +       mutex_unlock(&init_usb_class_mutex);
> +
>         if (retval)
>                 return retval;
> 
>  
>