Re: [PATCH] usbip: vhci_hcd: Check rhport everywhere in vhci_hub_control()

[Shuah Khan]

Hi Ben,

Thanks for the patch.

On 10/10/2018 11:30 PM, Ben Hutchings wrote:
> Commit 5b22f676118f "usbip: vhci_hcd: check rhport before using in
> vhci_hub_control()" added some validation of rhport, but left
> several problems:
> 
> - If VHCI_HC_PORTS < 256, we can get rhport >= VHCI_HC_PORTS which
>   is also out of range.  To keep things simple, set rhport to -1 if
>   this would happen.
> - For GetPortStatus, we range-check wIndex (and by implication
>   rhport) and report an error, but *don't* skip the following code.
>   Add a goto to the error path.
> - At the end of the function, there's one last port_status lookup
>   that's not protected by any range check.


I have patch out for this to fix a syzbot reported problem.

console output: https://syzkaller.appspot.com/x/log.txt?x=126a6f0e40
kernel config:  https://syzkaller.appspot.com/x/.config?x=531a917630d2a492
dashboard link: https://syzkaller.appspot.com/bug?extid=bccc1fe10b70fadc78d0
compiler:   gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:  https://syzkaller.appspot.com/x/repro.syz?x=121caa4640
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ed8ab640

I was able to reproduce the problem with the C reproducer and fixed it.

Here is fix:
https://patchwork.kernel.org/patch/10628833/

Sudip verified the patch.

thanks,
-- Shuah

Re: [PATCH] usbip: Fix misuse of strncpy()

[Shuah Khan]

On 07/26/2018 04:39 AM, Ben Hutchings wrote:
> On Tue, 2018-07-24 at 11:04 -0600, Shuah Khan wrote:
>> On 07/20/2018 08:12 PM, Ben Hutchings wrote:
>>> gcc 8 reports:
>>>
>>> usbip_device_driver.c: In function ‘read_usb_vudc_device’:
>>> usbip_device_driver.c:106:2: error: ‘strncpy’ specified bound 256 equals 
>>> destination size [-Werror=stringop-truncation]
>>>   strncpy(dev->path, path, SYSFS_PATH_MAX);
>>>   ^~~~
>>> usbip_device_driver.c:125:2: error: ‘strncpy’ specified bound 32 equals 
>>> destination size [-Werror=stringop-truncation]
>>>   strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
>>>   ^~~~
>>>
>>> I'm not convinced it makes sense to truncate the copied strings here,
>>> but since we're already doing so let's ensure they're still null-
>>> terminated.  We can't easily use strlcpy() here, so use snprintf().
>>>
>>> usbip_common.c has the same problem.
>>>
>>> Signed-off-by: Ben Hutchings 
>>> Cc: sta...@vger.kernel.org
>>> ---
>>>  tools/usb/usbip/libsrc/usbip_common.c|4 ++--
>>>  tools/usb/usbip/libsrc/usbip_device_driver.c |4 ++--
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> --- a/tools/usb/usbip/libsrc/usbip_common.c
>>> +++ b/tools/usb/usbip/libsrc/usbip_common.c
>>> @@ -226,8 +226,8 @@ int read_usb_device(struct udev_device *
>>> path = udev_device_get_syspath(sdev);
>>> name = udev_device_get_sysname(sdev);
>>>  
>>> -   strncpy(udev->path,  path,  SYSFS_PATH_MAX);
>>> -   strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
>>> +   snprintf(udev->path, SYSFS_PATH_MAX, "%s", path);
>>> +   snprintf(udev->busid, SYSFS_BUS_ID_SIZE, "%s", name);
>>
>> I am okay with the change to use snprintf(). Please add check for
>> return to avoid GCC 7 -Wformat-overflow wanrs on snprintf instances
>> that don't check return.
> [...]
> 
> I've tried running:
> 
> ./autogen.sh
> CC=gcc-7 CFLAGS=-Wformat-overflow ./configure 
> make
> 
> but this didn't produce any warnings.  How did you get the warning?
> 
> Ben.
> 

Sorry for the delay. It has been a busy few weeks with travel both
vacation and business.

Okay. Please check the e5dfa3f902b9a642ae8c6997d57d7c41e384a90b for
details. I didn't see this problem myself, likely because I was using
older gcc.

thanks,
-- Shuah

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: Fix misuse of strncpy()

[Shuah Khan]

On 07/20/2018 08:12 PM, Ben Hutchings wrote:
> gcc 8 reports:
> 
> usbip_device_driver.c: In function ‘read_usb_vudc_device’:
> usbip_device_driver.c:106:2: error: ‘strncpy’ specified bound 256 equals 
> destination size [-Werror=stringop-truncation]
>   strncpy(dev->path, path, SYSFS_PATH_MAX);
>   ^~~~
> usbip_device_driver.c:125:2: error: ‘strncpy’ specified bound 32 equals 
> destination size [-Werror=stringop-truncation]
>   strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
>   ^~~~
> 
> I'm not convinced it makes sense to truncate the copied strings here,
> but since we're already doing so let's ensure they're still null-
> terminated.  We can't easily use strlcpy() here, so use snprintf().
> 
> usbip_common.c has the same problem.
> 
> Signed-off-by: Ben Hutchings 
> Cc: sta...@vger.kernel.org
> ---
>  tools/usb/usbip/libsrc/usbip_common.c|4 ++--
>  tools/usb/usbip/libsrc/usbip_device_driver.c |4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/tools/usb/usbip/libsrc/usbip_common.c
> +++ b/tools/usb/usbip/libsrc/usbip_common.c
> @@ -226,8 +226,8 @@ int read_usb_device(struct udev_device *
>   path = udev_device_get_syspath(sdev);
>   name = udev_device_get_sysname(sdev);
>  
> - strncpy(udev->path,  path,  SYSFS_PATH_MAX);
> - strncpy(udev->busid, name, SYSFS_BUS_ID_SIZE);
> + snprintf(udev->path, SYSFS_PATH_MAX, "%s", path);
> + snprintf(udev->busid, SYSFS_BUS_ID_SIZE, "%s", name);

I am okay with the change to use snprintf(). Please add check for
return to avoid GCC 7 -Wformat-overflow wanrs on snprintf instances
that don't check return.

>  
>   sscanf(name, "%u-%u", , );
>   udev->busnum = busnum;
> --- a/tools/usb/usbip/libsrc/usbip_device_driver.c
> +++ b/tools/usb/usbip/libsrc/usbip_device_driver.c
> @@ -103,7 +103,7 @@ int read_usb_vudc_device(struct udev_dev
>   copy_descr_attr16(dev, , idProduct);
>   copy_descr_attr16(dev, , bcdDevice);
>  
> - strncpy(dev->path, path, SYSFS_PATH_MAX);
> + snprintf(dev->path, SYSFS_PATH_MAX, "%s", path);
>  
>   dev->speed = USB_SPEED_UNKNOWN;
>   speed = udev_device_get_sysattr_value(sdev, "current_speed");
> @@ -122,7 +122,7 @@ int read_usb_vudc_device(struct udev_dev
>   dev->busnum = 0;
>  
>   name = udev_device_get_sysname(plat);
> - strncpy(dev->busid, name, SYSFS_BUS_ID_SIZE);
> + snprintf(dev->busid, SYSFS_BUS_ID_SIZE, "%s", name);

I am okay with the change to use snprintf(). Please add check for
return to avoid GCC 7 -Wformat-overflow wanrs on snprintf instances
that don't check return.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] usbip: dynamically allocate idev by nports found in sysfs

[Shuah Khan]

On 05/23/2018 03:22 AM, Michael Grzeschik wrote:
> As the amount of available ports varies by the kernels build
> configuration. To remove the limitation of the fixed 128 ports
> we allocate the amount of idevs by using the number we get
> from the kernel.
> 
> Signed-off-by: Michael Grzeschik 
> ---
> v1 -> v2: - reworked memory allocation into one calloc call
>   - added error path on allocation failure
> v2 -> v3: - moved check for available nports to beginning of function
> 

Hmm. With this patch I see a segfault when I run usbip port command.
I think this patch is incomplete and more changes are needed to the
code that references the idev array.

I can't take this patch.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbip: dynamically allocate idev by nports found in sysfs

[Shuah Khan]

On 05/22/2018 11:04 AM, Michael Grzeschik wrote:
> As the amount of available ports varies by the kernels build
> configuration. To remove the limitation of the fixed 128 ports
> we allocate the amount of idevs by using the number we get
> from the kernel.
> 
> Signed-off-by: Michael Grzeschik 
> ---
> v1 -> v2: - reworked memory allocation into one calloc call
>   - added error path on allocation failure
> 
>  tools/usb/usbip/libsrc/vhci_driver.c | 14 +-
>  tools/usb/usbip/libsrc/vhci_driver.h |  3 +--
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index c9c81614a66ad..6e2a9edfd1f0d 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -242,13 +242,20 @@ static int read_record(int rhport, char *host, unsigned 
> long host_len,
>  
>  int usbip_vhci_driver_open(void)
>  {
> + int nports = get_nports();
> 

I missed this error leg in my previous comments. get_nports() could return
errorwhich is -1.

>   udev_context = udev_new();
>   if (!udev_context) {
>   err("udev_new failed");
>   return -1;
>   }
>  
> - vhci_driver = calloc(1, sizeof(struct usbip_vhci_driver));
> + vhci_driver = calloc(1, sizeof(struct usbip_vhci_driver) +
> + nports * sizeof(struct usbip_imported_device));

nports could be -1 at this point.

> + if (!vhci_driver) {
> + err("vhci_driver allocation failed");
> + return -1;
> + }
>  
>   /* will be freed in usbip_driver_close() */
>   vhci_driver->hc_device =
> @@ -260,15 +267,12 @@ int usbip_vhci_driver_open(void)
>   goto err;
>   }
>  
> - vhci_driver->nports = get_nports();
> + vhci_driver->nports = nports;
>   dbg("available ports: %d", vhci_driver->nports);
>  
>   if (vhci_driver->nports <= 0) {
>   err("no available ports");
>   goto err;
This check should move up along with the get_nports() call.

> - } else if (vhci_driver->nports > MAXNPORT) {
> - err("port number exceeds %d", MAXNPORT);
> - goto err;
>   }
>  
>   vhci_driver->ncontrollers = get_ncontrollers();
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.h 
> b/tools/usb/usbip/libsrc/vhci_driver.h
> index 418b404d51210..6c9aca2167051 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.h
> +++ b/tools/usb/usbip/libsrc/vhci_driver.h
> @@ -13,7 +13,6 @@
>  
>  #define USBIP_VHCI_BUS_TYPE "platform"
>  #define USBIP_VHCI_DEVICE_NAME "vhci_hcd.0"
> -#define MAXNPORT 128
>  
>  enum hub_speed {
>   HUB_SPEED_HIGH = 0,
> @@ -41,7 +40,7 @@ struct usbip_vhci_driver {
>  
>   int ncontrollers;
>   int nports;
> - struct usbip_imported_device idev[MAXNPORT];
> + struct usbip_imported_device idev[];
>  };
>  
>  
> 

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: dynamically allocate idev by nports found in sysfs

[Shuah Khan]

Hi Michael,

Thanks for the patch. Couple of comments below:

On 05/18/2018 08:39 AM, Michael Grzeschik wrote:
> As the amount of available ports varies by the kernels build
> configuration. To remove the limitation of the fixed 128 ports
> we allocate the amount of idevs by using the number we get
> from the kernel.
> 
> Signed-off-by: Michael Grzeschik 
> ---
>  tools/usb/usbip/libsrc/vhci_driver.c | 11 ---
>  tools/usb/usbip/libsrc/vhci_driver.h |  3 +--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index c9c81614a66ad..9a8acfc7697fa 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -266,11 +266,11 @@ int usbip_vhci_driver_open(void)
>   if (vhci_driver->nports <= 0) {
>   err("no available ports");
>   goto err;
> - } else if (vhci_driver->nports > MAXNPORT) {
> - err("port number exceeds %d", MAXNPORT);
> - goto err;
>   }
>  
> + vhci_driver->idev = calloc(vhci_driver->nports,
> + sizeof(struct usbip_imported_device));
> +

Missing check for memory allocation failure. Please add it.

>   vhci_driver->ncontrollers = get_ncontrollers();
>   dbg("available controllers: %d", vhci_driver->ncontrollers);
>  
> @@ -287,6 +287,9 @@ int usbip_vhci_driver_open(void)
>  err:
>   udev_device_unref(vhci_driver->hc_device);
>  
> + if (vhci_driver->idev)
> + free(vhci_driver->idev);
> +
>   if (vhci_driver)
>   free(vhci_driver);
>  
> @@ -305,6 +308,8 @@ void usbip_vhci_driver_close(void)
>  
>   udev_device_unref(vhci_driver->hc_device);
>  
> + free(vhci_driver->idev);
> +
>   free(vhci_driver);
>  
>   vhci_driver = NULL;
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.h 
> b/tools/usb/usbip/libsrc/vhci_driver.h
> index 418b404d51210..67dbd1551e159 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.h
> +++ b/tools/usb/usbip/libsrc/vhci_driver.h
> @@ -13,7 +13,6 @@
>  
>  #define USBIP_VHCI_BUS_TYPE "platform"
>  #define USBIP_VHCI_DEVICE_NAME "vhci_hcd.0"
> -#define MAXNPORT 128
>  
>  enum hub_speed {
>   HUB_SPEED_HIGH = 0,
> @@ -41,7 +40,7 @@ struct usbip_vhci_driver {
>  
>   int ncontrollers;
>   int nports;
> - struct usbip_imported_device idev[MAXNPORT];
> + struct usbip_imported_device *idev;
>  };
>  
>  
> 

Rest looks good.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] usbip: vhci_sysfs: fix potential Spectre v1

[Shuah Khan]

On 05/18/2018 07:13 PM, Gustavo A. R. Silva wrote:
> pdev_nr and rhport can be controlled by user-space, hence leading to
> a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This issue was detected with the help of Smatch:
> drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre 
> issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre 
> issue 'vhcis'
> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre 
> issue 'vhci->vhci_hcd_ss->vdev'
> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre 
> issue 'vhci->vhci_hcd_hs->vdev'
> 
> Fix this by sanitizing pdev_nr and rhport before using them to index
> vhcis and vhci->vhci_hcd_ss->vdev respectively.
> 
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
> 
> [1] https://marc.info/?l=linux-kernel=152449131114778=2
> 
> Cc: sta...@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> ---
> Changes in v3:
>  - Pass the addresses of pdev_nr and rhport into valid_port and
>valid_args.
> 
> Changes in v2:
>  - Place the barriers into valid_port.
> 
>  drivers/usb/usbip/vhci_sysfs.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
> index 4880838..be37aec 100644
> --- a/drivers/usb/usbip/vhci_sysfs.c
> +++ b/drivers/usb/usbip/vhci_sysfs.c
> @@ -10,6 +10,9 @@
>  #include 
>  #include 
>  
> +/* Hardening for Spectre-v1 */
> +#include 
> +
>  #include "usbip_common.h"
>  #include "vhci.h"
>  
> @@ -205,16 +208,20 @@ static int vhci_port_disconnect(struct vhci_hcd 
> *vhci_hcd, __u32 rhport)
>   return 0;
>  }
>  
> -static int valid_port(__u32 pdev_nr, __u32 rhport)
> +static int valid_port(__u32 *pdev_nr, __u32 *rhport)
>  {
> - if (pdev_nr >= vhci_num_controllers) {
> - pr_err("pdev %u\n", pdev_nr);
> + if (*pdev_nr >= vhci_num_controllers) {
> + pr_err("pdev %u\n", *pdev_nr);
>   return 0;
>   }
> - if (rhport >= VHCI_HC_PORTS) {
> - pr_err("rhport %u\n", rhport);
> + *pdev_nr = array_index_nospec(*pdev_nr, vhci_num_controllers);
> +
> + if (*rhport >= VHCI_HC_PORTS) {
> + pr_err("rhport %u\n", *rhport);
>   return 0;
>   }
> + *rhport = array_index_nospec(*rhport, VHCI_HC_PORTS);
> +
>   return 1;
>  }
>  
> @@ -232,7 +239,7 @@ static ssize_t detach_store(struct device *dev, struct 
> device_attribute *attr,
>   pdev_nr = port_to_pdev_nr(port);
>   rhport = port_to_rhport(port);
>  
> - if (!valid_port(pdev_nr, rhport))
> + if (!valid_port(_nr, ))
>   return -EINVAL;
>  
>   hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> @@ -258,7 +265,8 @@ static ssize_t detach_store(struct device *dev, struct 
> device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(detach);
>  
> -static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed 
> speed)
> +static int valid_args(__u32 *pdev_nr, __u32 *rhport,
> +   enum usb_device_speed speed)
>  {
>   if (!valid_port(pdev_nr, rhport)) {
>   return 0;
> @@ -322,7 +330,7 @@ static ssize_t attach_store(struct device *dev, struct 
> device_attribute *attr,
>sockfd, devid, speed);
>  
>   /* check received parameters */
> - if (!valid_args(pdev_nr, rhport, speed))
> + if (!valid_args(_nr, , speed))
>   return -EINVAL;
>  
>   hcd = platform_get_drvdata(vhcis[pdev_nr].pdev);
> 

Looks good to me. Thanks for taking care of this.

Acked-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>

-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] usbip: vhci_sysfs: fix potential Spectre v1

[Shuah Khan]

On 05/18/2018 07:47 AM, Greg Kroah-Hartman wrote:
> On Thu, May 17, 2018 at 03:16:28PM -0500, Gustavo A. R. Silva wrote:
>> pdev_nr and rhport can be controlled by user-space, hence leading to
>> a potential exploitation of the Spectre variant 1 vulnerability.
>>
>> This issue was detected with the help of Smatch:
>> drivers/usb/usbip/vhci_sysfs.c:238 detach_store() warn: potential spectre 
>> issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:328 attach_store() warn: potential spectre 
>> issue 'vhcis'
>> drivers/usb/usbip/vhci_sysfs.c:338 attach_store() warn: potential spectre 
>> issue 'vhci->vhci_hcd_ss->vdev'
>> drivers/usb/usbip/vhci_sysfs.c:340 attach_store() warn: potential spectre 
>> issue 'vhci->vhci_hcd_hs->vdev'
>>
>> Fix this by sanitizing pdev_nr and rhport before using them to index
>> vhcis and vhci->vhci_hcd_ss->vdev respectively.
>>
>> Notice that given that speculation windows are large, the policy is
>> to kill the speculation on the first load and not worry if it can be
>> completed with a dependent load/store [1].
>>
>> [1] https://marc.info/?l=linux-kernel=152449131114778=2
>>
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva 
>> ---
>> Changes in v2:
>>  - Place the barriers into valid_port.

attach_store() doesn't call valid_port() - can you make the change to
have attach_store() call valid_port() to protect that code path.

> 
> Thanks for the change.  I'll wait for Shuah's ack/review before queueing
> this up just as she knows that codebase much better than anyone else.
> > 

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: usbip_host: fix bad unlock balance during stub_probe()

[Shuah Khan (Samsung OSG)]

stub_probe() calls put_busid_priv() in an error path when device isn't
found in the busid_table. Fix it by making put_busid_priv() safe to be
called with null struct bus_id_priv pointer.

This problem happens when "usbip bind" is run without loading usbip_host
driver and then running modprobe. The first failed bind attempt unbinds
the device from the original driver and when usbip_host is modprobed,
stub_probe() runs and doesn't find the device in its busid table and calls
put_busid_priv(0 with null bus_id_priv pointer.

usbip-host 3-10.2: 3-10.2 is not in match_busid table...  skip!

[  367.359679] =
[  367.359681] WARNING: bad unlock balance detected!
[  367.359683] 4.17.0-rc4+ #5 Not tainted
[  367.359685] -
[  367.359688] modprobe/2768 is trying to release lock (
[  367.359689]
==
[  367.359696] BUG: KASAN: null-ptr-deref in
print_unlock_imbalance_bug+0x99/0x110
[  367.359699] Read of size 8 at addr 0058 by task
modprobe/2768

[  367.359705] CPU: 4 PID: 2768 Comm: modprobe Not tainted 4.17.0-rc4+ #5

Fixes: 22076557b07c ("usbip: usbip_host: fix NULL-ptr deref and
use-after-free errors") in usb-linus

Signed-off-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>
---

 drivers/usb/usbip/stub_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 41c7b9de2a92..bf8a5feb0ee9 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -82,7 +82,8 @@ struct bus_id_priv *get_busid_priv(const char *busid)
 
 void put_busid_priv(struct bus_id_priv *bid)
 {
-   spin_unlock(>busid_lock);
+   if (bid)
+   spin_unlock(>busid_lock);
 }
 
 static int add_match_busid(char *busid)
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: usbip_host: fix NULL-ptr deref and use-after-free errors

[Shuah Khan (Samsung OSG)]

usbip_host updates device status without holding lock from stub probe,
disconnect and rebind code paths. When multiple requests to import a
device are received, these unprotected code paths step all over each
other and drive fails with NULL-ptr deref and use-after-free errors.

The driver uses a table lock to protect the busid array for adding and
deleting busids to the table. However, the probe, disconnect and rebind
paths get the busid table entry and update the status without holding
the busid table lock. Add a new finer grain lock to protect the busid
entry. This new lock will be held to search and update the busid entry
fields from get_busid_idx(), add_match_busid() and del_match_busid().

match_busid_show() does the same to access the busid entry fields.

get_busid_priv() changed to return the pointer to the busid entry holding
the busid lock. stub_probe(), stub_disconnect() and stub_device_rebind()
call put_busid_priv() to release the busid lock before returning. This
changes fixes the unprotected code paths eliminating the race conditions
in updating the busid entries.

Signed-off-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>
---
 drivers/usb/usbip/stub.h  |  2 ++
 drivers/usb/usbip/stub_dev.c  | 33 +++--
 drivers/usb/usbip/stub_main.c | 40 +++-
 3 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
index 14a72357800a..35618ceb2791 100644
--- a/drivers/usb/usbip/stub.h
+++ b/drivers/usb/usbip/stub.h
@@ -73,6 +73,7 @@ struct bus_id_priv {
struct stub_device *sdev;
struct usb_device *udev;
char shutdown_busid;
+   spinlock_t busid_lock;
 };
 
 /* stub_priv is allocated from stub_priv_cache */
@@ -83,6 +84,7 @@ extern struct usb_device_driver stub_driver;
 
 /* stub_main.c */
 struct bus_id_priv *get_busid_priv(const char *busid);
+void put_busid_priv(struct bus_id_priv *bid);
 int del_match_busid(char *busid);
 void stub_device_cleanup_urbs(struct stub_device *sdev);
 
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 9d0425113c4b..c0d6ff1baa72 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -300,7 +300,7 @@ static int stub_probe(struct usb_device *udev)
struct stub_device *sdev = NULL;
const char *udev_busid = dev_name(>dev);
struct bus_id_priv *busid_priv;
-   int rc;
+   int rc = 0;
 
dev_dbg(>dev, "Enter probe\n");
 
@@ -317,13 +317,15 @@ static int stub_probe(struct usb_device *udev)
 * other matched drivers by the driver core.
 * See driver_probe_device() in driver/base/dd.c
 */
-   return -ENODEV;
+   rc = -ENODEV;
+   goto call_put_busid_priv;
}
 
if (udev->descriptor.bDeviceClass == USB_CLASS_HUB) {
dev_dbg(>dev, "%s is a usb hub device... skip!\n",
 udev_busid);
-   return -ENODEV;
+   rc = -ENODEV;
+   goto call_put_busid_priv;
}
 
if (!strcmp(udev->bus->bus_name, "vhci_hcd")) {
@@ -331,13 +333,16 @@ static int stub_probe(struct usb_device *udev)
"%s is attached on vhci_hcd... skip!\n",
udev_busid);
 
-   return -ENODEV;
+   rc = -ENODEV;
+   goto call_put_busid_priv;
}
 
/* ok, this is my device */
sdev = stub_device_alloc(udev);
-   if (!sdev)
-   return -ENOMEM;
+   if (!sdev) {
+   rc = -ENOMEM;
+   goto call_put_busid_priv;
+   }
 
dev_info(>dev,
"usbip-host: register new device (bus %u dev %u)\n",
@@ -369,7 +374,9 @@ static int stub_probe(struct usb_device *udev)
}
busid_priv->status = STUB_BUSID_ALLOC;
 
-   return 0;
+   rc = 0;
+   goto call_put_busid_priv;
+
 err_files:
usb_hub_release_port(udev->parent, udev->portnum,
 (struct usb_dev_state *) udev);
@@ -379,6 +386,9 @@ static int stub_probe(struct usb_device *udev)
 
busid_priv->sdev = NULL;
stub_device_free(sdev);
+
+call_put_busid_priv:
+   put_busid_priv(busid_priv);
return rc;
 }
 
@@ -417,7 +427,7 @@ static void stub_disconnect(struct usb_device *udev)
/* get stub_device */
if (!sdev) {
dev_err(>dev, "could not get device");
-   return;
+   goto call_put_busid_priv;
}
 
dev_set_drvdata(>dev, NULL);
@@ -432,12 +442,12 @@ static void stub_disconnect(struct usb_device *udev)
  (struct usb_dev_state *) udev);
if (rc) {
dev_dbg(>dev, "unable to release port\n");
-   

Re: [REBASED PATCH 1/2] usbip: usbip_host: delete device from busid_table after rebind

[Shuah Khan]

On 04/30/2018 04:48 PM, Greg KH wrote:
> On Mon, Apr 30, 2018 at 04:17:19PM -0600, Shuah Khan (Samsung OSG) wrote:
>> Device is left in the busid_table after unbind and rebind. Rebind
>> initiates usb bus scan and the original driver claims the device.
>> After rescan the device should be deleted from the busid_table as
>> it no longer belongs to usbip_host.
>>
>> Fix it to delete the device after device_attach() succeeds.
>>
>> Signed-off-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>
>> ---
>>  drivers/usb/usbip/stub_main.c | 6 ++
>>  1 file changed, 6 insertions(+)
> 
> Why are these REBASED?  I can't remember the first version of these
> patches, what are these for?
> 
> confused,
> 
> greg k-h
> 
> 

Here is the original patches

https://patchwork.kernel.org/patch/10337657/
https://patchwork.kernel.org/patch/10337661/

I sent a 3 patch series and the last two didn't apply to usb-next. You asked me 
to
rebase them. I realized the reason they didn't apply is because these two 
depend on
the fixes that went into 4.17-rc3 and now with usb-next at 4.17-rc3, they apply.

I ended up generating patches on usb-next latest and resend the rebased patches.

Hope this makes sense.

thanks,
-- Shuah


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[REBASED PATCH 2/2] usbip: usbip_host: run rebind from exit when module is removed

[Shuah Khan (Samsung OSG)]

After removing usbip_host module, devices it releases are left without
a driver. For example, when a keyboard or a mass storage device are
bound to usbip_host when it is removed, these devices are no longer
bound to any driver.

Fix it to run device_attach() from the module exit routine to restore
the devices to their original drivers. This includes cleanup changes
and moving device_attach() code to a common routine to be called from
rebind_store() and usbip_host_exit().

Signed-off-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>
---
 drivers/usb/usbip/stub_dev.c  |  6 +
 drivers/usb/usbip/stub_main.c | 60 ---
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 7813c1862941..9d0425113c4b 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -448,12 +448,8 @@ static void stub_disconnect(struct usb_device *udev)
busid_priv->sdev = NULL;
stub_device_free(sdev);
 
-   if (busid_priv->status == STUB_BUSID_ALLOC) {
+   if (busid_priv->status == STUB_BUSID_ALLOC)
busid_priv->status = STUB_BUSID_ADDED;
-   } else {
-   busid_priv->status = STUB_BUSID_OTHER;
-   del_match_busid((char *)udev_busid);
-   }
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index fb46bd62d538..587b9bc10042 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -14,6 +14,7 @@
 #define DRIVER_DESC "USB/IP Host Driver"
 
 struct kmem_cache *stub_priv_cache;
+
 /*
  * busid_tables defines matching busids that usbip can grab. A user can change
  * dynamically what device is locally used and what device is exported to a
@@ -169,6 +170,51 @@ static ssize_t match_busid_store(struct device_driver 
*dev, const char *buf,
 }
 static DRIVER_ATTR_RW(match_busid);
 
+static int do_rebind(char *busid, struct bus_id_priv *busid_priv)
+{
+   int ret;
+
+   /* device_attach() callers should hold parent lock for USB */
+   if (busid_priv->udev->dev.parent)
+   device_lock(busid_priv->udev->dev.parent);
+   ret = device_attach(_priv->udev->dev);
+   if (busid_priv->udev->dev.parent)
+   device_unlock(busid_priv->udev->dev.parent);
+   if (ret < 0) {
+   dev_err(_priv->udev->dev, "rebind failed\n");
+   return ret;
+   }
+   return 0;
+}
+
+static void stub_device_rebind(void)
+{
+#if IS_MODULE(CONFIG_USBIP_HOST)
+   struct bus_id_priv *busid_priv;
+   int i;
+
+   /* update status to STUB_BUSID_OTHER so probe ignores the device */
+   spin_lock(_table_lock);
+   for (i = 0; i < MAX_BUSID; i++) {
+   if (busid_table[i].name[0] &&
+   busid_table[i].shutdown_busid) {
+   busid_priv = &(busid_table[i]);
+   busid_priv->status = STUB_BUSID_OTHER;
+   }
+   }
+   spin_unlock(_table_lock);
+
+   /* now run rebind */
+   for (i = 0; i < MAX_BUSID; i++) {
+   if (busid_table[i].name[0] &&
+   busid_table[i].shutdown_busid) {
+   busid_priv = &(busid_table[i]);
+   do_rebind(busid_table[i].name, busid_priv);
+   }
+   }
+#endif
+}
+
 static ssize_t rebind_store(struct device_driver *dev, const char *buf,
 size_t count)
 {
@@ -189,16 +235,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
/* mark the device for deletion so probe ignores it during rescan */
bid->status = STUB_BUSID_OTHER;
 
-   /* device_attach() callers should hold parent lock for USB */
-   if (bid->udev->dev.parent)
-   device_lock(bid->udev->dev.parent);
-   ret = device_attach(>udev->dev);
-   if (bid->udev->dev.parent)
-   device_unlock(bid->udev->dev.parent);
-   if (ret < 0) {
-   dev_err(>udev->dev, "rebind failed\n");
+   ret = do_rebind((char *) buf, bid);
+   if (ret < 0)
return ret;
-   }
 
/* delete device from busid_table */
del_match_busid((char *) buf);
@@ -323,6 +362,9 @@ static void __exit usbip_host_exit(void)
 */
usb_deregister_device_driver(_driver);
 
+   /* initiate scan to attach devices */
+   stub_device_rebind();
+
kmem_cache_destroy(stub_priv_cache);
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[REBASED PATCH 1/2] usbip: usbip_host: delete device from busid_table after rebind

[Shuah Khan (Samsung OSG)]

Device is left in the busid_table after unbind and rebind. Rebind
initiates usb bus scan and the original driver claims the device.
After rescan the device should be deleted from the busid_table as
it no longer belongs to usbip_host.

Fix it to delete the device after device_attach() succeeds.

Signed-off-by: Shuah Khan (Samsung OSG) <sh...@kernel.org>
---
 drivers/usb/usbip/stub_main.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index d41d0cdeec0f..fb46bd62d538 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -186,6 +186,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
if (!bid)
return -ENODEV;
 
+   /* mark the device for deletion so probe ignores it during rescan */
+   bid->status = STUB_BUSID_OTHER;
+
/* device_attach() callers should hold parent lock for USB */
if (bid->udev->dev.parent)
device_lock(bid->udev->dev.parent);
@@ -197,6 +200,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
return ret;
}
 
+   /* delete device from busid_table */
+   del_match_busid((char *) buf);
+
return count;
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] usbip: usbip_host: run rebind from exit when module is removed

[Shuah Khan]

After removing usbip_host module, devices it releases are left without
a driver. For example, when a keyboard or a mass storage device are
bound to usbip_host when it is removed, these devices are no longer
bound to any driver.

Fix it to run device_attach() from the module exit routine to restore
the devices to their original drivers. This includes cleanup changes
and moving device_attach() code to a common routine to be called from
rebind_store() and usbip_host_exit().

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_dev.c  |  6 +
 drivers/usb/usbip/stub_main.c | 60 ---
 2 files changed, 52 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index 7813c1862941..9d0425113c4b 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -448,12 +448,8 @@ static void stub_disconnect(struct usb_device *udev)
busid_priv->sdev = NULL;
stub_device_free(sdev);
 
-   if (busid_priv->status == STUB_BUSID_ALLOC) {
+   if (busid_priv->status == STUB_BUSID_ALLOC)
busid_priv->status = STUB_BUSID_ADDED;
-   } else {
-   busid_priv->status = STUB_BUSID_OTHER;
-   del_match_busid((char *)udev_busid);
-   }
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index fb46bd62d538..587b9bc10042 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -14,6 +14,7 @@
 #define DRIVER_DESC "USB/IP Host Driver"
 
 struct kmem_cache *stub_priv_cache;
+
 /*
  * busid_tables defines matching busids that usbip can grab. A user can change
  * dynamically what device is locally used and what device is exported to a
@@ -169,6 +170,51 @@ static ssize_t match_busid_store(struct device_driver 
*dev, const char *buf,
 }
 static DRIVER_ATTR_RW(match_busid);
 
+static int do_rebind(char *busid, struct bus_id_priv *busid_priv)
+{
+   int ret;
+
+   /* device_attach() callers should hold parent lock for USB */
+   if (busid_priv->udev->dev.parent)
+   device_lock(busid_priv->udev->dev.parent);
+   ret = device_attach(_priv->udev->dev);
+   if (busid_priv->udev->dev.parent)
+   device_unlock(busid_priv->udev->dev.parent);
+   if (ret < 0) {
+   dev_err(_priv->udev->dev, "rebind failed\n");
+   return ret;
+   }
+   return 0;
+}
+
+static void stub_device_rebind(void)
+{
+#if IS_MODULE(CONFIG_USBIP_HOST)
+   struct bus_id_priv *busid_priv;
+   int i;
+
+   /* update status to STUB_BUSID_OTHER so probe ignores the device */
+   spin_lock(_table_lock);
+   for (i = 0; i < MAX_BUSID; i++) {
+   if (busid_table[i].name[0] &&
+   busid_table[i].shutdown_busid) {
+   busid_priv = &(busid_table[i]);
+   busid_priv->status = STUB_BUSID_OTHER;
+   }
+   }
+   spin_unlock(_table_lock);
+
+   /* now run rebind */
+   for (i = 0; i < MAX_BUSID; i++) {
+   if (busid_table[i].name[0] &&
+   busid_table[i].shutdown_busid) {
+   busid_priv = &(busid_table[i]);
+   do_rebind(busid_table[i].name, busid_priv);
+   }
+   }
+#endif
+}
+
 static ssize_t rebind_store(struct device_driver *dev, const char *buf,
 size_t count)
 {
@@ -189,16 +235,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
/* mark the device for deletion so probe ignores it during rescan */
bid->status = STUB_BUSID_OTHER;
 
-   /* device_attach() callers should hold parent lock for USB */
-   if (bid->udev->dev.parent)
-   device_lock(bid->udev->dev.parent);
-   ret = device_attach(>udev->dev);
-   if (bid->udev->dev.parent)
-   device_unlock(bid->udev->dev.parent);
-   if (ret < 0) {
-   dev_err(>udev->dev, "rebind failed\n");
+   ret = do_rebind((char *) buf, bid);
+   if (ret < 0)
return ret;
-   }
 
/* delete device from busid_table */
del_match_busid((char *) buf);
@@ -323,6 +362,9 @@ static void __exit usbip_host_exit(void)
 */
usb_deregister_device_driver(_driver);
 
+   /* initiate scan to attach devices */
+   stub_device_rebind();
+
kmem_cache_destroy(stub_priv_cache);
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usbip: usbip_host: refine probe and disconnect debug msgs to be useful

[Shuah Khan]

Refine probe and disconnect debug msgs to be useful and say what is
in progress.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_dev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index dd8ef36ab10e..7813c1862941 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -302,7 +302,7 @@ static int stub_probe(struct usb_device *udev)
struct bus_id_priv *busid_priv;
int rc;
 
-   dev_dbg(>dev, "Enter\n");
+   dev_dbg(>dev, "Enter probe\n");
 
/* check we should claim or not by busid_table */
busid_priv = get_busid_priv(udev_busid);
@@ -404,7 +404,7 @@ static void stub_disconnect(struct usb_device *udev)
struct bus_id_priv *busid_priv;
int rc;
 
-   dev_dbg(>dev, "Enter\n");
+   dev_dbg(>dev, "Enter disconnect\n");
 
busid_priv = get_busid_priv(udev_busid);
if (!busid_priv) {
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] usbip: usbip_host: delete device from busid_table after rebind

[Shuah Khan]

Device is left in the busid_table after unbind and rebind. Rebind
initiates usb bus scan and the original driver claims the device.
After rescan the device should be deleted from the busid_table as
it no longer belongs to usbip_host.

Fix it to delete the device after device_attach() succeeds.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_main.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index d41d0cdeec0f..fb46bd62d538 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -186,6 +186,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
if (!bid)
return -ENODEV;
 
+   /* mark the device for deletion so probe ignores it during rescan */
+   bid->status = STUB_BUSID_OTHER;
+
/* device_attach() callers should hold parent lock for USB */
if (bid->udev->dev.parent)
device_lock(bid->udev->dev.parent);
@@ -197,6 +200,9 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
return ret;
}
 
+   /* delete device from busid_table */
+   del_match_busid((char *) buf);
+
return count;
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: vhci_hcd: check rhport before using in vhci_hub_control()

[Shuah Khan]

On 04/06/2018 02:01 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 4/6/2018 1:31 AM, Shuah Khan wrote:
> 
>> Validate !rhport < 0 before using it to access port_status array.
> 
>    Why '!'?
> 

I should have explained it better in the commit log.

rhport is set based on input wIndex which could be 0. This isn't
the case for all the Request but some. wIndex is range checked in
the code paths that it shouldn't be. The same applies to rhport
in some request handling paths. Without the checks there is the
potential for out of bounds access on port_status array.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vhci_hcd: check rhport before using in vhci_hub_control()

[Shuah Khan]

Validate !rhport < 0 before using it to access port_status array.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 20e3d4609583..d11f3f8dad40 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -354,6 +354,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
usbip_dbg_vhci_rh(" ClearHubFeature\n");
break;
case ClearPortFeature:
+   if (rhport < 0)
+   goto error;
switch (wValue) {
case USB_PORT_FEAT_SUSPEND:
if (hcd->speed == HCD_USB3) {
@@ -511,11 +513,16 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
goto error;
}
 
+   if (rhport < 0)
+   goto error;
+
vhci_hcd->port_status[rhport] |= USB_PORT_STAT_SUSPEND;
break;
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(
" SetPortFeature: USB_PORT_FEAT_POWER\n");
+   if (rhport < 0)
+   goto error;
if (hcd->speed == HCD_USB3)
vhci_hcd->port_status[rhport] |= 
USB_SS_PORT_STAT_POWER;
else
@@ -524,6 +531,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_BH_PORT_RESET:
usbip_dbg_vhci_rh(
" SetPortFeature: 
USB_PORT_FEAT_BH_PORT_RESET\n");
+   if (rhport < 0)
+   goto error;
/* Applicable only for USB3.0 hub */
if (hcd->speed != HCD_USB3) {
pr_err("USB_PORT_FEAT_BH_PORT_RESET req not "
@@ -534,6 +543,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_RESET:
usbip_dbg_vhci_rh(
" SetPortFeature: USB_PORT_FEAT_RESET\n");
+   if (rhport < 0)
+   goto error;
/* if it's already enabled, disable */
if (hcd->speed == HCD_USB3) {
vhci_hcd->port_status[rhport] = 0;
@@ -554,6 +565,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
default:
usbip_dbg_vhci_rh(" SetPortFeature: default %d\n",
  wValue);
+   if (rhport < 0)
+   goto error;
if (hcd->speed == HCD_USB3) {
if ((vhci_hcd->port_status[rhport] &
 USB_SS_PORT_STAT_POWER) != 0) {
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: usbip_event: fix to not print kernel pointer address

[Shuah Khan]

Fix it to not print kernel pointer address. Remove the conditional
and debug message as it isn't very useful.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
---
 drivers/usb/usbip/usbip_event.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 5b4c0864ad92..5d88917c9631 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -91,10 +91,6 @@ static void event_handler(struct work_struct *work)
unset_event(ud, USBIP_EH_UNUSABLE);
}
 
-   /* Stop the error handler. */
-   if (ud->event & USBIP_EH_BYE)
-   usbip_dbg_eh("removed %p\n", ud);
-
wake_up(>eh_waitq);
}
 }
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: usbip_host: fix to hold parent lock for device_attach() calls

[Shuah Khan]

usbip_host calls device_attach() without holding dev->parent lock.
Fix it.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
---
 drivers/usb/usbip/stub_main.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index c31c8402a0c5..d41d0cdeec0f 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -186,7 +186,12 @@ static ssize_t rebind_store(struct device_driver *dev, 
const char *buf,
if (!bid)
return -ENODEV;
 
+   /* device_attach() callers should hold parent lock for USB */
+   if (bid->udev->dev.parent)
+   device_lock(bid->udev->dev.parent);
ret = device_attach(>udev->dev);
+   if (bid->udev->dev.parent)
+   device_unlock(bid->udev->dev.parent);
if (ret < 0) {
dev_err(>udev->dev, "rebind failed\n");
return ret;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shuah Khan]

On 04/05/2018 10:42 AM, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]
> 
> This commit has been processed by the -stable helper bot and determined
> to be a high probability candidate for -stable trees. (score: 13.1846)
> 
> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, 
> v4.4.126, 
> 
> v4.15.15: Build OK!
> v4.14.32: Build OK!
> v4.9.92: Failed to apply! Possible dependencies:
> 3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device 
> are attached"> 
> v4.4.126: Failed to apply! Possible dependencies:
> 0775a9cbc694 ("usbip: vhci extension: modifications to vhci driver")
> 3920ad4951e2 ("usbip: vhc_hcd: prevent module being removed while device 
> are attached")

This patch will not go into Linus's tree. Please don't pull it into stables.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: vhci_hcd: Fix usb device and sockfd leaks

[Shuah Khan]

On 04/05/2018 10:42 AM, Sasha Levin wrote:
> Hi.
> 
> [This is an automated email]
> 
> This commit has been processed by the -stable helper bot and determined
> to be a high probability candidate for -stable trees. (score: 10.6103)
> 
> The bot has tested the following trees: v4.15.15, v4.14.32, v4.9.92, 
> v4.4.126, 
> 
> v4.15.15: Build OK!
> v4.14.32: Build OK!
> v4.9.92: Build OK!
> v4.4.126: Failed to apply! Possible dependencies:
> f0f1beb8f155 ("usbip: vhci_hcd: Fix usb device and sockfd leaks")
> 
> 

Thanks. I will send patch for v4.4.126 that you can apply for this fix.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shuah Khan]

On 04/04/2018 02:25 AM, Oliver Neukum wrote:
> Am Dienstag, den 03.04.2018, 09:56 -0600 schrieb Shuah Khan:
>> 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
> 
> Hi,
> 
> you would doubtlessly lose connection to that device. Yet you would
> also lose connections if you down your network. You need to be root
> to unload a module. You could overwrite your root filesystems or flash
> your firmware. In general we cannot and don't try to protect root
> from such accidents.
> 

Yes. There are several scenarios that could disrupt access. There are
no bad things happening other than the expected read failures when the
device is actively in use.

thanks for the review.
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shuah Khan]

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 <shua...@osg.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 
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #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_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

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: vhci_hcd: Fix usb device and sockfd leaks

[Shuah Khan]

On 04/03/2018 12:56 AM, Greg KH wrote:
> On Mon, Apr 02, 2018 at 02:52:32PM -0600, Shuah Khan wrote:
>> vhci_hcd fails to do reset to put usb device and sockfd in the
>> module remove/stop paths. Fix the leak.
>>
>> Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
> 
> Should this be marked for the stable kernels?
> 

Yes please mark this for stables. I forgot to do that.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vhci_hcd: Fix usb device and sockfd leaks

[Shuah Khan]

vhci_hcd fails to do reset to put usb device and sockfd in the
module remove/stop paths. Fix the leak.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 473fb8a87289..bf8afe9b5883 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -243,7 +243,7 @@ enum usbip_side {
 #defineVUDC_EVENT_ERROR_USB(USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)
 #defineVUDC_EVENT_ERROR_MALLOC (USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)
 
-#defineVDEV_EVENT_REMOVED  (USBIP_EH_SHUTDOWN | USBIP_EH_BYE)
+#defineVDEV_EVENT_REMOVED (USBIP_EH_SHUTDOWN | USBIP_EH_RESET | 
USBIP_EH_BYE)
 #defineVDEV_EVENT_DOWN (USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 #defineVDEV_EVENT_ERROR_TCP(USBIP_EH_SHUTDOWN | USBIP_EH_RESET)
 #defineVDEV_EVENT_ERROR_MALLOC (USBIP_EH_SHUTDOWN | USBIP_EH_UNUSABLE)
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vhc_hcd: prevent module being removed while device are attached

[Shuah Khan]

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 <shua...@osg.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 
 #include 
 #include 
+#include 
 
 #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_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;
+   }
+
/* Extract socket from fd. */
socket = sockfd_lookup(sockfd, );
-   if (!socket)
-   return -EINVAL;
+   if (!socket) {
+   ret = -EINVAL;
+   goto error;
+   }
 
/* now need lock until setting vdev status as used */
 
@@ -362,7 +373,8 @@ static ssize_t attach_store(struct device *dev, struct 
device_attribute *attr,
 * Will be retried from userspace
 * if there's another free port.
 */
-   return -EBUSY;
+   ret = -EBUSY;
+   goto error;
}
 
dev_info(dev, "pdev(%u) rhport(%u) sockfd(%d)\n",
@@ -386,6 +398,11 @@ static ssize_t attach_store(struct device *dev, struct 
device_attribute *attr,
rh_port_connect(vdev, speed);
 
return count;
+
+error:
+   module_put(THIS_MODULE);
+module_get_err:
+   return ret;
 }
 static DEVICE_ATTR_WO(attach);
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: tools: usbipd: exclude exported devices from exportable device list

[Shuah Khan]

usbipd includes exported devices in response to exportable device list.
Exclude exported devices from exportable device list to avoid:

- import requests for devices that are exported only to fail the request.
- revealing devices that are imported by a client to another client.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbipd.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/src/usbipd.c b/tools/usb/usbip/src/usbipd.c
index f8ff735eb100..32864c52942d 100644
--- a/tools/usb/usbip/src/usbipd.c
+++ b/tools/usb/usbip/src/usbipd.c
@@ -175,10 +175,21 @@ static int send_reply_devlist(int connfd)
struct list_head *j;
int rc, i;
 
+   /*
+* Exclude devices that are already exported to a client from
+* the exportable device list to avoid:
+*  - import requests for devices that are exported only to
+*fail the request.
+*  - revealing devices that are imported by a client to
+*another client.
+*/
+
reply.ndev = 0;
/* number of exported devices */
list_for_each(j, >edev_list) {
-   reply.ndev += 1;
+   edev = list_entry(j, struct usbip_exported_device, node);
+   if (edev->status != SDEV_ST_USED)
+   reply.ndev += 1;
}
info("exportable devices: %d", reply.ndev);
 
@@ -197,6 +208,9 @@ static int send_reply_devlist(int connfd)
 
list_for_each(j, >edev_list) {
edev = list_entry(j, struct usbip_exported_device, node);
+   if (edev->status == SDEV_ST_USED)
+   continue;
+
dump_usb_device(>udev);
memcpy(_udev, >udev, sizeof(pdu_udev));
usbip_net_pack_usb_device(1, _udev);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] usbip: usbip_host_common: Use new error codes to return request status

[Shuah Khan]

Currently ST_OK and ST_NA are the only values used to communicate
status of a request from a client. Use new error codes to clearly
indicate what failed. For example, when client sends request to
import a device that isn't export-able, send ST_DEV_BUSY to the client.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/libsrc/usbip_host_common.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
b/tools/usb/usbip/libsrc/usbip_host_common.c
index 6ff7b601f854..dc93fadbee96 100644
--- a/tools/usb/usbip/libsrc/usbip_host_common.c
+++ b/tools/usb/usbip/libsrc/usbip_host_common.c
@@ -234,14 +234,17 @@ int usbip_export_device(struct usbip_exported_device 
*edev, int sockfd)
switch (edev->status) {
case SDEV_ST_ERROR:
dbg("status SDEV_ST_ERROR");
+   ret = ST_DEV_ERR;
break;
case SDEV_ST_USED:
dbg("status SDEV_ST_USED");
+   ret = ST_DEV_BUSY;
break;
default:
dbg("status unknown: 0x%x", edev->status);
+   ret = -1;
}
-   return -1;
+   return ret;
}
 
/* only the first interface is true */
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usbip: tools: add more error codes for usbip request/reply messages

[Shuah Khan]

Currently ST_OK and ST_NA are the only values defined to communicate
status of a request from a client. Add more error codes to clearly
indicate what failed. For example, when client sends request to import
a device that isn't export-able, server can send a specific error code
to the client.

Existing defines are moved to a common header in libsrc to be included
in the libusbip_la-usbip_common.o to be used by all the usbip tools.
Supporting interface to print error strings is added to the common lib.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/libsrc/usbip_common.c | 23 +++
 tools/usb/usbip/libsrc/usbip_common.h | 11 +++
 tools/usb/usbip/src/usbip_network.h   |  4 +---
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index 001bb8e8f668..bb424638d75b 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -66,6 +66,29 @@ const char *usbip_speed_string(int num)
return "Unknown Speed";
 }
 
+struct op_common_status_string {
+   int num;
+   char *desc;
+};
+
+static struct op_common_status_string op_common_status_strings[] = {
+   { ST_OK,"Request Completed Successfully" },
+   { ST_NA,"Request Failed" },
+   { ST_DEV_BUSY,  "Device busy (exported)" },
+   { ST_DEV_ERR,   "Device in error state" },
+   { ST_NODEV, "Device not found" },
+   { ST_ERROR, "Unexpected response" },
+   { 0, NULL}
+};
+
+const char *usbip_op_common_status_string(int status)
+{
+   for (int i = 0; op_common_status_strings[i].desc != NULL; i++)
+   if (op_common_status_strings[i].num == status)
+   return op_common_status_strings[i].desc;
+
+   return "Unknown Op Common Status";
+}
 
 #define DBG_UDEV_INTEGER(name)\
dbg("%-20s = %x", to_string(name), (int) udev->name)
diff --git a/tools/usb/usbip/libsrc/usbip_common.h 
b/tools/usb/usbip/libsrc/usbip_common.h
index e45ec9d2fdbc..73a367a7fa10 100644
--- a/tools/usb/usbip/libsrc/usbip_common.h
+++ b/tools/usb/usbip/libsrc/usbip_common.h
@@ -43,6 +43,16 @@
 #define SYSFS_PATH_MAX 256
 #define SYSFS_BUS_ID_SIZE  32
 
+/* Defines for op_code status in server/client op_common PDUs */
+#define ST_OK  0x00
+#define ST_NA  0x01
+   /* Device requested for import is not available */
+#define ST_DEV_BUSY0x02
+   /* Device requested for import is in error state */
+#define ST_DEV_ERR 0x03
+#define ST_NODEV   0x04
+#define ST_ERROR   0x05
+
 extern int usbip_use_syslog;
 extern int usbip_use_stderr;
 extern int usbip_use_debug ;
@@ -130,6 +140,7 @@ int read_usb_interface(struct usbip_usb_device *udev, int i,
 
 const char *usbip_speed_string(int num);
 const char *usbip_status_string(int32_t status);
+const char *usbip_op_common_status_string(int status);
 
 int usbip_names_init(char *);
 void usbip_names_free(void);
diff --git a/tools/usb/usbip/src/usbip_network.h 
b/tools/usb/usbip/src/usbip_network.h
index 7032687621d3..b6a2f9be888c 100644
--- a/tools/usb/usbip/src/usbip_network.h
+++ b/tools/usb/usbip/src/usbip_network.h
@@ -27,9 +27,7 @@ struct op_common {
 #define OP_REPLY   (0x00 << 8)
uint16_t code;
 
-   /* add more error code */
-#define ST_OK  0x00
-#define ST_NA  0x01
+   /* status codes defined in usbip_common.h */
uint32_t status; /* op_code status (for reply) */
 
 } __attribute__((packed));
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] usbip: tools: change to use new error codes in server reply messages

[Shuah Khan]

Changed usbip_network, usbip_attach, usbip_list, and usbipd to use
and propagate the new error codes in server reply messages.

usbip_net_recv_op_common() is changed to take a pointer to status
return the status returned in the op_common.status to callers.

usbip_attach and usbip_list use the common interface to print error
messages to indicate why the request failed.

With this change the messages say why a request failed:

- when a client requests a device that is already exported:

usbip attach -r server_name -b 3-10.2
usbip: error: Attach Request for 3-10.2 failed - Device busy (exported)

- when a client requests a device that isn't exportable,

usbip attach -r server_name -b 3-10.4
usbip: error: Attach Request for 3-10.4 failed - Device not found

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip_attach.c  | 11 +--
 tools/usb/usbip/src/usbip_list.c|  6 --
 tools/usb/usbip/src/usbip_network.c |  6 +-
 tools/usb/usbip/src/usbip_network.h |  2 +-
 tools/usb/usbip/src/usbipd.c| 18 +-
 5 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/tools/usb/usbip/src/usbip_attach.c 
b/tools/usb/usbip/src/usbip_attach.c
index f60738735398..ba88728483ff 100644
--- a/tools/usb/usbip/src/usbip_attach.c
+++ b/tools/usb/usbip/src/usbip_attach.c
@@ -135,6 +135,7 @@ static int query_import_device(int sockfd, char *busid)
struct op_import_request request;
struct op_import_reply   reply;
uint16_t code = OP_REP_IMPORT;
+   int status;
 
memset(, 0, sizeof(request));
memset(, 0, sizeof(reply));
@@ -157,9 +158,10 @@ static int query_import_device(int sockfd, char *busid)
}
 
/* receive a reply */
-   rc = usbip_net_recv_op_common(sockfd, );
+   rc = usbip_net_recv_op_common(sockfd, , );
if (rc < 0) {
-   err("recv op_common");
+   err("Attach Request for %s failed - %s\n",
+   busid, usbip_op_common_status_string(status));
return -1;
}
 
@@ -194,11 +196,8 @@ static int attach_device(char *host, char *busid)
}
 
rhport = query_import_device(sockfd, busid);
-   if (rhport < 0) {
-   err("Attach request for Device %s. Is this device exported?",
-   busid);
+   if (rhport < 0)
return -1;
-   }
 
close(sockfd);
 
diff --git a/tools/usb/usbip/src/usbip_list.c b/tools/usb/usbip/src/usbip_list.c
index d65a9f444174..8d4ccf4b9480 100644
--- a/tools/usb/usbip/src/usbip_list.c
+++ b/tools/usb/usbip/src/usbip_list.c
@@ -62,6 +62,7 @@ static int get_exported_devices(char *host, int sockfd)
struct usbip_usb_interface uintf;
unsigned int i;
int rc, j;
+   int status;
 
rc = usbip_net_send_op_common(sockfd, OP_REQ_DEVLIST, 0);
if (rc < 0) {
@@ -69,9 +70,10 @@ static int get_exported_devices(char *host, int sockfd)
return -1;
}
 
-   rc = usbip_net_recv_op_common(sockfd, );
+   rc = usbip_net_recv_op_common(sockfd, , );
if (rc < 0) {
-   dbg("usbip_net_recv_op_common failed");
+   err("Exported Device List Request failed - %s\n",
+   usbip_op_common_status_string(status));
return -1;
}
 
diff --git a/tools/usb/usbip/src/usbip_network.c 
b/tools/usb/usbip/src/usbip_network.c
index 90325fa8bc38..8ffcd47d9638 100644
--- a/tools/usb/usbip/src/usbip_network.c
+++ b/tools/usb/usbip/src/usbip_network.c
@@ -163,7 +163,7 @@ int usbip_net_send_op_common(int sockfd, uint32_t code, 
uint32_t status)
return 0;
 }
 
-int usbip_net_recv_op_common(int sockfd, uint16_t *code)
+int usbip_net_recv_op_common(int sockfd, uint16_t *code, int *status)
 {
struct op_common op_common;
int rc;
@@ -191,10 +191,14 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
if (op_common.code != *code) {
dbg("unexpected pdu %#0x for %#0x", op_common.code,
*code);
+   /* return error status */
+   *status = ST_ERROR;
goto err;
}
}
 
+   *status = op_common.status;
+
if (op_common.status != ST_OK) {
dbg("request failed at peer: %d", op_common.status);
goto err;
diff --git a/tools/usb/usbip/src/usbip_network.h 
b/tools/usb/usbip/src/usbip_network.h
index b6a2f9be888c..555215eae43e 100644
--- a/tools/usb/usbip/src/usbip_network.h
+++ b/tools/usb/usbip/src/usbip_network.h
@@ -174,7 +174,7 @@ void usbip_net_pack_usb_interface(int pack, struct 
usbip_usb_interface *uinf);
 ssize_t usbip_net_recv(int sockfd, void *buff, size_t bufflen);
 ssize_t usbip_net_send(int sockfd, void *buff, size_t bufflen);
 int usbip_net_send_op_c

[PATCH 0/3] More error codes for usbip request/reply messages

[Shuah Khan]

Currently ST_OK and ST_NA are the only values defined to communicate
status of a request from a client. Client doesn't know what failed
and prints a cryptic error message that something failed.

This patch series adds more error codes to clearly indicate what failed.
For example, when client sends request to import a device that isn't
export-able, server can send a specific error code to the client.

The first patch moves existing error codes to a common library header
and adds more codes. It also adds an interface to map the code to a
string to print messages from tools.

The second patch changes the server/host to return the right codes
in reply messages when client request fails.

The third patch changes the clinet tools to recognize the new error
codes and print messages.

With this change the messages say why a request failed:

- when a client requests a device that is already exported:

usbip attach -r server_name -b 3-10.2
usbip: error: Attach Request for 3-10.2 failed - Device busy (exported)

- when a client requests a device that isn't exportable,

usbip attach -r server_name -b 3-10.4
usbip: error: Attach Request for 3-10.4 failed - Device not found

Shuah Khan (3):
  usbip: tools: add more error codes for usbip request/reply messages
  usbip: usbip_host_common: Use new error codes to return request status
  usbip: tools: change to use new error codes in server reply messages

 tools/usb/usbip/libsrc/usbip_common.c  | 23 +++
 tools/usb/usbip/libsrc/usbip_common.h  | 11 +++
 tools/usb/usbip/libsrc/usbip_host_common.c |  5 -
 tools/usb/usbip/src/usbip_attach.c | 11 +--
 tools/usb/usbip/src/usbip_list.c   |  6 --
 tools/usb/usbip/src/usbip_network.c|  6 +-
 tools/usb/usbip/src/usbip_network.h|  6 ++
 tools/usb/usbip/src/usbipd.c   | 18 +-
 8 files changed, 63 insertions(+), 23 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Shuah Khan]

On 03/06/2018 01:35 AM, Salvador Fandiño wrote:
> 
> 
> On 03/06/2018 01:03 AM, Shuah Khan wrote:
>> On 03/05/2018 02:00 AM, Salvador Fandiño wrote:
>>> On 02/21/2018 01:35 AM, Shuah Khan wrote:
>>>> Hi Salvador,
>>>>
>>>> On 01/30/2018 01:36 AM, Salvador Fandino wrote:
>>>>> Let me start by explaining the problem that have motivated me to write
>>>>> this patches:
>>>>>
>>>>> I work on the QVD, a virtual desktop platform for Linux. This software
>>>>> runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
>>>>> containers, and makes then available through the network to remote
>>>>> users.
>>>>>
>>>>> Supporting USB devices is a common feature customers have been
>>>>> requesting us for a long time (in order to use, for instance, remote
>>>>> signature pads, bar-code scanners, fingerprint readers, etc.). So, we
>>>>> have been working on that feature using the USB/IP layer on the
>>>>> kernel.
>>>>>
>>>>> Connecting and disconnecting devices and transferring data works
>>>>> seamless for the devices listed above. But we also want to make the
>>>>> usbip operations private to the container where they are run.  For
>>>>> instance, it is unacceptable for our product, that one user could list
>>>>> the devices connected by other users or access them.
>>>>>
>>>>> We can control how can access every device using cgroups once those
>>>>> are attached, but the usbip layer is not providing any mechanism for
>>>>> controlling who can attach, detach or list the devices.
>> In this use-case:
>>
>> - does a container act as usbip client and attach devices from their
>>    host?
>> - do containers attach remote devices from other systems?
> In my particular case devices are imported from remote machines. But well, 
> the thing is that I don't care where the connections come from, they could 
> even be devices emulated in user space.
> 
>> Is the core of the problem really that any remote system can import without
>> a provision for being able to restrict export to a set of remote machines?
>> If so, this is a generic problem even without containers and I would like
>> to solve this with a generic solution that works in all cases, not just for
>> containers.
> No, that is a different issue. You are talking about controlling which 
> devices can be connected, from which hosts, etc. That is an interesting 
> problem but not the one I am trying to tackle here.
> 

Not entirely. These problems are linked if you use usbip driver and usbip
tools. USBIp driver is intended to be used in conjunction with the usbip
tools.

> I don't mind which every user does inside its container as far as it does not 
> interfere which other users. In practice that means:
> 
> 1- Not being able to attach/detach devices in other container

How do container attach/detach in other containers in your setup?

> 2- Not being able to list devices attached in other containers

How do container list devices in other containers in your setup?

> 3- Not being able to access devices attached in other containers.
> 
> Point 3 is already enforceable using the devices hierarchy in cgroups. For 
> points 1 and 2, my proposition is making every vhci_hcd device have its own 
> fully independent sysfs directory (instead of all of them going through 
> vhci_hcd.0) so that they can be selectively exposed with rw permissions 
> inside the containers.
> 
> 
> 
>> The approach in this patch series appears to solve the problem just for
>> containers.
>>
>>>> Did you explore a solution to add a mechanism for access control to
>>>> usbip?
>>> Could you elaborate on that?
>>>
>>> For "usbip", do you mean the user space tools?
>>> If that is the case, I don't think it would be enough.
>>> My aim is to limit vhci usage from containers and I have no control about 
>>> what runs inside the containers. So, a mangled usbip tool-set could > > be 
>>> used by a malicious user to circumvent any access control set there.>
>> I mean the driver. There might be changes necessary in the user-space
>> as well depending on how the access controls are implemented. I am not
>> proposing implementing access controls in the user-space.
>>
>>
>>> IMO, there is no other choice but to control access to VHCI at the kernel 
>>> level.
>>>
>> Probably. Please give

Re: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Shuah Khan]

On 03/05/2018 02:00 AM, Salvador Fandiño wrote:
> On 02/21/2018 01:35 AM, Shuah Khan wrote:
>> Hi Salvador,
>>
>> On 01/30/2018 01:36 AM, Salvador Fandino wrote:
>>> Let me start by explaining the problem that have motivated me to write
>>> this patches:
>>>
>>> I work on the QVD, a virtual desktop platform for Linux. This software
>>> runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
>>> containers, and makes then available through the network to remote
>>> users.
>>>
>>> Supporting USB devices is a common feature customers have been
>>> requesting us for a long time (in order to use, for instance, remote
>>> signature pads, bar-code scanners, fingerprint readers, etc.). So, we
>>> have been working on that feature using the USB/IP layer on the
>>> kernel.
>>>
>>> Connecting and disconnecting devices and transferring data works
>>> seamless for the devices listed above. But we also want to make the
>>> usbip operations private to the container where they are run.  For
>>> instance, it is unacceptable for our product, that one user could list
>>> the devices connected by other users or access them.
>>>
>>> We can control how can access every device using cgroups once those
>>> are attached, but the usbip layer is not providing any mechanism for
>>> controlling who can attach, detach or list the devices.

In this use-case:

- does a container act as usbip client and attach devices from their
  host?
- do containers attach remote devices from other systems?

Is the core of the problem really that any remote system can import without
a provision for being able to restrict export to a set of remote machines?
If so, this is a generic problem even without containers and I would like
to solve this with a generic solution that works in all cases, not just for
containers.

The approach in this patch series appears to solve the problem just for
containers.

>>
>> Did you explore a solution to add a mechanism for access control to
>> usbip?
> 
> Could you elaborate on that?
> 
> For "usbip", do you mean the user space tools? 
> If that is the case, I don't think it would be enough.
> My aim is to limit vhci usage from containers and I have no control about 
> what runs inside the containers. So, a mangled usbip tool-set could > > be 
> used by a malicious user to circumvent any access control set there.> 

I mean the driver. There might be changes necessary in the user-space
as well depending on how the access controls are implemented. I am not
proposing implementing access controls in the user-space.


> IMO, there is no other choice but to control access to VHCI at the kernel 
> level.
> 

Probably. Please give as many details as possible on your environment
for me to make a call on if this problem can be solved in a different
way.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: tools usbip_attach: Fix cryptic error messages

[Shuah Khan]

On 02/27/2018 03:45 PM, Krzysztof Opasiak wrote:
> 
> 
> On 02/27/2018 11:23 PM, Shuah Khan wrote:
>> Attach device error message is cryptic and useless. Fix it to be
>> informative.
>>
>> Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
>> ---
>>   tools/usb/usbip/src/usbip_attach.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/usb/usbip/src/usbip_attach.c 
>> b/tools/usb/usbip/src/usbip_attach.c
>> index 7f07b2d50f59..f60738735398 100644
>> --- a/tools/usb/usbip/src/usbip_attach.c
>> +++ b/tools/usb/usbip/src/usbip_attach.c
>> @@ -195,7 +195,8 @@ static int attach_device(char *host, char *busid)
>>     rhport = query_import_device(sockfd, busid);
>>   if (rhport < 0) {
>> -    err("query");
>> +    err("Attach request for Device %s. Is this device exported?",
>> +    busid);
>>   return -1;
>>   }
> 
> The code itself is ok and you may put my:
> 
> Reviewed-by: Krzysztof Opasiak <k.opas...@samsung.com>
> 
> but just because of my curiosity, there is a number of places in usbip tools 
> where the same level of descriptiveness is used for error message. Why did 
> you choose to fix this one not any other or all others?;)
> 
> Best regards,

Yes there are several very cryptic and useless error message all over the
place in usbip tools that could use refinement. This just happens to be the
one that I seem to run into very frequently. It frustrated me enough that
I decided to fix this one first. :)

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: tools usbip_network: Fix cryptic error messages

[Shuah Khan]

Kernel and tool version mismatch message is cryptic. Fix it to be
informative.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip_network.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/usb/usbip/src/usbip_network.c 
b/tools/usb/usbip/src/usbip_network.c
index b4c37e76a6e0..90325fa8bc38 100644
--- a/tools/usb/usbip/src/usbip_network.c
+++ b/tools/usb/usbip/src/usbip_network.c
@@ -179,8 +179,8 @@ int usbip_net_recv_op_common(int sockfd, uint16_t *code)
PACK_OP_COMMON(0, _common);
 
if (op_common.version != USBIP_VERSION) {
-   dbg("version mismatch: %d %d", op_common.version,
-   USBIP_VERSION);
+   err("USBIP Kernel and tool version mismatch: %d %d:",
+   op_common.version, USBIP_VERSION);
goto err;
}
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: tools usbip_attach: Fix cryptic error messages

[Shuah Khan]

Attach device error message is cryptic and useless. Fix it to be
informative.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip_attach.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/usb/usbip/src/usbip_attach.c 
b/tools/usb/usbip/src/usbip_attach.c
index 7f07b2d50f59..f60738735398 100644
--- a/tools/usb/usbip/src/usbip_attach.c
+++ b/tools/usb/usbip/src/usbip_attach.c
@@ -195,7 +195,8 @@ static int attach_device(char *host, char *busid)
 
rhport = query_import_device(sockfd, busid);
if (rhport < 0) {
-   err("query");
+   err("Attach request for Device %s. Is this device exported?",
+   busid);
return -1;
}
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.16] staging: usbip stub_rx fix static checker warning on unnecessary checks

[Shuah Khan]

Upstream commit 10c901209306
("usbip: stub_rx: fix static checker warning on unnecessary checks")

Back-port fix for static checker warning on unnecessary checks

smatch warnings:
drivers/staging/usbip/stub_rx.c:360 get_pipe() warn: impossible
condition '(pdu->u.cmd_submit.transfer_buffer_length > ((~0 >> 1))) =>
(s32min-s32max > s32max)'
drivers/staging/usbip/stub_rx.c:501 stub_recv_cmd_submit() warn: always
true condition '(pdu->u.cmd_submit.transfer_buffer_length <= ((~0 >>
1))) => (s32min-s32max <= s32max)'

Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/staging/usbip/stub_rx.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/staging/usbip/stub_rx.c b/drivers/staging/usbip/stub_rx.c
index 35f59747122a..d8544ab9577b 100644
--- a/drivers/staging/usbip/stub_rx.c
+++ b/drivers/staging/usbip/stub_rx.c
@@ -356,14 +356,6 @@ static int get_pipe(struct stub_device *sdev, struct 
usbip_header *pdu)
 
epd = >desc;
 
-   /* validate transfer_buffer_length */
-   if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
-   dev_err(>udev->dev,
-   "CMD_SUBMIT: -EMSGSIZE transfer_buffer_length %d\n",
-   pdu->u.cmd_submit.transfer_buffer_length);
-   return -1;
-   }
-
if (usb_endpoint_xfer_control(epd)) {
if (dir == USBIP_DIR_OUT)
return usb_sndctrlpipe(udev, epnum);
@@ -497,8 +489,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
}
 
/* allocate urb transfer buffer, if needed */
-   if (pdu->u.cmd_submit.transfer_buffer_length > 0 &&
-   pdu->u.cmd_submit.transfer_buffer_length <= INT_MAX) {
+   if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
priv->urb->transfer_buffer =
kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
GFP_KERNEL);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: vudc: fix null pointer dereference on udc->lock

[Shuah Khan]

On 02/22/2018 10:39 AM, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Currently the driver attempts to spin lock on udc->lock before a NULL
> pointer check is performed on udc, hence there is a potential null
> pointer dereference on udc->lock.  Fix this by moving the null check
> on udc before the lock occurs.
> 
> Fixes: ea6873a45a22 ("usbip: vudc: Add SysFS infrastructure for VUDC")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/usb/usbip/vudc_sysfs.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vudc_sysfs.c b/drivers/usb/usbip/vudc_sysfs.c
> index d86f7291..6dcd3ff655c3 100644
> --- a/drivers/usb/usbip/vudc_sysfs.c
> +++ b/drivers/usb/usbip/vudc_sysfs.c
> @@ -105,10 +105,14 @@ static ssize_t usbip_sockfd_store(struct device *dev, 
> struct device_attribute *a
>   if (rv != 0)
>   return -EINVAL;
>  
> + if (!udc) {
> + dev_err(dev, "no device");
> + return -ENODEV;
> + }
>   spin_lock_irqsave(>lock, flags);
>   /* Don't export what we don't have */
> - if (!udc || !udc->driver || !udc->pullup) {
> - dev_err(dev, "no device or gadget not bound");
> + if (!udc->driver || !udc->pullup) {
> + dev_err(dev, "gadget not bound");
>   ret = -ENODEV;
>   goto unlock;
>   }
> 

Thanks for the patch. Looks good to me.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Shuah Khan]

Hi Salvador,

On 01/30/2018 01:36 AM, Salvador Fandino wrote:
> Let me start by explaining the problem that have motivated me to write
> this patches:
> 
> I work on the QVD, a virtual desktop platform for Linux. This software
> runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
> containers, and makes then available through the network to remote
> users.
> 
> Supporting USB devices is a common feature customers have been
> requesting us for a long time (in order to use, for instance, remote
> signature pads, bar-code scanners, fingerprint readers, etc.). So, we
> have been working on that feature using the USB/IP layer on the
> kernel.
> 
> Connecting and disconnecting devices and transferring data works
> seamless for the devices listed above. But we also want to make the
> usbip operations private to the container where they are run.  For
> instance, it is unacceptable for our product, that one user could list
> the devices connected by other users or access them.
> 
> We can control how can access every device using cgroups once those
> are attached, but the usbip layer is not providing any mechanism for
> controlling who can attach, detach or list the devices.

Did you explore a solution to add a mechanism for access control to
usbip?

> 
> So, we got the idea that in order to enforce that remote usbip devices
> are only visible inside the container where they were imported, we
> could conveniently mount-bind inside every container just one of the
> vhci_hcd directories below /sys/devices/platform. So that it is as if
> every container had a vhci_hcd just for itself (and also, we restrict
> access to the matching USB ports in cgroups).
> 
> Unfortunately, all the vhci_hcd.* devices are controlled through
> attributes in vhci_hcd.0 making our approach fail and so... well, that
> is what this patch series changes. It makes every vhci_hcd device
> controllable through attributes inside its own sysfs directory.> 
> The first patch, does that in the kernel, and the second and third
> patches change user space, adapting the libusbip and the usbip tools
> code respectively.
> 
> Then there is a fourth patch, that allows to create much more USB
> hubs per machine. It was limited to 64 and we are running thousands of
> containers (every one requiring a hub) per host.
> 
> These changes are not completely backward compatible. In the sysfs
> side, besides moving around the attribute files, now the port numbers
> go from 0 to CONFIG_USBIP_VHCI_HC_PORTS * 2 - 1 and are reused for
> every vhci_hcd device. I could have maintained the absolute numeration
> but I think reusing the numbers is a better and simpler approach.

Not being able to maintain backwards compatibility is an issue. This is
a considerable change to the user interface.

> 
> I have considered that until very recently, support for vhci_hcd
> devices above the .0 was broken in the uspip tools (see
> 1ac7c8a78be85f84b019d3d2742d1a9f07255cc5 "usbip: fix usbip attach to
> find a port that matches the requested speed") and that has been the
> place where I have set the bar for backward compatibility: usage of
> the tools must remain unchanged for accessing "vhci_hcd.0", but may
> require changes otherwise. The same is true for the library functions
> which now provides new functions for selecting the target vhci_hcd
> device. The old functions now always target "vhci_hcd.0".
> 
> So, for instance, now, "usbip port" by default only shows "vhci_hcd.0"
> ports but "usbip port -a" shows all of them, and, for instance, "usbip
> port -i 4", shows the ports in "vhci_hcd.4".
> 

I am going to play with these patches and how extensive the changes are
for users. In the meantime, maybe you can explore alternatives that don't
break backwards compatibility.

thanks,
-- Shuah


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 4/9] usbip: Fix potential format overflow in userspace tools

[Shuah Khan]

Upstream commit e5dfa3f902b9 ("usbip: Fix potential format overflow in
userspace tools")

The usbip userspace tools call sprintf()/snprintf() and don't check for
the return value which can lead the paths to overflow, truncating the
final file in the path.

More urgently, GCC 7 now warns that these aren't checked with
-Wformat-overflow, and with -Werror enabled in configure.ac, that makes
these tools unbuildable.

This patch fixes these problems by replacing sprintf() with snprintf()
in one place and adding checks for the return value of snprintf().

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/libsrc/usbip_common.c  |  9 -
 tools/usb/usbip/libsrc/usbip_host_driver.c | 27 ++-
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710473de..8000445ff884 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -215,9 +215,16 @@ int read_usb_interface(struct usbip_usb_device *udev, int 
i,
   struct usbip_usb_interface *uinf)
 {
char busid[SYSFS_BUS_ID_SIZE];
+   int size;
struct udev_device *sif;
 
-   sprintf(busid, "%s:%d.%d", udev->busid, udev->bConfigurationValue, i);
+   size = snprintf(busid, sizeof(busid), "%s:%d.%d",
+   udev->busid, udev->bConfigurationValue, i);
+   if (size < 0 || (unsigned int)size >= sizeof(busid)) {
+   err("busid length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(busid));
+   return -1;
+   }
 
sif = udev_device_new_from_subsystem_sysname(udev_context, "usb", 
busid);
if (!sif) {
diff --git a/tools/usb/usbip/libsrc/usbip_host_driver.c 
b/tools/usb/usbip/libsrc/usbip_host_driver.c
index bef08d5c44e8..071b9ce99420 100644
--- a/tools/usb/usbip/libsrc/usbip_host_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_host_driver.c
@@ -39,13 +39,19 @@ struct udev *udev_context;
 static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
 {
char status_attr_path[SYSFS_PATH_MAX];
+   int size;
int fd;
int length;
char status;
int value = 0;
 
-   snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
-udev->path);
+   size = snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
+   udev->path);
+   if (size < 0 || (unsigned int)size >= sizeof(status_attr_path)) {
+   err("usbip_status path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(status_attr_path));
+   return -1;
+   }
 
fd = open(status_attr_path, O_RDONLY);
if (fd < 0) {
@@ -225,6 +231,7 @@ int usbip_host_export_device(struct usbip_exported_device 
*edev, int sockfd)
 {
char attr_name[] = "usbip_sockfd";
char sockfd_attr_path[SYSFS_PATH_MAX];
+   int size;
char sockfd_buff[30];
int ret;
 
@@ -244,10 +251,20 @@ int usbip_host_export_device(struct usbip_exported_device 
*edev, int sockfd)
}
 
/* only the first interface is true */
-   snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
-edev->udev.path, attr_name);
+   size = snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
+   edev->udev.path, attr_name);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_attr_path)) {
+   err("exported device path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_attr_path));
+   return -1;
+   }
 
-   snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_buff)) {
+   err("socket length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_buff));
+   return -1;
+   }
 
ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
strlen(sockfd_buff));
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 0/9] Backports for security and critical bug fixes

[Shuah Khan]

As I started backporting security fixes, found a few problems
that prevent tools to build on newer gcc releases, deadlock bug,
and another bug that prevents client from being able to use
imported devices.

This patch series consists of security fixes and fixes to critical
bugs.

Andrew Goodbody (1):
  usb: usbip: Fix possible deadlocks reported by lockdep

Shuah Khan (8):
  usbip: fix stub_rx: get_pipe() to validate endpoint number
  usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input
  usbip: prevent vhci_hcd driver from leaking a socket pointer address
  usbip: Fix potential format overflow in userspace tools
  usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit
  usbip: prevent leaking socket pointer address in messages
  usbip: stub: stop printing kernel pointer addresses in messages
  usbip: vhci: stop printing kernel pointer addresses in messages

 drivers/usb/usbip/stub_dev.c   |   3 +-
 drivers/usb/usbip/stub_main.c  |   5 +-
 drivers/usb/usbip/stub_rx.c|  53 +++
 drivers/usb/usbip/stub_tx.c|   4 +-
 drivers/usb/usbip/usbip_common.c   |  15 ++---
 drivers/usb/usbip/usbip_common.h   |   1 +
 drivers/usb/usbip/usbip_event.c|   5 +-
 drivers/usb/usbip/vhci_hcd.c   | 102 +++--
 drivers/usb/usbip/vhci_rx.c|  53 ---
 drivers/usb/usbip/vhci_sysfs.c |  45 +++--
 drivers/usb/usbip/vhci_tx.c|  17 +++--
 tools/usb/usbip/libsrc/usbip_common.c  |   9 ++-
 tools/usb/usbip/libsrc/usbip_host_driver.c |  27 ++--
 tools/usb/usbip/libsrc/vhci_driver.c   |   8 +--
 14 files changed, 205 insertions(+), 142 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 1/9] usbip: fix stub_rx: get_pipe() to validate endpoint number

[Shuah Khan]

Upstream commit 635f545a7e8b ("usbip: fix stub_rx: get_pipe() to
validate endpoint number")

get_pipe() routine doesn't validate the input endpoint number
and uses to reference ep_in and ep_out arrays. Invalid endpoint
number can trigger BUG(). Range check the epnum and returning
error instead of calling BUG().

Change caller stub_recv_cmd_submit() to handle the get_pipe()
error return.

Reported-by: Secunia Research <v...@secunia.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_rx.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 00e475c51a12..2e07acda456e 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -347,15 +347,15 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
 
+   if (epnum < 0 || epnum > 15)
+   goto err_ret;
+
if (dir == USBIP_DIR_IN)
ep = udev->ep_in[epnum & 0x7f];
else
ep = udev->ep_out[epnum & 0x7f];
-   if (!ep) {
-   dev_err(>interface->dev, "no such endpoint?, %d\n",
-   epnum);
-   BUG();
-   }
+   if (!ep)
+   goto err_ret;
 
epd = >desc;
if (usb_endpoint_xfer_control(epd)) {
@@ -386,9 +386,10 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
return usb_rcvisocpipe(udev, epnum);
}
 
+err_ret:
/* NOT REACHED */
dev_err(>interface->dev, "get pipe, epnum %d\n", epnum);
-   return 0;
+   return -1;
 }
 
 static void masking_bogus_flags(struct urb *urb)
@@ -454,6 +455,9 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct usb_device *udev = sdev->udev;
int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
 
+   if (pipe == -1)
+   return;
+
priv = stub_priv_alloc(sdev, pdu);
if (!priv)
return;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 6/9] usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit

[Shuah Khan]

Upstream commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
support")

vhci_hcd clears all the bits port_status bits instead of clearing
just the USB_PORT_STAT_POWER bit when it handles ClearPortFeature:
USB_PORT_FEAT_POWER. This causes vhci_hcd attach to fail in a bad
state, leaving device unusable by the client. The device is still
attached and however client can't use it.

The problem was fixed as part of larger change to add USB3 Super Speed
support.

This patch isolates the one line fix to clear the USB_PORT_STAT_POWER
from the original patch.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 2c7bed7b19d6..e480b924a04c 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -279,7 +279,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(
" ClearPortFeature: USB_PORT_FEAT_POWER\n");
-   dum->port_status[rhport] = 0;
+   dum->port_status[rhport] &= ~USB_PORT_STAT_POWER;
dum->resuming = 0;
break;
case USB_PORT_FEAT_C_RESET:
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 7/9] usbip: prevent leaking socket pointer address in messages

[Shuah Khan]

Upstream commit 90120d15f4c3 ("usbip: prevent leaking socket pointer
address in messages")

usbip driver is leaking socket pointer address in messages. Remove
the messages that aren't useful and print sockfd in the ones that
are useful for debugging.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_dev.c |  3 +--
 drivers/usb/usbip/usbip_common.c | 15 ---
 drivers/usb/usbip/vhci_hcd.c |  2 +-
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index fac20e0434c0..8123a6b2eade 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -163,8 +163,7 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
 * step 1?
 */
if (ud->tcp_socket) {
-   dev_dbg(>udev->dev, "shutdown tcp_socket %p\n",
-   ud->tcp_socket);
+   dev_dbg(>udev->dev, "shutdown sockfd %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 9752b93f754e..1838f1b2c2fa 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -317,18 +317,14 @@ int usbip_recv(struct socket *sock, void *buf, int size)
struct msghdr msg;
struct kvec iov;
int total = 0;
-
/* for blocks of if (usbip_dbg_flag_xmit) */
char *bp = buf;
int osize = size;
 
-   usbip_dbg_xmit("enter\n");
-
-   if (!sock || !buf || !size) {
-   pr_err("invalid arg, sock %p buff %p size %d\n", sock, buf,
-  size);
+   if (!sock || !buf || !size)
return -EINVAL;
-   }
+
+   usbip_dbg_xmit("enter\n");
 
do {
sock->sk->sk_allocation = GFP_NOIO;
@@ -341,11 +337,8 @@ int usbip_recv(struct socket *sock, void *buf, int size)
msg.msg_flags  = MSG_NOSIGNAL;
 
result = kernel_recvmsg(sock, , , 1, size, MSG_WAITALL);
-   if (result <= 0) {
-   pr_debug("receive sock %p buf %p size %u ret %d total 
%d\n",
-sock, buf, size, result, total);
+   if (result <= 0)
goto err;
-   }
 
size -= result;
buf += result;
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index e480b924a04c..a57843e1173f 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -782,7 +782,7 @@ static void vhci_shutdown_connection(struct usbip_device 
*ud)
 
/* need this? see stub_dev.c */
if (ud->tcp_socket) {
-   pr_debug("shutdown tcp_socket %p\n", ud->tcp_socket);
+   pr_debug("shutdown sockfd %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 5/9] usb: usbip: Fix possible deadlocks reported by lockdep

[Shuah Khan]

From: Andrew Goodbody <andrew.goodb...@cambrionix.com>

Upstream commit 21619792d1ec ("usb: usbip: Fix possible deadlocks
reported by lockdep")

Change spin_lock calls to spin_lock_irqsave to prevent
attmpted recursive lock taking in interrupt context.

This patch fixes Bug 109351
  https://bugzilla.kernel.org/show_bug.cgi?id=109351

Signed-off-by: Andrew Goodbody <andrew.goodb...@cambrionix.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_event.c |  5 ++-
 drivers/usb/usbip/vhci_hcd.c| 88 -
 drivers/usb/usbip/vhci_rx.c | 30 --
 drivers/usb/usbip/vhci_sysfs.c  | 19 +
 drivers/usb/usbip/vhci_tx.c | 14 ---
 5 files changed, 91 insertions(+), 65 deletions(-)

diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 64933b993d7a..2580a32bcdff 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -117,11 +117,12 @@ EXPORT_SYMBOL_GPL(usbip_event_add);
 int usbip_event_happened(struct usbip_device *ud)
 {
int happened = 0;
+   unsigned long flags;
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
if (ud->event != 0)
happened = 1;
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
 
return happened;
 }
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index c02374b6049c..2c7bed7b19d6 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -121,9 +121,11 @@ static void dump_port_status_diff(u32 prev_status, u32 
new_status)
 
 void rh_port_connect(int rhport, enum usb_device_speed speed)
 {
+   unsigned long   flags;
+
usbip_dbg_vhci_rh("rh_port_connect %d\n", rhport);
 
-   spin_lock(_controller->lock);
+   spin_lock_irqsave(_controller->lock, flags);
 
the_controller->port_status[rhport] |= USB_PORT_STAT_CONNECTION
| (1 << USB_PORT_FEAT_C_CONNECTION);
@@ -139,22 +141,24 @@ void rh_port_connect(int rhport, enum usb_device_speed 
speed)
break;
}
 
-   spin_unlock(_controller->lock);
+   spin_unlock_irqrestore(_controller->lock, flags);
 
usb_hcd_poll_rh_status(vhci_to_hcd(the_controller));
 }
 
 static void rh_port_disconnect(int rhport)
 {
+   unsigned long   flags;
+
usbip_dbg_vhci_rh("rh_port_disconnect %d\n", rhport);
 
-   spin_lock(_controller->lock);
+   spin_lock_irqsave(_controller->lock, flags);
 
the_controller->port_status[rhport] &= ~USB_PORT_STAT_CONNECTION;
the_controller->port_status[rhport] |=
(1 << USB_PORT_FEAT_C_CONNECTION);
 
-   spin_unlock(_controller->lock);
+   spin_unlock_irqrestore(_controller->lock, flags);
usb_hcd_poll_rh_status(vhci_to_hcd(the_controller));
 }
 
@@ -182,13 +186,14 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
int retval;
int rhport;
int changed = 0;
+   unsigned long   flags;
 
retval = DIV_ROUND_UP(VHCI_NPORTS + 1, 8);
memset(buf, 0, retval);
 
vhci = hcd_to_vhci(hcd);
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
if (!HCD_HW_ACCESSIBLE(hcd)) {
usbip_dbg_vhci_rh("hw accessible flag not on?\n");
goto done;
@@ -209,7 +214,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
usb_hcd_resume_root_hub(hcd);
 
 done:
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
return changed ? retval : 0;
 }
 
@@ -230,6 +235,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
struct vhci_hcd *dum;
int retval = 0;
int rhport;
+   unsigned long   flags;
 
u32 prev_port_status[VHCI_NPORTS];
 
@@ -248,7 +254,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
 
dum = hcd_to_vhci(hcd);
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
 
/* store old status and compare now and old later */
if (usbip_dbg_flag_vhci_rh) {
@@ -402,7 +408,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
}
usbip_dbg_vhci_rh(" bye\n");
 
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
 
return retval;
 }
@@ -425,6 +431,7 @@ static void vhci_tx_urb(struct urb *urb)
 {
struct vhci_device *vdev = get_vdev(urb->dev);
struct vhci_priv *priv;
+   unsigned long flags;
 
if (!vdev) {
pr_err("could not get virtual devi

[PATCH 3.18 8/9] usbip: stub: stop printing kernel pointer addresses in messages

[Shuah Khan]

Upstream commit 248a22044366 ("usbip: stub: stop printing kernel
pointer addresses in messages")

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_main.c | 5 +++--
 drivers/usb/usbip/stub_rx.c   | 7 ++-
 drivers/usb/usbip/stub_tx.c   | 4 ++--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index af10f7b131a4..325b4c05acdd 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -252,11 +252,12 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
struct stub_priv *priv;
struct urb *urb;
 
-   dev_dbg(>udev->dev, "free sdev %p\n", sdev);
+   dev_dbg(>udev->dev, "Stub device cleaning up urbs\n");
 
while ((priv = stub_priv_pop(sdev))) {
urb = priv->urb;
-   dev_dbg(>udev->dev, "free urb %p\n", urb);
+   dev_dbg(>udev->dev, "free urb seqnum %lu\n",
+   priv->seqnum);
usb_kill_urb(urb);
 
kmem_cache_free(stub_priv_cache, priv);
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index f5533c99cd48..56cacb68040c 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -230,9 +230,6 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
if (priv->seqnum != pdu->u.cmd_unlink.seqnum)
continue;
 
-   dev_info(>urb->dev->dev, "unlink urb %p\n",
-priv->urb);
-
/*
 * This matched urb is not completed yet (i.e., be in
 * flight in usb hcd hardware/driver). Now we are
@@ -271,8 +268,8 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
ret = usb_unlink_urb(priv->urb);
if (ret != -EINPROGRESS)
dev_err(>urb->dev->dev,
-   "failed to unlink a urb %p, ret %d\n",
-   priv->urb, ret);
+   "failed to unlink a urb # %lu, ret %d\n",
+   priv->seqnum, ret);
 
return 0;
}
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index af858d52608a..f4dd30c56f36 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -201,8 +201,8 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 
/* 1. setup usbip_header */
setup_ret_submit_pdu(_header, urb);
-   usbip_dbg_stub_tx("setup txdata seqnum: %d urb: %p\n",
- pdu_header.base.seqnum, urb);
+   usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+ pdu_header.base.seqnum);
usbip_header_correct_endian(_header, 1);
 
iov[iovnum].iov_base = _header;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 2/9] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

Upstream commit c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path
to handle malicious input")

Harden CMD_SUBMIT path to handle malicious input that could trigger
large memory allocations. Add checks to validate transfer_buffer_length
and number_of_packets to protect against bad input requesting for
unbounded memory allocations. Validate early in get_pipe() and return
failure.

Reported-by: Secunia Research <v...@secunia.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_rx.c | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 2e07acda456e..f5533c99cd48 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -341,11 +341,13 @@ static struct stub_priv *stub_priv_alloc(struct 
stub_device *sdev,
return priv;
 }
 
-static int get_pipe(struct stub_device *sdev, int epnum, int dir)
+static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 {
struct usb_device *udev = sdev->udev;
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
+   int epnum = pdu->base.ep;
+   int dir = pdu->base.direction;
 
if (epnum < 0 || epnum > 15)
goto err_ret;
@@ -358,6 +360,7 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
goto err_ret;
 
epd = >desc;
+
if (usb_endpoint_xfer_control(epd)) {
if (dir == USBIP_DIR_OUT)
return usb_sndctrlpipe(udev, epnum);
@@ -380,6 +383,27 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
}
 
if (usb_endpoint_xfer_isoc(epd)) {
+   /* validate packet size and number of packets */
+   unsigned int maxp, packets, bytes;
+
+#define USB_EP_MAXP_MULT_SHIFT  11
+#define USB_EP_MAXP_MULT_MASK   (3 << USB_EP_MAXP_MULT_SHIFT)
+#define USB_EP_MAXP_MULT(m) \
+   (((m) & USB_EP_MAXP_MULT_MASK) >> USB_EP_MAXP_MULT_SHIFT)
+
+   maxp = usb_endpoint_maxp(epd);
+   maxp *= (USB_EP_MAXP_MULT(
+   __le16_to_cpu(epd->wMaxPacketSize)) + 1);
+   bytes = pdu->u.cmd_submit.transfer_buffer_length;
+   packets = DIV_ROUND_UP(bytes, maxp);
+
+   if (pdu->u.cmd_submit.number_of_packets < 0 ||
+   pdu->u.cmd_submit.number_of_packets > packets) {
+   dev_err(>udev->dev,
+   "CMD_SUBMIT: isoc invalid num packets %d\n",
+   pdu->u.cmd_submit.number_of_packets);
+   return -1;
+   }
if (dir == USBIP_DIR_OUT)
return usb_sndisocpipe(udev, epnum);
else
@@ -388,7 +412,7 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
 
 err_ret:
/* NOT REACHED */
-   dev_err(>interface->dev, "get pipe, epnum %d\n", epnum);
+   dev_err(>udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
return -1;
 }
 
@@ -453,7 +477,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct stub_priv *priv;
struct usbip_device *ud = >ud;
struct usb_device *udev = sdev->udev;
-   int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
+   int pipe = get_pipe(sdev, pdu);
 
if (pipe == -1)
return;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3.18 3/9] usbip: prevent vhci_hcd driver from leaking a socket pointer address

[Shuah Khan]

commit 2f2d0088eb93 ("usbip: prevent vhci_hcd driver from leaking a
socket pointer address")

When a client has a USB device attached over IP, the vhci_hcd driver is
locally leaking a socket pointer address via the

/sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
output when "usbip --debug port" is run.

Fix it to not leak. The socket pointer address is not used at the moment
and it was made visible as a convenient way to find IP address from
socket pointer address by looking up /proc/net/{tcp,tcp6}.

As this opens a security hole, the fix replaces socket pointer address
with sockfd.

Reported-by: Secunia Research <v...@secunia.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.h |  1 +
 drivers/usb/usbip/vhci_sysfs.c   | 26 +++---
 tools/usb/usbip/libsrc/vhci_driver.c |  8 
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 86b08475c254..f875ccaa55f9 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -261,6 +261,7 @@ struct usbip_device {
/* lock for status */
spinlock_t lock;
 
+   int sockfd;
struct socket *tcp_socket;
 
struct task_struct *tcp_rx;
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 211f43f67ea2..f05f1e0a2baf 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -39,16 +39,20 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
 
/*
 * output example:
-* prt sta spd dev socket   local_busid
-* 000 004 000 000 c5a7bb80 1-2.3
-* 001 004 000 000 d8cee980 2-3.4
+* prt  sta spd dev  sockfd local_busid
+*  004 000  03 1-2.3
+* 0001 004 000  04 2-3.4
 *
-* IP address can be retrieved from a socket pointer address by looking
-* up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
-* port number and its peer IP address.
+* Output includes socket fd instead of socket pointer address to
+* avoid leaking kernel memory address in:
+*  /sys/devices/platform/vhci_hcd.0/status and in debug output.
+* The socket pointer address is not used at the moment and it was
+* made visible as a convenient way to find IP address from socket
+* pointer address by looking up /proc/net/{tcp,tcp6}. As this opens
+* a security hole, the change is made to use sockfd instead.
 */
out += sprintf(out,
-  "prt sta spd bus dev socket   local_busid\n");
+  "prt sta spd dev  sockfd local_busid\n");
 
for (i = 0; i < VHCI_NPORTS; i++) {
struct vhci_device *vdev = port_to_vdev(i);
@@ -59,12 +63,11 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
if (vdev->ud.status == VDEV_ST_USED) {
out += sprintf(out, "%03u %08x ",
   vdev->speed, vdev->devid);
-   out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
+   out += sprintf(out, "%06u ", vdev->ud.sockfd);
out += sprintf(out, "%s", dev_name(>udev->dev));
 
-   } else {
-   out += sprintf(out, "000 000 000  0-0");
-   }
+   } else
+   out += sprintf(out, "000  00 0-0");
 
out += sprintf(out, "\n");
spin_unlock(>ud.lock);
@@ -223,6 +226,7 @@ static ssize_t store_attach(struct device *dev, struct 
device_attribute *attr,
 
vdev->devid = devid;
vdev->speed = speed;
+   vdev->ud.sockfd = sockfd;
vdev->ud.tcp_socket = socket;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
 
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index ad9204773533..1274f326242c 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -55,12 +55,12 @@ static int parse_status(const char *value)
 
while (*c != '\0') {
int port, status, speed, devid;
-   unsigned long socket;
+   int sockfd;
char lbusid[SYSFS_BUS_ID_SIZE];
 
-   ret = sscanf(c, "%d %d %d %x %lx %31s\n",
+   ret = sscanf(c, "%d %d %d %x %u %31s\n",
, , ,
-   , , lbusid);
+   , , lbusid);
 
if

[PATCH 3.18 9/9] usbip: vhci: stop printing kernel pointer addresses in messages

[Shuah Khan]

Upstream commit 8272d099d05f ("usbip: vhci: stop printing kernel
pointer addresses in messages")

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/vhci_hcd.c | 10 --
 drivers/usb/usbip/vhci_rx.c  | 23 +++
 drivers/usb/usbip/vhci_tx.c  |  3 ++-
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index a57843e1173f..869938036248 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -469,9 +469,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb 
*urb,
struct vhci_device *vdev;
unsigned long flags;
 
-   usbip_dbg_vhci_hc("enter, usb_hcd %p urb %p mem_flags %d\n",
- hcd, urb, mem_flags);
-
/* patch to usb_sg_init() is in 2.5.60 */
BUG_ON(!urb->transfer_buffer && urb->transfer_buffer_length);
 
@@ -630,8 +627,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
struct vhci_device *vdev;
unsigned long flags;
 
-   pr_info("dequeue a urb %p\n", urb);
-
spin_lock_irqsave(_controller->lock, flags);
 
priv = urb->hcpriv;
@@ -659,7 +654,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
/* tcp connection is closed */
spin_lock(>priv_lock);
 
-   pr_info("device %p seems to be disconnected\n", vdev);
list_del(>list);
kfree(priv);
urb->hcpriv = NULL;
@@ -671,8 +665,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 * vhci_rx will receive RET_UNLINK and give back the URB.
 * Otherwise, we give back it here.
 */
-   pr_info("gives back urb %p\n", urb);
-
usb_hcd_unlink_urb_from_ep(hcd, urb);
 
spin_unlock_irqrestore(_controller->lock, flags);
@@ -701,8 +693,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 
unlink->unlink_seqnum = priv->seqnum;
 
-   pr_info("device %p seems to be still connected\n", vdev);
-
/* send cmd_unlink and try to cancel the pending URB in the
 * peer */
list_add_tail(>list, >unlink_tx);
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index d656e0edc3d5..323aa7789989 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -37,24 +37,23 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device 
*vdev, __u32 seqnum)
urb = priv->urb;
status = urb->status;
 
-   usbip_dbg_vhci_rx("find urb %p vurb %p seqnum %u\n",
-   urb, priv, seqnum);
+   usbip_dbg_vhci_rx("find urb seqnum %u\n", seqnum);
 
switch (status) {
case -ENOENT:
/* fall through */
case -ECONNRESET:
-   dev_info(>dev->dev,
-"urb %p was unlinked %ssynchronuously.\n", urb,
-status == -ENOENT ? "" : "a");
+   dev_dbg(>dev->dev,
+"urb seq# %u was unlinked %ssynchronuously\n",
+seqnum, status == -ENOENT ? "" : "a");
break;
case -EINPROGRESS:
/* no info output */
break;
default:
-   dev_info(>dev->dev,
-"urb %p may be in a error, status %d\n", urb,
-status);
+   dev_dbg(>dev->dev,
+"urb seq# %u may be in a error, status %d\n",
+seqnum, status);
}
 
list_del(>list);
@@ -79,8 +78,8 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
spin_unlock_irqrestore(>priv_lock, flags);
 
if (!urb) {
-   pr_err("cannot find a urb of seqnum %u\n", pdu->base.seqnum);
-   pr_info("max seqnum %d\n",
+   pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
+   pdu->base.seqnum,
atomic_read(_controller->seqnum));
usbip_event_add(ud, VDEV_EVENT_ERRO

[PATCH 4.4 v2 2/2] usbip: fix 3eee23c3ec14 tcp_socket address still in the status file

[Shuah Khan]

Commit 3eee23c3ec14 ("usbip: prevent vhci_hcd driver from leaking a
socket pointer address") backported the following commit from mailine.
However, backport error caused the tcp_socket address to still leak.

commit 2f2d0088eb93 ("usbip: prevent vhci_hcd driver from leaking a
socket pointer address")

When a client has a USB device attached over IP, the vhci_hcd driver is
locally leaking a socket pointer address via the

/sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
output when "usbip --debug port" is run.

Fix it to not leak. The socket pointer address is not used at the moment
and it was made visible as a convenient way to find IP address from
socket pointer address by looking up /proc/net/{tcp,tcp6}.

As this opens a security hole, the fix replaces socket pointer address
with sockfd.

Reported-by: Eric Biggers <ebigge...@gmail.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_sysfs.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 1c7f41a65565..b9432fdec775 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -53,7 +53,7 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
 * a security hole, the change is made to use sockfd instead.
 */
out += sprintf(out,
-  "prt sta spd bus dev sockfd local_busid\n");
+  "prt sta spd dev  sockfd local_busid\n");
 
for (i = 0; i < VHCI_NPORTS; i++) {
struct vhci_device *vdev = port_to_vdev(i);
@@ -64,12 +64,11 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
if (vdev->ud.status == VDEV_ST_USED) {
out += sprintf(out, "%03u %08x ",
   vdev->speed, vdev->devid);
-   out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
-   out += sprintf(out, "%06u", vdev->ud.sockfd);
+   out += sprintf(out, "%06u ", vdev->ud.sockfd);
out += sprintf(out, "%s", dev_name(>udev->dev));
 
} else
-   out += sprintf(out, "000 000 000 00 0-0");
+   out += sprintf(out, "000  00 0-0");
 
out += sprintf(out, "\n");
spin_unlock(>ud.lock);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 v2 1/2] usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit

[Shuah Khan]

Upstream commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
support")

vhci_hcd clears all the bits port_status bits instead of clearing
just the USB_PORT_STAT_POWER bit when it handles ClearPortFeature:
USB_PORT_FEAT_POWER. This causes vhci_hcd attach to fail in a bad
state, leaving device unusable by the client. The device is still
attached and however client can't use it.

The problem was fixed as part of larger change to add USB3 Super Speed
support.

This patch isolates the one line fix to clear the USB_PORT_STAT_POWER
from the original patch.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 00d68945548e..2d96bfd34138 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -285,7 +285,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(
" ClearPortFeature: USB_PORT_FEAT_POWER\n");
-   dum->port_status[rhport] = 0;
+   dum->port_status[rhport] &= ~USB_PORT_STAT_POWER;
dum->resuming = 0;
break;
case USB_PORT_FEAT_C_RESET:
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 v2 0/2] Backports for fixes

[Shuah Khan]

The first patch in this series isolates and backports a fix to clear
just the USB_PORT_STAT_POWER. Without this fix, client can't use the
imported device.

The second patch is fix to back-ported commit 3eee23c3ec14. tcp_socket
address still present in the status file. This is my bad. The bug fixed
in the first patch in this series masked this bug. With these two fixes,
client can use the imported devices on 4.4

Eric Biggers also reported the tcp_socket address still in the status
file while I am getting the patch ready. I added him to Reported-by.

Shuah Khan (2):
  usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit
  usbip: fix 3eee23c3ec14 tcp_socket address still in the status file

 drivers/usb/usbip/vhci_hcd.c   | 2 +-
 drivers/usb/usbip/vhci_sysfs.c | 7 +++
 2 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATH 0/4] usbip: make vhci_hcd.* objects independent of vhci_hcd.0

[Shuah Khan]

On 01/30/2018 01:36 AM, Salvador Fandino wrote:
> Let me start by explaining the problem that have motivated me to write
> this patches:
> 
> I work on the QVD, a virtual desktop platform for Linux. This software
> runs Linux desktops (i.e. XFCE, KDE) and their applications inside LXC
> containers, and makes then available through the network to remote
> users.
> 
> Supporting USB devices is a common feature customers have been
> requesting us for a long time (in order to use, for instance, remote
> signature pads, bar-code scanners, fingerprint readers, etc.). So, we
> have been working on that feature using the USB/IP layer on the
> kernel.
> 
> Connecting and disconnecting devices and transferring data works
> seamless for the devices listed above. But we also want to make the
> usbip operations private to the container where they are run.  For
> instance, it is unacceptable for our product, that one user could list
> the devices connected by other users or access them.
> 
> We can control how can access every device using cgroups once those
> are attached, but the usbip layer is not providing any mechanism for
> controlling who can attach, detach or list the devices.
> 
> So, we got the idea that in order to enforce that remote usbip devices
> are only visible inside the container where they were imported, we
> could conveniently mount-bind inside every container just one of the
> vhci_hcd directories below /sys/devices/platform. So that it is as if
> every container had a vhci_hcd just for itself (and also, we restrict
> access to the matching USB ports in cgroups).
> 
> Unfortunately, all the vhci_hcd.* devices are controlled through
> attributes in vhci_hcd.0 making our approach fail and so... well, that
> is what this patch series changes. It makes every vhci_hcd device
> controllable through attributes inside its own sysfs directory.
> 
> The first patch, does that in the kernel, and the second and third
> patches change user space, adapting the libusbip and the usbip tools
> code respectively.
> 
> Then there is a fourth patch, that allows to create much more USB
> hubs per machine. It was limited to 64 and we are running thousands of
> containers (every one requiring a hub) per host.
> 
> These changes are not completely backward compatible. In the sysfs
> side, besides moving around the attribute files, now the port numbers
> go from 0 to CONFIG_USBIP_VHCI_HC_PORTS * 2 - 1 and are reused for
> every vhci_hcd device. I could have maintained the absolute numeration
> but I think reusing the numbers is a better and simpler approach.
> 
> I have considered that until very recently, support for vhci_hcd
> devices above the .0 was broken in the uspip tools (see
> 1ac7c8a78be85f84b019d3d2742d1a9f07255cc5 "usbip: fix usbip attach to
> find a port that matches the requested speed") and that has been the
> place where I have set the bar for backward compatibility: usage of
> the tools must remain unchanged for accessing "vhci_hcd.0", but may
> require changes otherwise. The same is true for the library functions
> which now provides new functions for selecting the target vhci_hcd
> device. The old functions now always target "vhci_hcd.0".
> 
> So, for instance, now, "usbip port" by default only shows "vhci_hcd.0"
> ports but "usbip port -a" shows all of them, and, for instance, "usbip
> port -i 4", shows the ports in "vhci_hcd.4".
> 
> Cheers.
> 

I will take a look at these in the next week or so.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4.9] usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit

[Shuah Khan]

On 01/28/2018 05:14 AM, Greg KH wrote:
> On Fri, Jan 26, 2018 at 11:54:35AM -0700, Shuah Khan wrote:
>> Upstream commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
>> support")
> 
> Hm, I think you have the wrong commit id here.
> 
> I don't see any commit upstream with the Subject you have here, what are
> you referring to?
> 
> thanks,
> 
> greg k-h
> 

That is odd. Th commit went in a while back. Here are the details:

This is the commit from upstream:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/usbip/vhci_hcd.c?id=1c9de5bf428612458427943b724bea51abde520a

commit 1c9de5bf428612458427943b724bea51abde520a
Author: Yuyang Du <yuyang...@intel.com>
Date:   Thu Jun 8 13:04:10 2017 +0800

usbip: vhci-hcd: Add USB3 SuperSpeed support

This patch adds a USB3 HCD to an existing USB2 HCD and provides
the support of SuperSpeed, in case the device can only be enumerated
with SuperSpeed.

The bulk of the added code in usb3_bos_desc and hub_control to support
SuperSpeed is borrowed from the commit 1cd8fd2887e162ad ("usb: gadget:
dummy_hcd: add SuperSpeed support").

With this patch, each vhci will have VHCI_HC_PORTS HighSpeed ports
and VHCI_HC_PORTS SuperSpeed ports.

Suggested-by: Krzysztof Opasiak <k.opas...@samsung.com>
Signed-off-by: Yuyang Du <yuyang...@intel.com>
Acked-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>


I isolated and backported a one line fix to the problem I saw in 4.9
and 4.4 stables.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: keep usbip_device sockfd state in sync with tcp_socket

[Shuah Khan]

Keep usbip_device sockfd state in sync with tcp_socket. When tcp_socket
is reset to null, reset sockfd to -1 to keep it in sync.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: sta...@vger.kernel.org
---
 drivers/usb/usbip/stub_dev.c | 3 +++
 drivers/usb/usbip/vhci_hcd.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index e31a6f204397..86037e5b1101 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -73,6 +73,7 @@ static ssize_t store_sockfd(struct device *dev, struct 
device_attribute *attr,
goto err;
 
sdev->ud.tcp_socket = socket;
+   sdev->ud.sockfd = sockfd;
 
spin_unlock_irq(>ud.lock);
 
@@ -172,6 +173,7 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
if (ud->tcp_socket) {
sockfd_put(ud->tcp_socket);
ud->tcp_socket = NULL;
+   ud->sockfd = -1;
}
 
/* 3. free used data */
@@ -266,6 +268,7 @@ static struct stub_device *stub_device_alloc(struct 
usb_device *udev)
sdev->ud.status = SDEV_ST_AVAILABLE;
spin_lock_init(>ud.lock);
sdev->ud.tcp_socket = NULL;
+   sdev->ud.sockfd = -1;
 
INIT_LIST_HEAD(>priv_init);
INIT_LIST_HEAD(>priv_tx);
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index c3e1008aa491..20e3d4609583 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -984,6 +984,7 @@ static void vhci_shutdown_connection(struct usbip_device 
*ud)
if (vdev->ud.tcp_socket) {
sockfd_put(vdev->ud.tcp_socket);
vdev->ud.tcp_socket = NULL;
+   vdev->ud.sockfd = -1;
}
pr_info("release socket\n");
 
@@ -1030,6 +1031,7 @@ static void vhci_device_reset(struct usbip_device *ud)
if (ud->tcp_socket) {
sockfd_put(ud->tcp_socket);
ud->tcp_socket = NULL;
+   ud->sockfd = -1;
}
ud->status = VDEV_ST_NULL;
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4] usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit

[Shuah Khan]

Upstream commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
support")

vhci_hcd clears all the bits port_status bits instead of clearing
just the USB_PORT_STAT_POWER bit when it handles ClearPortFeature:
USB_PORT_FEAT_POWER. This causes vhci_hcd attach to fail in a bad
state, leaving device unusable by the client. The device is still
attached and however client can't use it.

The problem was fixed as part of larger change to add  USB3 Super
Speed support. This patch backports just the change to clear the
USB_PORT_STAT_POWER.

In addition, a minor formatting error in status file is fixed.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c   | 2 +-
 drivers/usb/usbip/vhci_sysfs.c | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 00d68945548e..2d96bfd34138 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -285,7 +285,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(
" ClearPortFeature: USB_PORT_FEAT_POWER\n");
-   dum->port_status[rhport] = 0;
+   dum->port_status[rhport] &= ~USB_PORT_STAT_POWER;
dum->resuming = 0;
break;
case USB_PORT_FEAT_C_RESET:
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 1c7f41a65565..ebf133c9eea3 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -53,7 +53,7 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
 * a security hole, the change is made to use sockfd instead.
 */
out += sprintf(out,
-  "prt sta spd bus dev sockfd local_busid\n");
+  "prt sta spd dev  sockfd local_busid\n");
 
for (i = 0; i < VHCI_NPORTS; i++) {
struct vhci_device *vdev = port_to_vdev(i);
@@ -64,8 +64,7 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
if (vdev->ud.status == VDEV_ST_USED) {
out += sprintf(out, "%03u %08x ",
   vdev->speed, vdev->devid);
-   out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
-   out += sprintf(out, "%06u", vdev->ud.sockfd);
+   out += sprintf(out, "%06u ", vdev->ud.sockfd);
out += sprintf(out, "%s", dev_name(>udev->dev));
 
} else
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.9] usbip: vhci_hcd: clear just the USB_PORT_STAT_POWER bit

[Shuah Khan]

Upstream commit 1c9de5bf4286 ("usbip: vhci-hcd: Add USB3 SuperSpeed
support")

vhci_hcd clears all the bits port_status bits instead of clearing
just the USB_PORT_STAT_POWER bit when it handles ClearPortFeature:
USB_PORT_FEAT_POWER. This causes vhci_hcd attach to fail in a bad
state, leaving device unusable by the client. The device is still
attached and however client can't use it.

The problem was fixed as part of larger change to add  USB3 Super
Speed support. This patch backports just the change to clear the
USB_PORT_STAT_POWER.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 7f161b095176..dbe615ba07c9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -300,7 +300,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
case USB_PORT_FEAT_POWER:
usbip_dbg_vhci_rh(
" ClearPortFeature: USB_PORT_FEAT_POWER\n");
-   dum->port_status[rhport] = 0;
+   dum->port_status[rhport] &= ~USB_PORT_STAT_POWER;
dum->resuming = 0;
break;
case USB_PORT_FEAT_C_RESET:
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 0/4] Backport missing sccurity and deadlock fix

[Shuah Khan]

As I started backporting security fixes, I found a deadlock bug that was
fixed in a later release. This patch series contains backports for all
these problems.

Andrew Goodbody (1):
  usb: usbip: Fix possible deadlocks reported by lockdep

Shuah Khan (3):
  usbip: fix stub_rx: get_pipe() to validate endpoint number
  usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input
  usbip: prevent leaking socket pointer address in messages

 drivers/usb/usbip/stub_dev.c |  3 +-
 drivers/usb/usbip/stub_rx.c  | 46 
 drivers/usb/usbip/usbip_common.c | 15 ++-
 drivers/usb/usbip/usbip_event.c  |  5 ++-
 drivers/usb/usbip/vhci_hcd.c | 90 +++-
 drivers/usb/usbip/vhci_rx.c  | 30 --
 drivers/usb/usbip/vhci_sysfs.c   | 19 +
 drivers/usb/usbip/vhci_tx.c  | 14 ---
 8 files changed, 134 insertions(+), 88 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 1/4] usb: usbip: Fix possible deadlocks reported by lockdep

[Shuah Khan]

From: Andrew Goodbody <andrew.goodb...@cambrionix.com>

Upstream commit 21619792d1ec ("usb: usbip: Fix possible deadlocks
reported by lockdep")

Change spin_lock calls to spin_lock_irqsave to prevent
attmpted recursive lock taking in interrupt context.

This patch fixes Bug 109351
  https://bugzilla.kernel.org/show_bug.cgi?id=109351

Signed-off-by: Andrew Goodbody <andrew.goodb...@cambrionix.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_event.c |  5 ++-
 drivers/usb/usbip/vhci_hcd.c| 88 -
 drivers/usb/usbip/vhci_rx.c | 30 --
 drivers/usb/usbip/vhci_sysfs.c  | 19 +
 drivers/usb/usbip/vhci_tx.c | 14 ---
 5 files changed, 91 insertions(+), 65 deletions(-)

diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 64933b993d7a..2580a32bcdff 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -117,11 +117,12 @@ EXPORT_SYMBOL_GPL(usbip_event_add);
 int usbip_event_happened(struct usbip_device *ud)
 {
int happened = 0;
+   unsigned long flags;
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
if (ud->event != 0)
happened = 1;
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
 
return happened;
 }
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index f9af04d7f02f..0aaa8e524afd 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -121,9 +121,11 @@ static void dump_port_status_diff(u32 prev_status, u32 
new_status)
 
 void rh_port_connect(int rhport, enum usb_device_speed speed)
 {
+   unsigned long   flags;
+
usbip_dbg_vhci_rh("rh_port_connect %d\n", rhport);
 
-   spin_lock(_controller->lock);
+   spin_lock_irqsave(_controller->lock, flags);
 
the_controller->port_status[rhport] |= USB_PORT_STAT_CONNECTION
| (1 << USB_PORT_FEAT_C_CONNECTION);
@@ -139,22 +141,24 @@ void rh_port_connect(int rhport, enum usb_device_speed 
speed)
break;
}
 
-   spin_unlock(_controller->lock);
+   spin_unlock_irqrestore(_controller->lock, flags);
 
usb_hcd_poll_rh_status(vhci_to_hcd(the_controller));
 }
 
 static void rh_port_disconnect(int rhport)
 {
+   unsigned long   flags;
+
usbip_dbg_vhci_rh("rh_port_disconnect %d\n", rhport);
 
-   spin_lock(_controller->lock);
+   spin_lock_irqsave(_controller->lock, flags);
 
the_controller->port_status[rhport] &= ~USB_PORT_STAT_CONNECTION;
the_controller->port_status[rhport] |=
(1 << USB_PORT_FEAT_C_CONNECTION);
 
-   spin_unlock(_controller->lock);
+   spin_unlock_irqrestore(_controller->lock, flags);
usb_hcd_poll_rh_status(vhci_to_hcd(the_controller));
 }
 
@@ -182,13 +186,14 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
int retval;
int rhport;
int changed = 0;
+   unsigned long   flags;
 
retval = DIV_ROUND_UP(VHCI_NPORTS + 1, 8);
memset(buf, 0, retval);
 
vhci = hcd_to_vhci(hcd);
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
if (!HCD_HW_ACCESSIBLE(hcd)) {
usbip_dbg_vhci_rh("hw accessible flag not on?\n");
goto done;
@@ -209,7 +214,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
usb_hcd_resume_root_hub(hcd);
 
 done:
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
return changed ? retval : 0;
 }
 
@@ -236,6 +241,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
struct vhci_hcd *dum;
int retval = 0;
int rhport;
+   unsigned long   flags;
 
u32 prev_port_status[VHCI_NPORTS];
 
@@ -254,7 +260,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
 
dum = hcd_to_vhci(hcd);
 
-   spin_lock(>lock);
+   spin_lock_irqsave(>lock, flags);
 
/* store old status and compare now and old later */
if (usbip_dbg_flag_vhci_rh) {
@@ -408,7 +414,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 
typeReq, u16 wValue,
}
usbip_dbg_vhci_rh(" bye\n");
 
-   spin_unlock(>lock);
+   spin_unlock_irqrestore(>lock, flags);
 
return retval;
 }
@@ -431,6 +437,7 @@ static void vhci_tx_urb(struct urb *urb)
 {
struct vhci_device *vdev = get_vdev(urb->dev);
struct vhci_priv *priv;
+   unsigned long flags;
 
if (!vdev) {
pr_err("could not get virtual devi

[PATCH 4.4 2/4] usbip: fix stub_rx: get_pipe() to validate endpoint number

[Shuah Khan]

Upstream commit 635f545a7e8b ("usbip: fix stub_rx: get_pipe() to validate
endpoint number")

get_pipe() routine doesn't validate the input endpoint number
and uses to reference ep_in and ep_out arrays. Invalid endpoint
number can trigger BUG(). Range check the epnum and returning
error instead of calling BUG().

Change caller stub_recv_cmd_submit() to handle the get_pipe()
error return.

Reported-by: Secunia Research <v...@secunia.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_rx.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 7de54a66044f..e617c90661b4 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -344,15 +344,15 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
 
+   if (epnum < 0 || epnum > 15)
+   goto err_ret;
+
if (dir == USBIP_DIR_IN)
ep = udev->ep_in[epnum & 0x7f];
else
ep = udev->ep_out[epnum & 0x7f];
-   if (!ep) {
-   dev_err(>interface->dev, "no such endpoint?, %d\n",
-   epnum);
-   BUG();
-   }
+   if (!ep)
+   goto err_ret;
 
epd = >desc;
if (usb_endpoint_xfer_control(epd)) {
@@ -383,9 +383,10 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
return usb_rcvisocpipe(udev, epnum);
}
 
+err_ret:
/* NOT REACHED */
-   dev_err(>interface->dev, "get pipe, epnum %d\n", epnum);
-   return 0;
+   dev_err(>udev->dev, "get pipe() invalid epnum %d\n", epnum);
+   return -1;
 }
 
 static void masking_bogus_flags(struct urb *urb)
@@ -451,6 +452,9 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct usb_device *udev = sdev->udev;
int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
 
+   if (pipe == -1)
+   return;
+
priv = stub_priv_alloc(sdev, pdu);
if (!priv)
return;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 4/4] usbip: prevent leaking socket pointer address in messages

[Shuah Khan]

Upstream commit 90120d15f4c3 ("usbip: prevent leaking socket pointer
address in messages")

usbip driver is leaking socket pointer address in messages. Remove
the messages that aren't useful and print sockfd in the ones that
are useful for debugging.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_dev.c |  3 +--
 drivers/usb/usbip/usbip_common.c | 15 ---
 drivers/usb/usbip/vhci_hcd.c |  2 +-
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index a3ec49bdc1e6..ec38370ffcab 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -163,8 +163,7 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
 * step 1?
 */
if (ud->tcp_socket) {
-   dev_dbg(>udev->dev, "shutdown tcp_socket %p\n",
-   ud->tcp_socket);
+   dev_dbg(>udev->dev, "shutdown sockfd %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 9752b93f754e..1838f1b2c2fa 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -317,18 +317,14 @@ int usbip_recv(struct socket *sock, void *buf, int size)
struct msghdr msg;
struct kvec iov;
int total = 0;
-
/* for blocks of if (usbip_dbg_flag_xmit) */
char *bp = buf;
int osize = size;
 
-   usbip_dbg_xmit("enter\n");
-
-   if (!sock || !buf || !size) {
-   pr_err("invalid arg, sock %p buff %p size %d\n", sock, buf,
-  size);
+   if (!sock || !buf || !size)
return -EINVAL;
-   }
+
+   usbip_dbg_xmit("enter\n");
 
do {
sock->sk->sk_allocation = GFP_NOIO;
@@ -341,11 +337,8 @@ int usbip_recv(struct socket *sock, void *buf, int size)
msg.msg_flags  = MSG_NOSIGNAL;
 
result = kernel_recvmsg(sock, , , 1, size, MSG_WAITALL);
-   if (result <= 0) {
-   pr_debug("receive sock %p buf %p size %u ret %d total 
%d\n",
-sock, buf, size, result, total);
+   if (result <= 0)
goto err;
-   }
 
size -= result;
buf += result;
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 0aaa8e524afd..00d68945548e 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -778,7 +778,7 @@ static void vhci_shutdown_connection(struct usbip_device 
*ud)
 
/* need this? see stub_dev.c */
if (ud->tcp_socket) {
-   pr_debug("shutdown tcp_socket %p\n", ud->tcp_socket);
+   pr_debug("shutdown sockfd %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4 3/4] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

Upstream commit c6688ef9f297 ("usbip: fix stub_rx: harden CMD_SUBMIT path
to handle malicious input")

Harden CMD_SUBMIT path to handle malicious input that could trigger
large memory allocations. Add checks to validate transfer_buffer_length
and number_of_packets to protect against bad input requesting for
unbounded memory allocations. Validate early in get_pipe() and return
failure.

Reported-by: Secunia Research <v...@secunia.com>
Cc: stable <sta...@vger.kernel.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
---
 drivers/usb/usbip/stub_rx.c | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index e617c90661b4..56cacb68040c 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -338,11 +338,13 @@ static struct stub_priv *stub_priv_alloc(struct 
stub_device *sdev,
return priv;
 }
 
-static int get_pipe(struct stub_device *sdev, int epnum, int dir)
+static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 {
struct usb_device *udev = sdev->udev;
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
+   int epnum = pdu->base.ep;
+   int dir = pdu->base.direction;
 
if (epnum < 0 || epnum > 15)
goto err_ret;
@@ -355,6 +357,7 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
goto err_ret;
 
epd = >desc;
+
if (usb_endpoint_xfer_control(epd)) {
if (dir == USBIP_DIR_OUT)
return usb_sndctrlpipe(udev, epnum);
@@ -377,6 +380,27 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
}
 
if (usb_endpoint_xfer_isoc(epd)) {
+   /* validate packet size and number of packets */
+   unsigned int maxp, packets, bytes;
+
+#define USB_EP_MAXP_MULT_SHIFT  11
+#define USB_EP_MAXP_MULT_MASK   (3 << USB_EP_MAXP_MULT_SHIFT)
+#define USB_EP_MAXP_MULT(m) \
+   (((m) & USB_EP_MAXP_MULT_MASK) >> USB_EP_MAXP_MULT_SHIFT)
+
+   maxp = usb_endpoint_maxp(epd);
+   maxp *= (USB_EP_MAXP_MULT(
+   __le16_to_cpu(epd->wMaxPacketSize)) + 1);
+   bytes = pdu->u.cmd_submit.transfer_buffer_length;
+   packets = DIV_ROUND_UP(bytes, maxp);
+
+   if (pdu->u.cmd_submit.number_of_packets < 0 ||
+   pdu->u.cmd_submit.number_of_packets > packets) {
+   dev_err(>udev->dev,
+   "CMD_SUBMIT: isoc invalid num packets %d\n",
+   pdu->u.cmd_submit.number_of_packets);
+   return -1;
+   }
if (dir == USBIP_DIR_OUT)
return usb_sndisocpipe(udev, epnum);
else
@@ -385,7 +409,7 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
 
 err_ret:
/* NOT REACHED */
-   dev_err(>udev->dev, "get pipe() invalid epnum %d\n", epnum);
+   dev_err(>udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
return -1;
 }
 
@@ -450,7 +474,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct stub_priv *priv;
struct usbip_device *ud = >ud;
struct usb_device *udev = sdev->udev;
-   int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
+   int pipe = get_pipe(sdev, pdu);
 
if (pipe == -1)
return;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4] usbip: Fix implicit fallthrough warning

[Shuah Khan]

From: Jonathan Dieter <jdie...@lesbg.com>

Upstream commit cfd6ed4537a9 ("usbip: Fix implicit fallthrough warning")

GCC 7 now warns when switch statements fall through implicitly, and with
-Werror enabled in configure.ac, that makes these tools unbuildable.

We fix this by notifying the compiler that this particular case statement
is meant to fall through.

Reviewed-by: Peter Senna Tschudin <peter.se...@gmail.com>
Signed-off-by: Jonathan Dieter <jdie...@lesbg.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index d7599d943529..73d8eee8130b 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -176,6 +176,8 @@ int main(int argc, char *argv[])
break;
case '?':
printf("usbip: invalid option\n");
+   /* Terminate after printing error */
+   /* FALLTHRU */
default:
usbip_usage();
goto out;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4] usbip: Fix potential format overflow in userspace tools

[Shuah Khan]

Upstream commit e5dfa3f902b9 ("usbip: Fix potential format overflow in
userspace tools")

The usbip userspace tools call sprintf()/snprintf() and don't check for
the return value which can lead the paths to overflow, truncating the
final file in the path.

More urgently, GCC 7 now warns that these aren't checked with
-Wformat-overflow, and with -Werror enabled in configure.ac, that makes
these tools unbuildable.

This patch fixes these problems by replacing sprintf() with snprintf()
in one place and adding checks for the return value of snprintf().

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/libsrc/usbip_common.c  |  9 -
 tools/usb/usbip/libsrc/usbip_host_driver.c | 27 ++-
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710473de..8000445ff884 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -215,9 +215,16 @@ int read_usb_interface(struct usbip_usb_device *udev, int 
i,
   struct usbip_usb_interface *uinf)
 {
char busid[SYSFS_BUS_ID_SIZE];
+   int size;
struct udev_device *sif;
 
-   sprintf(busid, "%s:%d.%d", udev->busid, udev->bConfigurationValue, i);
+   size = snprintf(busid, sizeof(busid), "%s:%d.%d",
+   udev->busid, udev->bConfigurationValue, i);
+   if (size < 0 || (unsigned int)size >= sizeof(busid)) {
+   err("busid length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(busid));
+   return -1;
+   }
 
sif = udev_device_new_from_subsystem_sysname(udev_context, "usb", 
busid);
if (!sif) {
diff --git a/tools/usb/usbip/libsrc/usbip_host_driver.c 
b/tools/usb/usbip/libsrc/usbip_host_driver.c
index bef08d5c44e8..14c2916b4fec 100644
--- a/tools/usb/usbip/libsrc/usbip_host_driver.c
+++ b/tools/usb/usbip/libsrc/usbip_host_driver.c
@@ -39,13 +39,19 @@ struct udev *udev_context;
 static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
 {
char status_attr_path[SYSFS_PATH_MAX];
+   int size;
int fd;
int length;
char status;
int value = 0;
 
-   snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
-udev->path);
+   size = snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
+   udev->path);
+   if (size < 0 || (unsigned int)size >= sizeof(status_attr_path)) {
+   err("usbip_status path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(status_attr_path));
+   return -1;
+   }
 
fd = open(status_attr_path, O_RDONLY);
if (fd < 0) {
@@ -225,6 +231,7 @@ int usbip_host_export_device(struct usbip_exported_device 
*edev, int sockfd)
 {
char attr_name[] = "usbip_sockfd";
char sockfd_attr_path[SYSFS_PATH_MAX];
+   int size;
char sockfd_buff[30];
int ret;
 
@@ -244,10 +251,20 @@ int usbip_host_export_device(struct usbip_exported_device 
*edev, int sockfd)
}
 
/* only the first interface is true */
-   snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
-edev->udev.path, attr_name);
+   size = snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
+   edev->udev.path, attr_name);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_attr_path)) {
+   err("exported device path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_attr_path));
+   return -1;
+   }
 
-   snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_buff)) {
+   err("socket length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_buff));
+   return -1;
+   }
 
ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
strlen(sockfd_buff));
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4] usbip: prevent vhci_hcd driver from leaking a socket pointer address

[Shuah Khan]

commit 2f2d0088eb93 ("usbip: prevent vhci_hcd driver from leaking a
socket pointer address")

When a client has a USB device attached over IP, the vhci_hcd driver is
locally leaking a socket pointer address via the

/sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
output when "usbip --debug port" is run.

Fix it to not leak. The socket pointer address is not used at the moment
and it was made visible as a convenient way to find IP address from
socket pointer address by looking up /proc/net/{tcp,tcp6}.

As this opens a security hole, the fix replaces socket pointer address
with sockfd.

Reported-by: Secunia Research <v...@secunia.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.h |  1 +
 drivers/usb/usbip/vhci_sysfs.c   | 25 +++--
 tools/usb/usbip/libsrc/vhci_driver.c |  8 
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 86b08475c254..f875ccaa55f9 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -261,6 +261,7 @@ struct usbip_device {
/* lock for status */
spinlock_t lock;
 
+   int sockfd;
struct socket *tcp_socket;
 
struct task_struct *tcp_rx;
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 211f43f67ea2..84c21c4ccf46 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -39,16 +39,20 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
 
/*
 * output example:
-* prt sta spd dev socket   local_busid
-* 000 004 000 000 c5a7bb80 1-2.3
-* 001 004 000 000 d8cee980 2-3.4
+* port sta spd dev  sockfd local_busid
+*  004 000  03 1-2.3
+* 0001 004 000  04 2-3.4
 *
-* IP address can be retrieved from a socket pointer address by looking
-* up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
-* port number and its peer IP address.
+* Output includes socket fd instead of socket pointer address to
+* avoid leaking kernel memory address in:
+*  /sys/devices/platform/vhci_hcd.0/status and in debug output.
+* The socket pointer address is not used at the moment and it was
+* made visible as a convenient way to find IP address from socket
+* pointer address by looking up /proc/net/{tcp,tcp6}. As this opens
+* a security hole, the change is made to use sockfd instead.
 */
out += sprintf(out,
-  "prt sta spd bus dev socket   local_busid\n");
+  "prt sta spd bus dev sockfd local_busid\n");
 
for (i = 0; i < VHCI_NPORTS; i++) {
struct vhci_device *vdev = port_to_vdev(i);
@@ -60,11 +64,11 @@ static ssize_t status_show(struct device *dev, struct 
device_attribute *attr,
out += sprintf(out, "%03u %08x ",
   vdev->speed, vdev->devid);
out += sprintf(out, "%16p ", vdev->ud.tcp_socket);
+   out += sprintf(out, "%06u", vdev->ud.sockfd);
out += sprintf(out, "%s", dev_name(>udev->dev));
 
-   } else {
-   out += sprintf(out, "000 000 000  0-0");
-   }
+   } else
+   out += sprintf(out, "000 000 000 00 0-0");
 
out += sprintf(out, "\n");
spin_unlock(>ud.lock);
@@ -223,6 +227,7 @@ static ssize_t store_attach(struct device *dev, struct 
device_attribute *attr,
 
vdev->devid = devid;
vdev->speed = speed;
+   vdev->ud.sockfd = sockfd;
vdev->ud.tcp_socket = socket;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
 
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index ad9204773533..1274f326242c 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -55,12 +55,12 @@ static int parse_status(const char *value)
 
while (*c != '\0') {
int port, status, speed, devid;
-   unsigned long socket;
+   int sockfd;
char lbusid[SYSFS_BUS_ID_SIZE];
 
-   ret = sscanf(c, "%d %d %d %x %lx %31s\n",
+   ret = sscanf(c, "%d %d %d %x %u %31s\n",
, , ,
-   , , lbusid);
+   , , lbusid);
 
if (ret < 5) {
dbg("sscanf fai

[PATCH 4.9] usbip: Fix potential format overflow in userspace tools

[Shuah Khan]

From: Jonathan Dieter <jdie...@lesbg.com>

Upstream commit e5dfa3f902b9 ("usbip: Fix potential format overflow in
userspace tools")

The usbip userspace tools call sprintf()/snprintf() and don't check for
the return value which can lead the paths to overflow, truncating the
final file in the path.

More urgently, GCC 7 now warns that these aren't checked with
-Wformat-overflow, and with -Werror enabled in configure.ac, that makes
these tools unbuildable.

This patch fixes these problems by replacing sprintf() with snprintf() in
one place and adding checks for the return value of snprintf().

Reviewed-by: Peter Senna Tschudin <peter.se...@gmail.com>
Signed-off-by: Jonathan Dieter <jdie...@lesbg.com>
Acked-by: Shuah Khan <shua...@osg.samsung.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/libsrc/usbip_common.c  |  9 -
 tools/usb/usbip/libsrc/usbip_host_common.c | 28 +++-
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/tools/usb/usbip/libsrc/usbip_common.c 
b/tools/usb/usbip/libsrc/usbip_common.c
index ac73710473de..8000445ff884 100644
--- a/tools/usb/usbip/libsrc/usbip_common.c
+++ b/tools/usb/usbip/libsrc/usbip_common.c
@@ -215,9 +215,16 @@ int read_usb_interface(struct usbip_usb_device *udev, int 
i,
   struct usbip_usb_interface *uinf)
 {
char busid[SYSFS_BUS_ID_SIZE];
+   int size;
struct udev_device *sif;
 
-   sprintf(busid, "%s:%d.%d", udev->busid, udev->bConfigurationValue, i);
+   size = snprintf(busid, sizeof(busid), "%s:%d.%d",
+   udev->busid, udev->bConfigurationValue, i);
+   if (size < 0 || (unsigned int)size >= sizeof(busid)) {
+   err("busid length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(busid));
+   return -1;
+   }
 
sif = udev_device_new_from_subsystem_sysname(udev_context, "usb", 
busid);
if (!sif) {
diff --git a/tools/usb/usbip/libsrc/usbip_host_common.c 
b/tools/usb/usbip/libsrc/usbip_host_common.c
index 9d415228883d..c10379439668 100644
--- a/tools/usb/usbip/libsrc/usbip_host_common.c
+++ b/tools/usb/usbip/libsrc/usbip_host_common.c
@@ -40,13 +40,20 @@ struct udev *udev_context;
 static int32_t read_attr_usbip_status(struct usbip_usb_device *udev)
 {
char status_attr_path[SYSFS_PATH_MAX];
+   int size;
int fd;
int length;
char status;
int value = 0;
 
-   snprintf(status_attr_path, SYSFS_PATH_MAX, "%s/usbip_status",
-udev->path);
+   size = snprintf(status_attr_path, sizeof(status_attr_path),
+   "%s/usbip_status", udev->path);
+   if (size < 0 || (unsigned int)size >= sizeof(status_attr_path)) {
+   err("usbip_status path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(status_attr_path));
+   return -1;
+   }
+
 
fd = open(status_attr_path, O_RDONLY);
if (fd < 0) {
@@ -218,6 +225,7 @@ int usbip_export_device(struct usbip_exported_device *edev, 
int sockfd)
 {
char attr_name[] = "usbip_sockfd";
char sockfd_attr_path[SYSFS_PATH_MAX];
+   int size;
char sockfd_buff[30];
int ret;
 
@@ -237,10 +245,20 @@ int usbip_export_device(struct usbip_exported_device 
*edev, int sockfd)
}
 
/* only the first interface is true */
-   snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
-edev->udev.path, attr_name);
+   size = snprintf(sockfd_attr_path, sizeof(sockfd_attr_path), "%s/%s",
+   edev->udev.path, attr_name);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_attr_path)) {
+   err("exported device path length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_attr_path));
+   return -1;
+   }
 
-   snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   size = snprintf(sockfd_buff, sizeof(sockfd_buff), "%d\n", sockfd);
+   if (size < 0 || (unsigned int)size >= sizeof(sockfd_buff)) {
+   err("socket length %i >= %lu or < 0", size,
+   (unsigned long)sizeof(sockfd_buff));
+   return -1;
+   }
 
ret = write_sysfs_attribute(sockfd_attr_path, sockfd_buff,
strlen(sockfd_buff));
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.9] usbip: Fix implicit fallthrough warning

[Shuah Khan]

From: Jonathan Dieter <jdie...@lesbg.com>

Upstream commit cfd6ed4537a9 ("usbip: Fix implicit fallthrough warning")

GCC 7 now warns when switch statements fall through implicitly, and with
-Werror enabled in configure.ac, that makes these tools unbuildable.

We fix this by notifying the compiler that this particular case statement
is meant to fall through.

Reviewed-by: Peter Senna Tschudin <peter.se...@gmail.com>
Signed-off-by: Jonathan Dieter <jdie...@lesbg.com>
Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/usb/usbip/src/usbip.c b/tools/usb/usbip/src/usbip.c
index d7599d943529..73d8eee8130b 100644
--- a/tools/usb/usbip/src/usbip.c
+++ b/tools/usb/usbip/src/usbip.c
@@ -176,6 +176,8 @@ int main(int argc, char *argv[])
break;
case '?':
printf("usbip: invalid option\n");
+   /* Terminate after printing error */
+   /* FALLTHRU */
default:
usbip_usage();
goto out;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.9] usbip: prevent vhci_hcd driver from leaking a socket pointer address

[Shuah Khan]

commit 2f2d0088eb93 ("usbip: prevent vhci_hcd driver from leaking a
socket pointer address")

When a client has a USB device attached over IP, the vhci_hcd driver is
locally leaking a socket pointer address via the

/sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
output when "usbip --debug port" is run.

Fix it to not leak. The socket pointer address is not used at the moment
and it was made visible as a convenient way to find IP address from
socket pointer address by looking up /proc/net/{tcp,tcp6}.

As this opens a security hole, the fix replaces socket pointer address
with sockfd.

Reported-by: Secunia Research <v...@secunia.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.h |  1 +
 drivers/usb/usbip/vhci_sysfs.c   | 25 +++--
 tools/usb/usbip/libsrc/vhci_driver.c |  8 
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index 9f490375ac92..f0b955f8504e 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -271,6 +271,7 @@ struct usbip_device {
/* lock for status */
spinlock_t lock;
 
+   int sockfd;
struct socket *tcp_socket;
 
struct task_struct *tcp_rx;
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index b96e5b189269..c287ccc78fde 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -49,13 +49,17 @@ static ssize_t status_show_vhci(int pdev_nr, char *out)
 
/*
 * output example:
-* port sta spd dev  socket   local_busid
-*  004 000  c5a7bb80 1-2.3
-* 0001 004 000  d8cee980 2-3.4
+* port sta spd dev  sockfd local_busid
+*  004 000  03 1-2.3
+* 0001 004 000  04 2-3.4
 *
-* IP address can be retrieved from a socket pointer address by looking
-* up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
-* port number and its peer IP address.
+* Output includes socket fd instead of socket pointer address to
+* avoid leaking kernel memory address in:
+*  /sys/devices/platform/vhci_hcd.0/status and in debug output.
+* The socket pointer address is not used at the moment and it was
+* made visible as a convenient way to find IP address from socket
+* pointer address by looking up /proc/net/{tcp,tcp6}. As this opens
+* a security hole, the change is made to use sockfd instead.
 */
for (i = 0; i < VHCI_HC_PORTS; i++) {
struct vhci_device *vdev = >vdev[i];
@@ -68,13 +72,13 @@ static ssize_t status_show_vhci(int pdev_nr, char *out)
if (vdev->ud.status == VDEV_ST_USED) {
out += sprintf(out, "%03u %08x ",
vdev->speed, vdev->devid);
-   out += sprintf(out, "%16p %s",
-   vdev->ud.tcp_socket,
+   out += sprintf(out, "%06u %s",
+   vdev->ud.sockfd,
dev_name(>udev->dev));
 
} else {
out += sprintf(out, "000  ");
-   out += sprintf(out, " 0-0");
+   out += sprintf(out, "00 0-0");
}
 
out += sprintf(out, "\n");
@@ -125,7 +129,7 @@ static ssize_t status_show(struct device *dev,
int pdev_nr;
 
out += sprintf(out,
-  "port sta spd dev  socket   local_busid\n");
+  "port sta spd dev  sockfd local_busid\n");
 
pdev_nr = status_name_to_id(attr->attr.name);
if (pdev_nr < 0)
@@ -324,6 +328,7 @@ static ssize_t store_attach(struct device *dev, struct 
device_attribute *attr,
 
vdev->devid = devid;
vdev->speed = speed;
+   vdev->ud.sockfd = sockfd;
vdev->ud.tcp_socket = socket;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
 
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index ad9204773533..1274f326242c 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -55,12 +55,12 @@ static int parse_status(const char *value)
 
while (*c != '\0') {
int port, status, speed, devid;
-   unsigned long socket;
+   int sockfd;
char lbusid[SYSFS_BUS_ID_SIZE];
 
-   ret = sscanf(c, "%d %d %d %x %lx %31s\n",
+ 

Re: [PATCH 3/6] USB: move many drivers to use DEVICE_ATTR_WO

[Shuah Khan]

On 01/23/2018 03:24 AM, Greg Kroah-Hartman wrote:
> Instead of "open coding" a DEVICE_ATTR() define, use the
> DEVICE_ATTR_WO() macro instead, which does everything properly instead.
> 
> This does require a few static functions to be renamed to work properly,
> but thanks to a script from Joe Perches, this was easily done.
> 
> Reported-by: Joe Perches <j...@perches.com>
> Cc: Peter Chen <peter.c...@nxp.com>
> Cc: Felipe Balbi <ba...@kernel.org>
> Cc: Johan Hovold <jo...@kernel.org>
> Cc: Valentina Manea <valentina.mane...@gmail.com>
> Cc: Shuah Khan <sh...@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> ---
>  drivers/usb/chipidea/otg_fsm.c | 4 ++--
>  drivers/usb/gadget/udc/core.c  | 8 
>  drivers/usb/phy/phy-mv-usb.c   | 4 ++--
>  drivers/usb/serial/ftdi_sio.c  | 4 ++--
>  drivers/usb/usbip/stub_dev.c   | 4 ++--
>  drivers/usb/usbip/vhci_sysfs.c | 8 
>  drivers/usb/usbip/vudc_sysfs.c | 4 ++--
>  7 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> index d076cfa22fdf..6ed4b00dba96 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -162,7 +162,7 @@ b_bus_req_store(struct device *dev, struct 
> device_attribute *attr,
>  static DEVICE_ATTR_RW(b_bus_req);
>  
>  static ssize_t
> -set_a_clr_err(struct device *dev, struct device_attribute *attr,
> +a_clr_err_store(struct device *dev, struct device_attribute *attr,
>   const char *buf, size_t count)
>  {
>   struct ci_hdrc  *ci = dev_get_drvdata(dev);
> @@ -179,7 +179,7 @@ set_a_clr_err(struct device *dev, struct device_attribute 
> *attr,
>  
>   return count;
>  }
> -static DEVICE_ATTR(a_clr_err, S_IWUSR, NULL, set_a_clr_err);
> +static DEVICE_ATTR_WO(a_clr_err);
>  
>  static struct attribute *inputs_attrs[] = {
>   _attr_a_bus_req.attr,
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index ac0541529499..859d5b11ba4c 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1417,7 +1417,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_unregister_driver);
>  
>  /* - 
> */
>  
> -static ssize_t usb_udc_srp_store(struct device *dev,
> +static ssize_t srp_store(struct device *dev,
>   struct device_attribute *attr, const char *buf, size_t n)
>  {
>   struct usb_udc  *udc = container_of(dev, struct usb_udc, dev);
> @@ -1427,9 +1427,9 @@ static ssize_t usb_udc_srp_store(struct device *dev,
>  
>   return n;
>  }
> -static DEVICE_ATTR(srp, S_IWUSR, NULL, usb_udc_srp_store);
> +static DEVICE_ATTR_WO(srp);
>  
> -static ssize_t usb_udc_softconn_store(struct device *dev,
> +static ssize_t soft_connect_store(struct device *dev,
>   struct device_attribute *attr, const char *buf, size_t n)
>  {
>   struct usb_udc  *udc = container_of(dev, struct usb_udc, dev);
> @@ -1453,7 +1453,7 @@ static ssize_t usb_udc_softconn_store(struct device 
> *dev,
>  
>   return n;
>  }
> -static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);
> +static DEVICE_ATTR_WO(soft_connect);
>  
>  static ssize_t state_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> diff --git a/drivers/usb/phy/phy-mv-usb.c b/drivers/usb/phy/phy-mv-usb.c
> index 49a4dd88c301..cfd9add10bf4 100644
> --- a/drivers/usb/phy/phy-mv-usb.c
> +++ b/drivers/usb/phy/phy-mv-usb.c
> @@ -562,7 +562,7 @@ a_bus_req_store(struct device *dev, struct 
> device_attribute *attr,
>  static DEVICE_ATTR_RW(a_bus_req);
>  
>  static ssize_t
> -set_a_clr_err(struct device *dev, struct device_attribute *attr,
> +a_clr_err_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
>  {
>   struct mv_otg *mvotg = dev_get_drvdata(dev);
> @@ -586,7 +586,7 @@ set_a_clr_err(struct device *dev, struct device_attribute 
> *attr,
>   return count;
>  }
>  
> -static DEVICE_ATTR(a_clr_err, S_IWUSR, NULL, set_a_clr_err);
> +static DEVICE_ATTR_WO(a_clr_err);
>  
>  static ssize_t
>  a_bus_drop_show(struct device *dev, struct device_attribute *attr,
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index fc68952c994a..f58c4ff6b387 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1691,7 +1691,7 @@ static DEVICE_ATTR_RW(latency_timer);
>  
>  /* Write an event character directly to the FTDI registe

[PATCH] usbip: vhci_hcd: update 'status' file header and format

[Shuah Khan]

Commit 2f2d0088eb93
("usbip: prevent vhci_hcd driver from leaking a socket pointer address")
in the /sys/devices/platform/vhci_hcd/status.

Fix the header and field alignment to reflect the changes and make it
easier to read.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_sysfs.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index 091f76b7196d..a9de15cab2ec 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -17,10 +17,10 @@
 
 /*
  * output example:
- * hub port sta spd dev   sockfdlocal_busid
- * hs   004 000   3 1-2.3
+ * hub port sta spd dev   sockfd local_busid
+ * hs   004 000   03 1-2.3
  * 
- * ss  0008 004 000   4 2-3.4
+ * ss  0008 004 000   04 2-3.4
  * 
  *
  * Output includes socket fd instead of socket pointer address to avoid
@@ -44,13 +44,13 @@ static void port_show_vhci(char **out, int hub, int port, 
struct vhci_device *vd
if (vdev->ud.status == VDEV_ST_USED) {
*out += sprintf(*out, "%03u %08x ",
  vdev->speed, vdev->devid);
-   *out += sprintf(*out, "%u %s",
+   *out += sprintf(*out, "%06u %s",
  vdev->ud.sockfd,
  dev_name(>udev->dev));
 
} else {
*out += sprintf(*out, "000  ");
-   *out += sprintf(*out, " 0-0");
+   *out += sprintf(*out, "00 0-0");
}
 
*out += sprintf(*out, "\n");
@@ -148,7 +148,7 @@ static ssize_t status_show(struct device *dev,
int pdev_nr;
 
out += sprintf(out,
-  "hub port sta spd dev  socket   
local_busid\n");
+  "hub port sta spd dev  sockfd local_busid\n");
 
pdev_nr = status_name_to_id(attr->attr.name);
if (pdev_nr < 0)
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.9] usbip: fix warning in vhci_hcd_probe/lockdep_init_map

[Shuah Khan]

commit 918b8ac55b6c809b70aa05c279087109584e393e upstream

vhci_hcd calls sysfs_create_group() with dynamically allocated sysfs
attributes triggering the lock-class key not persistent warning. Call
sysfs_attr_init() for dynamically allocated sysfs attributes to fix it.

vhci_hcd vhci_hcd: USB/IP Virtual Host Controller
vhci_hcd vhci_hcd: new USB bus registered, assigned bus number 2
BUG: key 88006a7e8d18 not in .data!
[ cut here ]
WARNING: CPU: 0 PID: 1 at kernel/locking/lockdep.c:3131
lockdep_init_map+0x60c/0x770
DEBUG_LOCKS_WARN_ON(1)[1.567044] Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-rc7+ #58
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 88006bce6eb8 81f96c8a 0a02 11000d79cd6a
 ed000d79cd62 00046bce6ed8 41b58ab3 8598af40
 81f969f8  41b58ab3 0200
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x292/0x398 lib/dump_stack.c:51
 [] __warn+0x19f/0x1e0 kernel/panic.c:550
 [] warn_slowpath_fmt+0xc5/0x110 kernel/panic.c:565
 [] lockdep_init_map+0x60c/0x770 kernel/locking/lockdep.c:3131
 [] __kernfs_create_file+0x114/0x2a0 fs/kernfs/file.c:954
 [] sysfs_add_file_mode_ns+0x225/0x520 fs/sysfs/file.c:305
 [< inline >] create_files fs/sysfs/group.c:64
 [] internal_create_group+0x239/0x8f0 fs/sysfs/group.c:134
 [] sysfs_create_group+0x1f/0x30 fs/sysfs/group.c:156
 [] vhci_start+0x5b4/0x7a0 drivers/usb/usbip/vhci_hcd.c:978
 [] usb_add_hcd+0x8da/0x1c60 drivers/usb/core/hcd.c:2867
 [] vhci_hcd_probe+0x97/0x130
drivers/usb/usbip/vhci_hcd.c:1103
 ---
 ---
---[ end trace c33c7b202cf3aac8 ]---

Reported-by: Andrey Konovalov <andreyk...@google.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---

Greg,

Please apply this fix to 4.9 stable. I re-discovered the problem
on 4.9.77-rc1 and re-tested the patch on it.

thanks,
-- Shuah

 drivers/usb/usbip/vhci_sysfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index c404017..b96e5b1 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -361,6 +361,7 @@ static void set_status_attr(int id)
status->attr.attr.name = status->name;
status->attr.attr.mode = S_IRUGO;
status->attr.show = status_show;
+   sysfs_attr_init(>attr.attr);
 }
 
 static int init_status_attrs(void)
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: list: don't list devices attached to vhci_hcd

[Shuah Khan]

usbip host lists devices attached to vhci_hcd on the same server
when user does attach over localhost or specifies the server as the
remote.

usbip attach -r localhost -b busid
or
usbip attach -r servername (or server IP)

Fix it to check and not list devices that are attached to vhci_hcd.

Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip_list.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tools/usb/usbip/src/usbip_list.c b/tools/usb/usbip/src/usbip_list.c
index f1b38e866dd7..d65a9f444174 100644
--- a/tools/usb/usbip/src/usbip_list.c
+++ b/tools/usb/usbip/src/usbip_list.c
@@ -187,6 +187,7 @@ static int list_devices(bool parsable)
const char *busid;
char product_name[128];
int ret = -1;
+   const char *devpath;
 
/* Create libudev context. */
udev = udev_new();
@@ -209,6 +210,14 @@ static int list_devices(bool parsable)
path = udev_list_entry_get_name(dev_list_entry);
dev = udev_device_new_from_syspath(udev, path);
 
+   /* Ignore devices attached to vhci_hcd */
+   devpath = udev_device_get_devpath(dev);
+   if (strstr(devpath, USBIP_VHCI_DRV_NAME)) {
+   dbg("Skip the device %s already attached to %s\n",
+   devpath, USBIP_VHCI_DRV_NAME);
+   continue;
+   }
+
/* Get device information. */
idVendor = udev_device_get_sysattr_value(dev, "idVendor");
idProduct = udev_device_get_sysattr_value(dev, "idProduct");
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: prevent bind loops on devices attached to vhci_hcd

[Shuah Khan]

usbip host binds to devices attached to vhci_hcd on the same server
when user does attach over localhost or specifies the server as the
remote.

usbip attach -r localhost -b busid
or
usbip attach -r servername (or server IP)

Unbind followed by bind works, however device is left in a bad state with
accesses via the attached busid result in errors and system hangs during
shutdown.

Fix it to check and bail out if the device is already attached to vhci_hcd.

Cc: sta...@vger.kernel.org
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 tools/usb/usbip/src/usbip_bind.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/tools/usb/usbip/src/usbip_bind.c b/tools/usb/usbip/src/usbip_bind.c
index fa46141ae68b..e121cfb1746a 100644
--- a/tools/usb/usbip/src/usbip_bind.c
+++ b/tools/usb/usbip/src/usbip_bind.c
@@ -144,6 +144,7 @@ static int bind_device(char *busid)
int rc;
struct udev *udev;
struct udev_device *dev;
+   const char *devpath;
 
/* Check whether the device with this bus ID exists. */
udev = udev_new();
@@ -152,8 +153,16 @@ static int bind_device(char *busid)
err("device with the specified bus ID does not exist");
return -1;
}
+   devpath = udev_device_get_devpath(dev);
udev_unref(udev);
 
+   /* If the device is already attached to vhci_hcd - bail out */
+   if (strstr(devpath, USBIP_VHCI_DRV_NAME)) {
+   err("bind loop detected: device: %s is attached to %s\n",
+   devpath, USBIP_VHCI_DRV_NAME);
+   return -1;
+   }
+
rc = unbind_other(busid);
if (rc == UNBIND_ST_FAILED) {
err("could not unbind driver from device on busid %s", busid);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] USBIP: return correct port ENABLE status

[Shuah Khan]

On 12/18/2017 11:00 PM, pei.zh...@intel.com wrote:
> From: Pei Zhang 
> 
> USB system will clear port's ENABLE feature for some USB devices when
> vdev is already assigned port address. This cause getPortStatus reports
> to system that this device is not enabled, client OS will failed to use
> this usb device.
> 
> The failure devices include a SAMSUNG SSD storage, Logitech webcam C920.
> 
> V2: send again to all related maintainers.
> 
> Signed-off-by: Pei Zhang 

Hi Pei Zhang,

Can you elaborate on how you can trigger this condition? Can you send me
more information on how to recreate the problem and demsg from server and
client side when this problem happens.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][next] usbip: vhci: fix spelling mistake: "synchronuously" -> "synchronously"

[Shuah Khan]

On 01/03/2018 02:18 AM, Colin King wrote:
> From: Colin Ian King <colin.k...@canonical.com>
> 
> Trivial fix to spelling mistake in dev_dbg debug message.
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/usb/usbip/vhci_rx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
> index 112ebb90d8c9..44cd64518925 100644
> --- a/drivers/usb/usbip/vhci_rx.c
> +++ b/drivers/usb/usbip/vhci_rx.c
> @@ -30,7 +30,7 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device 
> *vdev, __u32 seqnum)
>   /* fall through */
>   case -ECONNRESET:
>   dev_dbg(>dev->dev,
> -  "urb seq# %u was unlinked %ssynchronuously\n",
> +  "urb seq# %u was unlinked %ssynchronously\n",
>seqnum, status == -ENOENT ? "" : "a");
>   break;
>   case -EINPROGRESS:
> 

Thanks Colin!

Acked-by: Shuah Khan <shua...@osg.samsung.com>

Greg, Please pick this up.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: usbip: remove useless call in usbip_recv

[Shuah Khan]

On 01/02/2018 07:02 AM, Gustavo A. R. Silva wrote:
> Calling msg_data_left() is only useful for its return value,
> which in this particular case is ignored.
> 
> Fix this by removing such call.
> 
> Addresses-Coverity-ID: 1427080
> Fixes: 90120d15f4c3 ("usbip: prevent leaking socket pointer address in 
> messages")
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
> ---
>  drivers/usb/usbip/usbip_common.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/usbip_common.c 
> b/drivers/usb/usbip/usbip_common.c
> index 7b219d9..b5af6fc 100644
> --- a/drivers/usb/usbip/usbip_common.c
> +++ b/drivers/usb/usbip/usbip_common.c
> @@ -325,7 +325,6 @@ int usbip_recv(struct socket *sock, void *buf, int size)
>   usbip_dbg_xmit("enter\n");
>  
>   do {
> - msg_data_left();
>   sock->sk->sk_allocation = GFP_NOIO;
>  
>   result = sock_recvmsg(sock, , MSG_WAITALL);
> 

Thanks for the patch. Looks good to me.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

Greg, please pick this patch up.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools: usb: usbip_device_driver: prefer 'unsigned int' to 'unsigned'

[Shuah Khan]

On 12/30/2017 09:11 AM, Elad Wexler wrote:
> Fixup a coding style issue
> 
> Signed-off-by: Elad Wexler <elad.wex...@gmail.com>


Thanks for the patch. Looks good to me.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

Greg, please pick this patch up.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tools: usb: usbip: fix fd leak in case of 'fread' failure

[Shuah Khan]

On 12/30/2017 09:01 AM, Elad Wexler wrote:
> Fix possible resource leak: fd
> 
> Signed-off-by: Elad Wexler <elad.wex...@gmail.com>

Thanks for the patch. Looks good to me.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

Greg, please pick this patch up.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vudc_tx: fix v_send_ret_submit() vulnerability to null xfer buffer

[Shuah Khan]

v_send_ret_submit() handles urb with a null transfer_buffer, when it
replays a packet with potential malicious data that could contain a
null buffer.

Add a check for the condition when actual_length > 0 and transfer_buffer
is null.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vudc_tx.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c
index 1440ae0919ec..3ccb17c3e840 100644
--- a/drivers/usb/usbip/vudc_tx.c
+++ b/drivers/usb/usbip/vudc_tx.c
@@ -85,6 +85,13 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp 
*urb_p)
memset(_header, 0, sizeof(pdu_header));
memset(, 0, sizeof(msg));
 
+   if (urb->actual_length > 0 && !urb->transfer_buffer) {
+   dev_err(>gadget.dev,
+   "urb: actual_length %d transfer_buffer null\n",
+   urb->actual_length);
+   return -1;
+   }
+
if (urb_p->type == USB_ENDPOINT_XFER_ISOC)
iovnum = 2 + urb->number_of_packets;
else
@@ -100,8 +107,8 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp 
*urb_p)
 
/* 1. setup usbip_header */
setup_ret_submit_pdu(_header, urb_p);
-   usbip_dbg_stub_tx("setup txdata seqnum: %d urb: %p\n",
- pdu_header.base.seqnum, urb);
+   usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+ pdu_header.base.seqnum);
usbip_header_correct_endian(_header, 1);
 
iov[iovnum].iov_base = _header;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: fix vudc_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

Harden CMD_SUBMIT path to handle malicious input that could trigger
large memory allocations. Add checks to validate transfer_buffer_length
and number_of_packets to protect against bad input requesting for
unbounded memory allocations.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vudc_rx.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
index df1e30989148..1e8a23d92cb4 100644
--- a/drivers/usb/usbip/vudc_rx.c
+++ b/drivers/usb/usbip/vudc_rx.c
@@ -120,6 +120,25 @@ static int v_recv_cmd_submit(struct vudc *udc,
urb_p->new = 1;
urb_p->seqnum = pdu->base.seqnum;
 
+   if (urb_p->ep->type == USB_ENDPOINT_XFER_ISOC) {
+   /* validate packet size and number of packets */
+   unsigned int maxp, packets, bytes;
+
+   maxp = usb_endpoint_maxp(urb_p->ep->desc);
+   maxp *= usb_endpoint_maxp_mult(urb_p->ep->desc);
+   bytes = pdu->u.cmd_submit.transfer_buffer_length;
+   packets = DIV_ROUND_UP(bytes, maxp);
+
+   if (pdu->u.cmd_submit.number_of_packets < 0 ||
+   pdu->u.cmd_submit.number_of_packets > packets) {
+   dev_err(>gadget.dev,
+   "CMD_SUBMIT: isoc invalid num packets %d\n",
+   pdu->u.cmd_submit.number_of_packets);
+   ret = -EMSGSIZE;
+   goto free_urbp;
+   }
+   }
+
ret = alloc_urb_from_cmd(_p->urb, pdu, urb_p->ep->type);
if (ret) {
usbip_event_add(>ud, VUDC_EVENT_ERROR_MALLOC);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: remove kernel addresses from usb device and urb debug msgs

[Shuah Khan]

usbip_dump_usb_device() and usbip_dump_urb() print kernel addresses.
Remove kernel addresses from usb device and urb debug msgs and improve
the message content.

Instead of printing parent device and bus addresses, print parent device
and bus names.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.c | 17 +++--
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index 7b219d9109b4..ee2bbce24584 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -91,7 +91,7 @@ static void usbip_dump_usb_device(struct usb_device *udev)
dev_dbg(dev, "   devnum(%d) devpath(%s) usb speed(%s)",
udev->devnum, udev->devpath, usb_speed_string(udev->speed));
 
-   pr_debug("tt %p, ttport %d\n", udev->tt, udev->ttport);
+   pr_debug("tt hub ttport %d\n", udev->ttport);
 
dev_dbg(dev, "");
for (i = 0; i < 16; i++)
@@ -124,12 +124,8 @@ static void usbip_dump_usb_device(struct usb_device *udev)
}
pr_debug("\n");
 
-   dev_dbg(dev, "parent %p, bus %p\n", udev->parent, udev->bus);
-
-   dev_dbg(dev,
-   "descriptor %p, config %p, actconfig %p, rawdescriptors %p\n",
-   >descriptor, udev->config,
-   udev->actconfig, udev->rawdescriptors);
+   dev_dbg(dev, "parent %s, bus %s\n", dev_name(>parent->dev),
+   udev->bus->bus_name);
 
dev_dbg(dev, "have_langid %d, string_langid %d\n",
udev->have_langid, udev->string_langid);
@@ -237,9 +233,6 @@ void usbip_dump_urb(struct urb *urb)
 
dev = >dev->dev;
 
-   dev_dbg(dev, "   urb   :%p\n", urb);
-   dev_dbg(dev, "   dev   :%p\n", urb->dev);
-
usbip_dump_usb_device(urb->dev);
 
dev_dbg(dev, "   pipe  :%08x ", urb->pipe);
@@ -248,11 +241,9 @@ void usbip_dump_urb(struct urb *urb)
 
dev_dbg(dev, "   status:%d\n", urb->status);
dev_dbg(dev, "   transfer_flags:%08X\n", urb->transfer_flags);
-   dev_dbg(dev, "   transfer_buffer   :%p\n", urb->transfer_buffer);
dev_dbg(dev, "   transfer_buffer_length:%d\n",
urb->transfer_buffer_length);
dev_dbg(dev, "   actual_length :%d\n", urb->actual_length);
-   dev_dbg(dev, "   setup_packet  :%p\n", urb->setup_packet);
 
if (urb->setup_packet && usb_pipetype(urb->pipe) == PIPE_CONTROL)
usbip_dump_usb_ctrlrequest(
@@ -262,8 +253,6 @@ void usbip_dump_urb(struct urb *urb)
dev_dbg(dev, "   number_of_packets :%d\n", urb->number_of_packets);
dev_dbg(dev, "   interval  :%d\n", urb->interval);
dev_dbg(dev, "   error_count   :%d\n", urb->error_count);
-   dev_dbg(dev, "   context   :%p\n", urb->context);
-   dev_dbg(dev, "   complete  :%p\n", urb->complete);
 }
 EXPORT_SYMBOL_GPL(usbip_dump_urb);
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: vhci: stop printing kernel pointer addresses in messages

[Shuah Khan]

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/vhci_hcd.c | 10 --
 drivers/usb/usbip/vhci_rx.c  | 23 +++
 drivers/usb/usbip/vhci_tx.c  |  3 ++-
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 390053e05dca..4dc8b42db7d9 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -656,9 +656,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb 
*urb, gfp_t mem_flag
struct vhci_device *vdev;
unsigned long flags;
 
-   usbip_dbg_vhci_hc("enter, usb_hcd %p urb %p mem_flags %d\n",
- hcd, urb, mem_flags);
-
if (portnum > VHCI_HC_PORTS) {
pr_err("invalid port number %d\n", portnum);
return -ENODEV;
@@ -822,8 +819,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
struct vhci_device *vdev;
unsigned long flags;
 
-   pr_info("dequeue a urb %p\n", urb);
-
spin_lock_irqsave(>lock, flags);
 
priv = urb->hcpriv;
@@ -851,7 +846,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
/* tcp connection is closed */
spin_lock(>priv_lock);
 
-   pr_info("device %p seems to be disconnected\n", vdev);
list_del(>list);
kfree(priv);
urb->hcpriv = NULL;
@@ -863,8 +857,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 * vhci_rx will receive RET_UNLINK and give back the URB.
 * Otherwise, we give back it here.
 */
-   pr_info("gives back urb %p\n", urb);
-
usb_hcd_unlink_urb_from_ep(hcd, urb);
 
spin_unlock_irqrestore(>lock, flags);
@@ -892,8 +884,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb 
*urb, int status)
 
unlink->unlink_seqnum = priv->seqnum;
 
-   pr_info("device %p seems to be still connected\n", vdev);
-
/* send cmd_unlink and try to cancel the pending URB in the
 * peer */
list_add_tail(>list, >unlink_tx);
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index 90577e8b2282..112ebb90d8c9 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -23,24 +23,23 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device 
*vdev, __u32 seqnum)
urb = priv->urb;
status = urb->status;
 
-   usbip_dbg_vhci_rx("find urb %p vurb %p seqnum %u\n",
-   urb, priv, seqnum);
+   usbip_dbg_vhci_rx("find urb seqnum %u\n", seqnum);
 
switch (status) {
case -ENOENT:
/* fall through */
case -ECONNRESET:
-   dev_info(>dev->dev,
-"urb %p was unlinked %ssynchronuously.\n", urb,
-status == -ENOENT ? "" : "a");
+   dev_dbg(>dev->dev,
+"urb seq# %u was unlinked %ssynchronuously\n",
+seqnum, status == -ENOENT ? "" : "a");
break;
case -EINPROGRESS:
/* no info output */
break;
default:
-   dev_info(>dev->dev,
-"urb %p may be in a error, status %d\n", urb,
-status);
+   dev_dbg(>dev->dev,
+"urb seq# %u may be in a error, status %d\n",
+seqnum, status);
}
 
list_del(>list);
@@ -67,8 +66,8 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
spin_unlock_irqrestore(>priv_lock, flags);
 
if (!urb) {
-   pr_err("cannot find a urb of seqnum %u\n", pdu->base.seqnum);
-   pr_info("max seqnum %d\n",
+   pr_err("cannot find a urb of seqnum %u max seqnum %d\n",
+   pdu->base.seqnum,
atomic_read(_hcd->seqnum));
usbip_event_add(ud, VDEV_EVENT_ERROR_TCP);
return;
@@ -91,7 +90,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
if (usbip_dbg_flag_vhci_rx)
usbip_dump_urb(urb);
 
-   usbip_dbg_vhci_rx("now giveb

[PATCH] usbip: stub: stop printing kernel pointer addresses in messages

[Shuah Khan]

Remove and/or change debug, info. and error messages to not print
kernel pointer addresses.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_main.c | 5 +++--
 drivers/usb/usbip/stub_rx.c   | 7 ++-
 drivers/usb/usbip/stub_tx.c   | 6 +++---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 4f48b306713f..c31c8402a0c5 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -237,11 +237,12 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
struct stub_priv *priv;
struct urb *urb;
 
-   dev_dbg(>udev->dev, "free sdev %p\n", sdev);
+   dev_dbg(>udev->dev, "Stub device cleaning up urbs\n");
 
while ((priv = stub_priv_pop(sdev))) {
urb = priv->urb;
-   dev_dbg(>udev->dev, "free urb %p\n", urb);
+   dev_dbg(>udev->dev, "free urb seqnum %lu\n",
+   priv->seqnum);
usb_kill_urb(urb);
 
kmem_cache_free(stub_priv_cache, priv);
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index b1820ab083e1..6c5a59313999 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -211,9 +211,6 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
if (priv->seqnum != pdu->u.cmd_unlink.seqnum)
continue;
 
-   dev_info(>urb->dev->dev, "unlink urb %p\n",
-priv->urb);
-
/*
 * This matched urb is not completed yet (i.e., be in
 * flight in usb hcd hardware/driver). Now we are
@@ -252,8 +249,8 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev,
ret = usb_unlink_urb(priv->urb);
if (ret != -EINPROGRESS)
dev_err(>urb->dev->dev,
-   "failed to unlink a urb %p, ret %d\n",
-   priv->urb, ret);
+   "failed to unlink a urb # %lu, ret %d\n",
+   priv->seqnum, ret);
 
return 0;
}
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index 53172b1f6257..f0ec41a50cbc 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -88,7 +88,7 @@ void stub_complete(struct urb *urb)
/* link a urb to the queue of tx. */
spin_lock_irqsave(>priv_lock, flags);
if (sdev->ud.tcp_socket == NULL) {
-   usbip_dbg_stub_tx("ignore urb for closed connection %p", urb);
+   usbip_dbg_stub_tx("ignore urb for closed connection\n");
/* It will be freed in stub_device_cleanup_urbs(). */
} else if (priv->unlinking) {
stub_enqueue_ret_unlink(sdev, priv->seqnum, urb->status);
@@ -190,8 +190,8 @@ static int stub_send_ret_submit(struct stub_device *sdev)
 
/* 1. setup usbip_header */
setup_ret_submit_pdu(_header, urb);
-   usbip_dbg_stub_tx("setup txdata seqnum: %d urb: %p\n",
- pdu_header.base.seqnum, urb);
+   usbip_dbg_stub_tx("setup txdata seqnum: %d\n",
+ pdu_header.base.seqnum);
usbip_header_correct_endian(_header, 1);
 
iov[iovnum].iov_base = _header;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: prevent leaking socket pointer address in messages

[Shuah Khan]

usbip driver is leaking socket pointer address in messages. Remove
the messages that aren't useful and print sockfd in the ones that
are useful for debugging.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_dev.c |  3 +--
 drivers/usb/usbip/usbip_common.c | 16 +---
 drivers/usb/usbip/vhci_hcd.c |  2 +-
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index a3df8ee82faf..e31a6f204397 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -149,8 +149,7 @@ static void stub_shutdown_connection(struct usbip_device 
*ud)
 * step 1?
 */
if (ud->tcp_socket) {
-   dev_dbg(>udev->dev, "shutdown tcp_socket %p\n",
-   ud->tcp_socket);
+   dev_dbg(>udev->dev, "shutdown sockfd %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
diff --git a/drivers/usb/usbip/usbip_common.c b/drivers/usb/usbip/usbip_common.c
index f7978933b402..7b219d9109b4 100644
--- a/drivers/usb/usbip/usbip_common.c
+++ b/drivers/usb/usbip/usbip_common.c
@@ -317,26 +317,20 @@ int usbip_recv(struct socket *sock, void *buf, int size)
struct msghdr msg = {.msg_flags = MSG_NOSIGNAL};
int total = 0;
 
+   if (!sock || !buf || !size)
+   return -EINVAL;
+
iov_iter_kvec(_iter, READ|ITER_KVEC, , 1, size);
 
usbip_dbg_xmit("enter\n");
 
-   if (!sock || !buf || !size) {
-   pr_err("invalid arg, sock %p buff %p size %d\n", sock, buf,
-  size);
-   return -EINVAL;
-   }
-
do {
-   int sz = msg_data_left();
+   msg_data_left();
sock->sk->sk_allocation = GFP_NOIO;
 
result = sock_recvmsg(sock, , MSG_WAITALL);
-   if (result <= 0) {
-   pr_debug("receive sock %p buf %p size %u ret %d total 
%d\n",
-sock, buf + total, sz, result, total);
+   if (result <= 0)
goto err;
-   }
 
total += result;
} while (msg_data_left());
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 7c7a341f7bb9..390053e05dca 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -975,7 +975,7 @@ static void vhci_shutdown_connection(struct usbip_device 
*ud)
 
/* need this? see stub_dev.c */
if (ud->tcp_socket) {
-   pr_debug("shutdown tcp_socket %p\n", ud->tcp_socket);
+   pr_debug("shutdown tcp_socket %d\n", ud->sockfd);
kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
}
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: fix usbip bind writing random string after command in match_busid

[Shuah Khan]

On 12/15/2017 02:21 AM, Juan Zea wrote:
> usbip bind writes commands followed by random string when writing to
> match_busid attribute in sysfs, caused by using full variable size
> instead of string length.
> 
> Signed-off-by: Juan Zea <juan@qindel.com>
> ---
>  tools/usb/usbip/src/utils.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c
> index 2b3d6d2..3d7b42e 100644
> --- a/tools/usb/usbip/src/utils.c
> +++ b/tools/usb/usbip/src/utils.c
> @@ -30,6 +30,7 @@ int modify_match_busid(char *busid, int add)
>   char command[SYSFS_BUS_ID_SIZE + 4];
>   char match_busid_attr_path[SYSFS_PATH_MAX];
>   int rc;
> + int cmd_size;
>  
>   snprintf(match_busid_attr_path, sizeof(match_busid_attr_path),
>"%s/%s/%s/%s/%s/%s", SYSFS_MNT_PATH, SYSFS_BUS_NAME,
> @@ -37,12 +38,14 @@ int modify_match_busid(char *busid, int add)
>attr_name);
>  
>   if (add)
> - snprintf(command, SYSFS_BUS_ID_SIZE + 4, "add %s", busid);
> + cmd_size = snprintf(command, SYSFS_BUS_ID_SIZE + 4, "add %s",
> + busid);
>   else
> - snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid);
> + cmd_size = snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s",
> + busid);
>  
>   rc = write_sysfs_attribute(match_busid_attr_path, command,
> -sizeof(command));
> +cmd_size);
>   if (rc < 0) {
>   dbg("failed to write match_busid: %s", strerror(errno));
>   return -1;
> 

Thanks for the patch.

Greg, could you please pick this up.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usbip: stub_rx: fix static checker warning on unnecessary checks

[Shuah Khan]

Fix the following static checker warnings:

The patch c6688ef9f297: "usbip: fix stub_rx: harden CMD_SUBMIT path
to handle malicious input" from Dec 7, 2017, leads to the following
static checker warning:

drivers/usb/usbip/stub_rx.c:346 get_pipe()
warn: impossible condition
'(pdu->u.cmd_submit.transfer_buffer_length > ((~0 >> 1))) =>
(s32min-s32max > s32max)'
drivers/usb/usbip/stub_rx.c:486 stub_recv_cmd_submit()
warn: always true condition
'(pdu->u.cmd_submit.transfer_buffer_length <= ((~0 >> 1))) =>
(s32min-s32max <= s32max)'

Reported-by: Dan Carpenter <dan.carpen...@oracle.com>
Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_rx.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 493ac2928391..b1820ab083e1 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -342,14 +342,6 @@ static int get_pipe(struct stub_device *sdev, struct 
usbip_header *pdu)
 
epd = >desc;
 
-   /* validate transfer_buffer_length */
-   if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
-   dev_err(>udev->dev,
-   "CMD_SUBMIT: -EMSGSIZE transfer_buffer_length %d\n",
-   pdu->u.cmd_submit.transfer_buffer_length);
-   return -1;
-   }
-
if (usb_endpoint_xfer_control(epd)) {
if (dir == USBIP_DIR_OUT)
return usb_sndctrlpipe(udev, epnum);
@@ -482,8 +474,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
}
 
/* allocate urb transfer buffer, if needed */
-   if (pdu->u.cmd_submit.transfer_buffer_length > 0 &&
-   pdu->u.cmd_submit.transfer_buffer_length <= INT_MAX) {
+   if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
priv->urb->transfer_buffer =
kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
GFP_KERNEL);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

On 12/14/2017 01:16 PM, Dan Carpenter wrote:
> On Thu, Dec 14, 2017 at 11:01:15AM -0700, Shuah Khan wrote:
>> Hi Dan,
>>
>> On 12/14/2017 12:58 AM, Dan Carpenter wrote:
>>> Hello Shuah Khan,
>>>
>>> The patch c6688ef9f297: "usbip: fix stub_rx: harden CMD_SUBMIT path
>>> to handle malicious input" from Dec 7, 2017, leads to the following
>>> static checker warning:
>>>
>>> drivers/usb/usbip/stub_rx.c:346 get_pipe()
>>> warn: impossible condition '(pdu->u.cmd_submit.transfer_buffer_length > 
>>> ((~0 >> 1))) => (s32min-s32max > s32max)'
>>> drivers/usb/usbip/stub_rx.c:486 stub_recv_cmd_submit()
>>> warn: always true condition '(pdu->u.cmd_submit.transfer_buffer_length 
>>> <= ((~0 >> 1))) => (s32min-s32max <= s32max)'
>>>
>>> drivers/usb/usbip/stub_rx.c
>>>343  epd = >desc;
>>>344  
>>>345  /* validate transfer_buffer_length */
>>>346  if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
>>>   ^^
>>> This is an int.
>>
>> Yeah the check should have been against S32_MAX for the two checks
>> in this patch.
> 
> TBH, I don't understand.
> 

Yeah. I didn't make any sense there. Anyway, I have to check against
a reasonable max value for this protocol. I will send a fix.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: fix usbip bind writing random string after command in match_busid

[Shuah Khan]

On 12/14/2017 03:23 AM, Juan Zea wrote:
>> Why not use the return value from snprintf() for length, instead of calling
> strlen(command)?
> 
> Yes, that makes sense. Something like this?

Yes

> 
> diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c
> index 2b3d6d2..3d7b42e 100644
> --- a/tools/usb/usbip/src/utils.c
> +++ b/tools/usb/usbip/src/utils.c
> @@ -30,6 +30,7 @@ int modify_match_busid(char *busid, int add)
> char command[SYSFS_BUS_ID_SIZE + 4];
> char match_busid_attr_path[SYSFS_PATH_MAX];
> int rc;
> +   int cmd_size;
>  
> snprintf(match_busid_attr_path, sizeof(match_busid_attr_path),
>  "%s/%s/%s/%s/%s/%s", SYSFS_MNT_PATH, SYSFS_BUS_NAME,
> @@ -37,12 +38,14 @@ int modify_match_busid(char *busid, int add)
>  attr_name);
>  
> if (add)
> -   snprintf(command, SYSFS_BUS_ID_SIZE + 4, "add %s", busid);
> +   cmd_size = snprintf(command, SYSFS_BUS_ID_SIZE + 4, "add %s",
> +   busid);
> else
> -   snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid);
> +   cmd_size = snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s",
> +   busid);
>  
> rc = write_sysfs_attribute(match_busid_attr_path, command,
> -  sizeof(command));
> +  cmd_size);
> if (rc < 0) {
> dbg("failed to write match_busid: %s", strerror(errno));
> return -1;
> 
> 
> Regards,
> Juan
> 

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug report] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

Hi Dan,

On 12/14/2017 12:58 AM, Dan Carpenter wrote:
> Hello Shuah Khan,
> 
> The patch c6688ef9f297: "usbip: fix stub_rx: harden CMD_SUBMIT path
> to handle malicious input" from Dec 7, 2017, leads to the following
> static checker warning:
> 
> drivers/usb/usbip/stub_rx.c:346 get_pipe()
> warn: impossible condition '(pdu->u.cmd_submit.transfer_buffer_length > 
> ((~0 >> 1))) => (s32min-s32max > s32max)'
> drivers/usb/usbip/stub_rx.c:486 stub_recv_cmd_submit()
> warn: always true condition '(pdu->u.cmd_submit.transfer_buffer_length <= 
> ((~0 >> 1))) => (s32min-s32max <= s32max)'
> 
> drivers/usb/usbip/stub_rx.c
>343  epd = >desc;
>344  
>345  /* validate transfer_buffer_length */
>346  if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
>   ^^
> This is an int.

Yeah the check should have been against S32_MAX for the two checks
in this patch.

snip
>483  
>484  /* allocate urb transfer buffer, if needed */
>485  if (pdu->u.cmd_submit.transfer_buffer_length > 0 &&
>486  pdu->u.cmd_submit.transfer_buffer_length <= INT_MAX) {

I will send a new version fixing the problem.

Greg, this patch is in usb-linus - would you like me to send a patch fixing
the warn or send v2 for this patch? I can go with whichever works the best
for you.

thanks,
-- Shuah


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: fix usbip bind writing random string after command in match_busid

[Shuah Khan]

On 12/13/2017 04:07 AM, Juan Zea wrote:
> usbip bind writes commands followed by random string when writing to
> match_busid attribute in sysfs, caused by using full variable size
> instead of string length.
> 
> Signed-off-by: Juan Zea 
> ---
>  tools/usb/usbip/src/utils.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/src/utils.c b/tools/usb/usbip/src/utils.c
> index 2b3d6d2..ea1a1af 100644
> --- a/tools/usb/usbip/src/utils.c
> +++ b/tools/usb/usbip/src/utils.c
> @@ -42,7 +42,7 @@ int modify_match_busid(char *busid, int add)
> snprintf(command, SYSFS_BUS_ID_SIZE + 4, "del %s", busid);
>  
> rc = write_sysfs_attribute(match_busid_attr_path, command,
> -  sizeof(command));
> +  strlen(command));
> if (rc < 0) {
> dbg("failed to write match_busid: %s", strerror(errno));
> return -1;
> 

Why not use the return value from snprintf() for length, instead of calling
strlen(command)?

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] USB: remove the URB_NO_FSBR flag

[Shuah Khan]

On 12/11/2017 09:58 AM, Alan Stern wrote:
> The URB_NO_FSBR flag has never really been used.  It was introduced as
> a potential way for UHCI to minimize PCI bus usage (by not attempting
> full-speed bulk and control transfers more than once per frame), but
> the flag was not set by any drivers.
> 
> There's no point in keeping it around.  This patch simplifies the API
> by removing it.  Unfortunately, it does have to be kept as part of the
> usbfs ABI, but at least we can document in
> include/uapi/linux/usbdevice_fs.h that it doesn't do anything.
> 
> Signed-off-by: Alan Stern <st...@rowland.harvard.edu>
> 
> ---
> 
> Shuah, I'm not sure if this will cause an interoperability problem for 
> usbip.  In theory, a machine running an older kernel might set the 
> URB_NO_FSBR flag when communicating with a new kernel, which would 
> cause an error.  But I don't know any reason why even an old kernel 
> would ever set the flag.

Alan, I am not sure how the interoperability will manifest.



> ===
> --- usb-4.x.orig/drivers/usb/core/urb.c
> +++ usb-4.x/drivers/usb/core/urb.c
> @@ -479,9 +479,6 @@ int usb_submit_urb(struct urb *urb, gfp_
>   if (is_out)
>   allowed |= URB_ZERO_PACKET;
>   /* FALLTHROUGH */
> - case USB_ENDPOINT_XFER_CONTROL:
> - allowed |= URB_NO_FSBR; /* only affects UHCI */
> - /* FALLTHROUGH */
>   default:/* all non-iso endpoints */
>   if (!is_out)
>   allowed |= URB_SHORT_NOT_OK;
> Index: usb-4.x/drivers/usb/host/uhci-q.c
> ===
> --- usb-4.x.orig/drivers/usb/host/uhci-q.c
> +++ usb-4.x/drivers/usb/host/uhci-q.c
> @@ -73,8 +73,7 @@ static void uhci_add_fsbr(struct uhci_hc
>  {
>   struct urb_priv *urbp = urb->hcpriv;
>  
> - if (!(urb->transfer_flags & URB_NO_FSBR))
> - urbp->fsbr = 1;
> + urbp->fsbr = 1;

So essentially fsbr gets set always. URB_NO_FSBR has no effect.

>  }
>  
>  static void uhci_urbp_wants_fsbr(struct uhci_hcd *uhci, struct urb_priv 
> *urbp)
> Index: usb-4.x/drivers/usb/usbip/stub_rx.c
> ===
> --- usb-4.x.orig/drivers/usb/usbip/stub_rx.c
> +++ usb-4.x/drivers/usb/usbip/stub_rx.c
> @@ -412,9 +412,6 @@ static void masking_bogus_flags(struct u
>   if (is_out)
>   allowed |= URB_ZERO_PACKET;
>   /* FALLTHROUGH */
> - case USB_ENDPOINT_XFER_CONTROL:
> - allowed |= URB_NO_FSBR; /* only affects UHCI */
> - /* FALLTHROUGH */

usbip doesn't really do anything special for URB_NO_FSBR, other
than just setting the flag here. This is followed by usb_submit_urb().
Probably there is no need to set URB_NO_FSBR in USBIP to begin with,
considering usb_submit_urb() handled that before this patch.

I think it is safe to make the change.

Acked-by: Shuah Khan <shua...@osg.samsung.com>

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] USB over IP Secuurity fixes

[Shuah Khan]

On 12/08/2017 09:33 AM, Greg KH wrote:
> On Fri, Dec 08, 2017 at 08:44:58AM -0700, Shuah Khan wrote:
>> Hi Jakub,
>>
>> On 12/08/2017 08:14 AM, Secunia Research wrote:
>>> Hi Shuah,
>>>
>>> Thanks a lot for the quick fixes.
>>
>> Thanks for finding them and doing all the leg work in
>> pin pointing the issues.
>>
>>>
>>> Please, use this email address: v...@secunia.com
>>>
>>> We have assigned the following CVEs to the issues:
>>> CVE-2017-16911 usbip: prevent vhci_hcd driver from leaking a socket pointer
>>> address
>>> CVE-2017-16912 usbip: fix stub_rx: get_pipe() to validate endpoint number
>>> CVE-2017-16913 usbip: fix stub_rx: harden CMD_SUBMIT path to handle
>>> malicious input
>>> CVE-2017-16914 usbip: fix stub_send_ret_submit() vulnerability to null
>>> transfer_buffer
>>>
>>> Please, let me know if we should proceed with a coordinated disclosure. I'm
>>> not quite sure how many distros / downstreams actually use this
>>> functionality.
>>
>> I believe so. We have to get these into mainline and propagate them into
>> stables first which could take a couple of weeks.
>>
>> I will defer to Greg KH on this to comment and weigh in.
> 
> I've queued them all up and will send them to Linus in a few days.

Thanks.

> 
> As for "disclosure", well, you all are talking about this on a public
> mailing list, so I think there's really not much else that needs to be
> "disclosed" :)

Yes :)

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] USB over IP Secuurity fixes

[Shuah Khan]

Hi Jakub,

On 12/08/2017 08:14 AM, Secunia Research wrote:
> Hi Shuah,
> 
> Thanks a lot for the quick fixes.

Thanks for finding them and doing all the leg work in
pin pointing the issues.

> 
> Please, use this email address: v...@secunia.com
> 
> We have assigned the following CVEs to the issues:
> CVE-2017-16911 usbip: prevent vhci_hcd driver from leaking a socket pointer
> address
> CVE-2017-16912 usbip: fix stub_rx: get_pipe() to validate endpoint number
> CVE-2017-16913 usbip: fix stub_rx: harden CMD_SUBMIT path to handle
> malicious input
> CVE-2017-16914 usbip: fix stub_send_ret_submit() vulnerability to null
> transfer_buffer
> 
> Please, let me know if we should proceed with a coordinated disclosure. I'm
> not quite sure how many distros / downstreams actually use this
> functionality.

I believe so. We have to get these into mainline and propagate them into
stables first which could take a couple of weeks.

I will defer to Greg KH on this to comment and weigh in.

thanks,
-- Shuah

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] USB over IP Secuurity fixes

[Shuah Khan]

On 12/07/2017 11:25 PM, Greg KH wrote:
> On Thu, Dec 07, 2017 at 02:16:46PM -0700, Shuah Khan wrote:
>> Jakub Jirasek from Secunia Research at Flexera reported security
>> vulnerabilities in the USB over IP driver. This patch series all
>> the 4 reported problems.
> 
> Nice!
> 
> These should also all go to the stable kernels, right?
> 
> thanks,
> 
> greg k-h
> 

These should go into stables as well. I think 3/4 needs to worked as
a special patch for 4.9 - which I can take care of for 4.9 stable.

thanks,
-- Shuah
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] usbip: fix stub_rx: get_pipe() to validate endpoint number

[Shuah Khan]

get_pipe() routine doesn't validate the input endpoint number
and uses to reference ep_in and ep_out arrays. Invalid endpoint
number can trigger BUG(). Range check the epnum and returning
error instead of calling BUG().

Change caller stub_recv_cmd_submit() to handle the get_pipe()
error return.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_rx.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 536e037f541f..4d61063c259d 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -328,15 +328,15 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
 
+   if (epnum < 0 || epnum > 15)
+   goto err_ret;
+
if (dir == USBIP_DIR_IN)
ep = udev->ep_in[epnum & 0x7f];
else
ep = udev->ep_out[epnum & 0x7f];
-   if (!ep) {
-   dev_err(>udev->dev, "no such endpoint?, %d\n",
-   epnum);
-   BUG();
-   }
+   if (!ep)
+   goto err_ret;
 
epd = >desc;
if (usb_endpoint_xfer_control(epd)) {
@@ -367,9 +367,10 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
return usb_rcvisocpipe(udev, epnum);
}
 
+err_ret:
/* NOT REACHED */
-   dev_err(>udev->dev, "get pipe, epnum %d\n", epnum);
-   return 0;
+   dev_err(>udev->dev, "get pipe() invalid epnum %d\n", epnum);
+   return -1;
 }
 
 static void masking_bogus_flags(struct urb *urb)
@@ -435,6 +436,9 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct usb_device *udev = sdev->udev;
int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
 
+   if (pipe == -1)
+   return;
+
priv = stub_priv_alloc(sdev, pdu);
if (!priv)
return;
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] USB over IP Secuurity fixes

[Shuah Khan]

Jakub Jirasek from Secunia Research at Flexera reported security
vulnerabilities in the USB over IP driver. This patch series all
the 4 reported problems.

Jakub, could you please suggest an email address I can use for the
Reported-by tag?

Shuah Khan (4):
  usbip: fix stub_rx: get_pipe() to validate endpoint number
  usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input
  usbip: prevent vhci_hcd driver from leaking a socket pointer address
  usbip: fix stub_send_ret_submit() vulnerability to null
transfer_buffer

 drivers/usb/usbip/stub_rx.c  | 51 +---
 drivers/usb/usbip/stub_tx.c  |  7 +
 drivers/usb/usbip/usbip_common.h |  1 +
 drivers/usb/usbip/vhci_sysfs.c   | 25 +++---
 tools/usb/usbip/libsrc/vhci_driver.c |  8 +++---
 5 files changed, 69 insertions(+), 23 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] usbip: fix stub_rx: harden CMD_SUBMIT path to handle malicious input

[Shuah Khan]

Harden CMD_SUBMIT path to handle malicious input that could trigger
large memory allocations. Add checks to validate transfer_buffer_length
and number_of_packets to protect against bad input requesting for
unbounded memory allocations. Validate early in get_pipe() and return
failure.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_rx.c | 35 +++
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 4d61063c259d..493ac2928391 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -322,11 +322,13 @@ static struct stub_priv *stub_priv_alloc(struct 
stub_device *sdev,
return priv;
 }
 
-static int get_pipe(struct stub_device *sdev, int epnum, int dir)
+static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 {
struct usb_device *udev = sdev->udev;
struct usb_host_endpoint *ep;
struct usb_endpoint_descriptor *epd = NULL;
+   int epnum = pdu->base.ep;
+   int dir = pdu->base.direction;
 
if (epnum < 0 || epnum > 15)
goto err_ret;
@@ -339,6 +341,15 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
goto err_ret;
 
epd = >desc;
+
+   /* validate transfer_buffer_length */
+   if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
+   dev_err(>udev->dev,
+   "CMD_SUBMIT: -EMSGSIZE transfer_buffer_length %d\n",
+   pdu->u.cmd_submit.transfer_buffer_length);
+   return -1;
+   }
+
if (usb_endpoint_xfer_control(epd)) {
if (dir == USBIP_DIR_OUT)
return usb_sndctrlpipe(udev, epnum);
@@ -361,6 +372,21 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
}
 
if (usb_endpoint_xfer_isoc(epd)) {
+   /* validate packet size and number of packets */
+   unsigned int maxp, packets, bytes;
+
+   maxp = usb_endpoint_maxp(epd);
+   maxp *= usb_endpoint_maxp_mult(epd);
+   bytes = pdu->u.cmd_submit.transfer_buffer_length;
+   packets = DIV_ROUND_UP(bytes, maxp);
+
+   if (pdu->u.cmd_submit.number_of_packets < 0 ||
+   pdu->u.cmd_submit.number_of_packets > packets) {
+   dev_err(>udev->dev,
+   "CMD_SUBMIT: isoc invalid num packets %d\n",
+   pdu->u.cmd_submit.number_of_packets);
+   return -1;
+   }
if (dir == USBIP_DIR_OUT)
return usb_sndisocpipe(udev, epnum);
else
@@ -369,7 +395,7 @@ static int get_pipe(struct stub_device *sdev, int epnum, 
int dir)
 
 err_ret:
/* NOT REACHED */
-   dev_err(>udev->dev, "get pipe() invalid epnum %d\n", epnum);
+   dev_err(>udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
return -1;
 }
 
@@ -434,7 +460,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
struct stub_priv *priv;
struct usbip_device *ud = >ud;
struct usb_device *udev = sdev->udev;
-   int pipe = get_pipe(sdev, pdu->base.ep, pdu->base.direction);
+   int pipe = get_pipe(sdev, pdu);
 
if (pipe == -1)
return;
@@ -456,7 +482,8 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
}
 
/* allocate urb transfer buffer, if needed */
-   if (pdu->u.cmd_submit.transfer_buffer_length > 0) {
+   if (pdu->u.cmd_submit.transfer_buffer_length > 0 &&
+   pdu->u.cmd_submit.transfer_buffer_length <= INT_MAX) {
priv->urb->transfer_buffer =
kzalloc(pdu->u.cmd_submit.transfer_buffer_length,
GFP_KERNEL);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] usbip: fix stub_send_ret_submit() vulnerability to null transfer_buffer

[Shuah Khan]

stub_send_ret_submit() handles urb with a potential null transfer_buffer,
when it replays a packet with potential malicious data that could contain
a null buffer. Add a check for the condition when actual_length > 0 and
transfer_buffer is null.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/stub_tx.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index b18bce96c212..53172b1f6257 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -167,6 +167,13 @@ static int stub_send_ret_submit(struct stub_device *sdev)
memset(_header, 0, sizeof(pdu_header));
memset(, 0, sizeof(msg));
 
+   if (urb->actual_length > 0 && !urb->transfer_buffer) {
+   dev_err(>udev->dev,
+   "urb: actual_length %d transfer_buffer null\n",
+   urb->actual_length);
+   return -1;
+   }
+
if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
iovnum = 2 + urb->number_of_packets;
else
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] usbip: prevent vhci_hcd driver from leaking a socket pointer address

[Shuah Khan]

When a client has a USB device attached over IP, the vhci_hcd driver is
locally leaking a socket pointer address via the

/sys/devices/platform/vhci_hcd/status file (world-readable) and in debug
output when "usbip --debug port" is run.

Fix it to not leak. The socket pointer address is not used at the moment
and it was made visible as a convenient way to find IP address from socket
pointer address by looking up /proc/net/{tcp,tcp6}.

As this opens a security hole, the fix replaces socket pointer address with
sockfd.

Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
---
 drivers/usb/usbip/usbip_common.h |  1 +
 drivers/usb/usbip/vhci_sysfs.c   | 25 -
 tools/usb/usbip/libsrc/vhci_driver.c |  8 
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index e5de35c8c505..473fb8a87289 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -256,6 +256,7 @@ struct usbip_device {
/* lock for status */
spinlock_t lock;
 
+   int sockfd;
struct socket *tcp_socket;
 
struct task_struct *tcp_rx;
diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c
index e78f7472cac4..091f76b7196d 100644
--- a/drivers/usb/usbip/vhci_sysfs.c
+++ b/drivers/usb/usbip/vhci_sysfs.c
@@ -17,15 +17,20 @@
 
 /*
  * output example:
- * hub port sta spd dev  socket   local_busid
- * hs   004 000  c5a7bb80 1-2.3
+ * hub port sta spd dev   sockfdlocal_busid
+ * hs   004 000   3 1-2.3
  * 
- * ss  0008 004 000  d8cee980 2-3.4
+ * ss  0008 004 000   4 2-3.4
  * 
  *
- * IP address can be retrieved from a socket pointer address by looking
- * up /proc/net/{tcp,tcp6}. Also, a userland program may remember a
- * port number and its peer IP address.
+ * Output includes socket fd instead of socket pointer address to avoid
+ * leaking kernel memory address in:
+ * /sys/devices/platform/vhci_hcd.0/status and in debug output.
+ * The socket pointer address is not used at the moment and it was made
+ * visible as a convenient way to find IP address from socket pointer
+ * address by looking up /proc/net/{tcp,tcp6}. As this opens a security
+ * hole, the change is made to use sockfd instead.
+ *
  */
 static void port_show_vhci(char **out, int hub, int port, struct vhci_device 
*vdev)
 {
@@ -39,8 +44,8 @@ static void port_show_vhci(char **out, int hub, int port, 
struct vhci_device *vd
if (vdev->ud.status == VDEV_ST_USED) {
*out += sprintf(*out, "%03u %08x ",
  vdev->speed, vdev->devid);
-   *out += sprintf(*out, "%16p %s",
- vdev->ud.tcp_socket,
+   *out += sprintf(*out, "%u %s",
+ vdev->ud.sockfd,
  dev_name(>udev->dev));
 
} else {
@@ -160,7 +165,8 @@ static ssize_t nports_show(struct device *dev, struct 
device_attribute *attr,
char *s = out;
 
/*
-* Half the ports are for SPEED_HIGH and half for SPEED_SUPER, thus the 
* 2.
+* Half the ports are for SPEED_HIGH and half for SPEED_SUPER,
+* thus the * 2.
 */
out += sprintf(out, "%d\n", VHCI_PORTS * vhci_num_controllers);
return out - s;
@@ -366,6 +372,7 @@ static ssize_t store_attach(struct device *dev, struct 
device_attribute *attr,
 
vdev->devid = devid;
vdev->speed = speed;
+   vdev->ud.sockfd = sockfd;
vdev->ud.tcp_socket = socket;
vdev->ud.status = VDEV_ST_NOTASSIGNED;
 
diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
b/tools/usb/usbip/libsrc/vhci_driver.c
index 8a1cd1616de4..d1fc0f9f00fb 100644
--- a/tools/usb/usbip/libsrc/vhci_driver.c
+++ b/tools/usb/usbip/libsrc/vhci_driver.c
@@ -50,14 +50,14 @@ static int parse_status(const char *value)
 
while (*c != '\0') {
int port, status, speed, devid;
-   unsigned long socket;
+   int sockfd;
char lbusid[SYSFS_BUS_ID_SIZE];
struct usbip_imported_device *idev;
char hub[3];
 
-   ret = sscanf(c, "%2s  %d %d %d %x %lx %31s\n",
+   ret = sscanf(c, "%2s  %d %d %d %x %u %31s\n",
hub, , , ,
-   , , lbusid);
+   , , lbusid);
 
if (ret < 5) {
dbg("sscanf failed: %d", ret);
@@ -66,7 +66,7 @@ static int parse_status(const char *value)
 
dbg("hub %s port %d stat