linux-kernel - Re: [PATCH] usbip: vhc_hcd: prevent module being removed while device are attached

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, 3 Apr 2018 09:56:14 -0600
From:   Shuah Khan <shuahkh@....samsung.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     valentina.manea.m@...il.com, shuah@...nel.org,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shuah Khan <shuahkh@....samsung.com>
Subject: Re: [PATCH] usbip: vhc_hcd: prevent module being removed while device
 are attached

On 04/03/2018 12:56 AM, Greg KH wrote:
> On Mon, Apr 02, 2018 at 02:52:31PM -0600, Shuah Khan wrote:
>> vhci_hcd module can be removed even when devices are attached. Fix to
>> prevent module removal when devices are still attached.
>>
>> Signed-off-by: Shuah Khan <shuahkh@....samsung.com>
>> ---
>>  drivers/usb/usbip/vhci_sysfs.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
>> index 48808388ec33..6a54b9aa92be 100644
>> --- a/drivers/usb/usbip/vhci_sysfs.c
>> +++ b/drivers/usb/usbip/vhci_sysfs.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/net.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>> +#include <linux/module.h>
>>  
>>  #include "usbip_common.h"
>>  #include "vhci.h"
>> @@ -252,6 +253,8 @@ static ssize_t detach_store(struct device *dev, struct device_attribute *attr,
>>  	if (ret < 0)
>>  		return -EINVAL;
>>  
>> +	module_put(THIS_MODULE);
>> +
>>  	usbip_dbg_vhci_sysfs("Leave\n");
>>  
>>  	return count;
>> @@ -302,7 +305,7 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>  	struct vhci_hcd *vhci_hcd;
>>  	struct vhci_device *vdev;
>>  	struct vhci *vhci;
>> -	int err;
>> +	int err, ret;
>>  	unsigned long flags;
>>  
>>  	/*
>> @@ -339,10 +342,18 @@ static ssize_t attach_store(struct device *dev, struct device_attribute *attr,
>>  	else
>>  		vdev = &vhci->vhci_hcd_hs->vdev[rhport];
>>  
>> +	/* get module ref to avoid being removed with active attached devs */
>> +	if (!try_module_get(THIS_MODULE)) {
>> +		ret = -EAGAIN;
>> +		goto module_get_err;
>> +	}
> 
> That's really not a good idea, trying to grab your own module reference
> is considered racy and we stopped adding that code pattern to the kernel
> a long time ago.

Thanks. Good to know.

> 
> What's wrong if you remove the vhci module if devices are attached?  You
> can do that today for any other USB host driver, this shouldn't be
> "special".

This is a virtual device associated to a real physical device on a different
system. My concern is that if the module gets removed accidentally then it
could disrupt access to the remote device. The remote nature of the device
with several players involved makes this scenario a bit more complex than
a regular usb case when device is physically on the system. It is probably one
of the minor problems that could happen with a remote device.

I found a few modules that hold reference to themselves in the kernel. Block,
crypto, net, md, vfio, nfs.

md for example holds reference to itself when it creates a blank device.
vfio get regerence to itself it when opens pci and mediated devices.
Some network drivers do the same for handling virtual functions.
nfs does this for rpc handling.

I am not making a case for adding one more, however I am curious if this
vhci_hcd falls into a similar special case of handling virtual connections
and devices.

> 
> Also, kernel modules are never automatically removed by the system, so
> someone has to do this by hand, so they knew what they were doing :)
> 

Yes. This will not be a usual scenario. The question is whether not kernel
should detect driver is in use and prevent it.

thanks,
-- Shuah