linux-kernel - Re: Subject: [PATCH v4] 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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Mar 2017 04:35:37 +0000
From:   Ajay Kaher <ajay.kaher@...sung.com>
To:     Alan Stern <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "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: Subject: [PATCH v4] USB:Core: BugFix: Proper handling of Race
 Condition when two USB class drivers try to call init_usb_class simultaneously

 
 
 
> On Fri, 3 Mar 2017, Ajay Kaher wrote:
> 
> > > usb_class->kref is not accessible outside the file.c
> > > as usb_class is _static_ inside the file.c and
> > > pointer of usb_class->kref is not passed anywhere.
> > > 
> > > Hence as you wanted, there are no references of usb_class->kref
> > > other than taken by init_usb_class() and released by destroy_usb_class().
> > 
> > Verified the code again, I hope my last comments clarifed the things
> > which came in your mind and helps you to accept the patch :)
>  
> Your main point is that usb_class->kref is accessed from only two
> points, both of which are protected by the new mutex.  This means there
> is no reason for the value to be a struct kref at all.  You should
> change it to an int (and change its name).  Leaving it as a kref will
> make readers wonder why it needs to be updated atomically.

At many places in Linux kernel, instances of Kref have been used within
Mutex, SpinLock and don’t have any side effect.

Making to int and handle (i.e. get/put) it within file.c seems
not good as we have Kref. Instead, we can have non_atomic version of kref.
We can discuss about non_atomic kref in another thread, if you are interested.

> Also, why does destroy_usb_class() have that "if (usb_class) "test? 
> Isn't it true that usb_class can never be NULL there?

Removed in Patch v4.

thanks,
ajay kaher
 
  
Signed-off-by: Ajay Kaher
 
---

 drivers/usb/core/file.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index 822ced9..422ce7b 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,9 @@ static void release_usb_class(struct kref *kref)

 static void destroy_usb_class(void)
 {
-       if (usb_class)
-               kref_put(&usb_class->kref, release_usb_class);
+       mutex_lock(&init_usb_class_mutex);
+       kref_put(&usb_class->kref, release_usb_class);
+       mutex_unlock(&init_usb_class_mutex);
 }

 int usb_major_init(void)
@@ -171,7 +173,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;