[PATCH 0/4] USB over IP Secuurity fixes

[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

[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 <shuahkh@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(&sdev->udev->dev, "no such endpoint?, %d\n",
-			epnum);
-		BUG();
-	}
+	if (!ep)
+		goto err_ret;
 
 	epd = &ep->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(&sdev->udev->dev, "get pipe, epnum %d\n", epnum);
-	return 0;
+	dev_err(&sdev->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

[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 <shuahkh@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 = &ep->desc;
+
+	/* validate transfer_buffer_length */
+	if (pdu->u.cmd_submit.transfer_buffer_length > INT_MAX) {
+		dev_err(&sdev->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(&sdev->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(&sdev->udev->dev, "get pipe() invalid epnum %d\n", epnum);
+	dev_err(&sdev->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 = &sdev->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

[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 <shuahkh@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  0000 004 000 00000000         c5a7bb80 1-2.3
+ * hub port sta spd dev       sockfd    local_busid
+ * hs  0000 004 000 00000000  3         1-2.3
  * ................................................
- * ss  0008 004 000 00000000         d8cee980 2-3.4
+ * ss  0008 004 000 00000000  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(&vdev->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, &port, &status, &speed,
-				&devid, &socket, lbusid);
+				&devid, &sockfd, 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 status %d speed %d devid %x",
 				hub, port, status, speed, devid);
-		dbg("socket %lx lbusid %s", socket, lbusid);
+		dbg("sockfd %u lbusid %s", sockfd, lbusid);
 
 		/* if a device is connected, look at it */
 		idev = &vhci_driver->idev[port];
-- 
2.14.1

[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 <shuahkh@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(&pdu_header, 0, sizeof(pdu_header));
 		memset(&msg, 0, sizeof(msg));
 
+		if (urb->actual_length > 0 && !urb->transfer_buffer) {
+			dev_err(&sdev->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

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

[Greg KH]

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

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

[Secunia Research]

Hi Shuah,

Thanks a lot for the quick fixes.

Please, use this email address: vuln@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.

Best Regards,
Jakub

-----Original Message-----
From: Shuah Khan [mailto:shuahkh@osg.samsung.com] 
Sent: Thursday, December 7, 2017 10:17 PM
To: shuah@kernel.org; valentina.manea.m@gmail.com;
gregkh@linuxfoundation.org
Cc: Shuah Khan <shuahkh@osg.samsung.com>; linux-usb@vger.kernel.org;
linux-kernel@vger.kernel.org; vuln@secunia.com
Subject: [PATCH 0/4] USB over IP Secuurity fixes

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

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

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: vuln@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

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

[Greg KH]

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: vuln@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.

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" :)

thanks,

greg k-h

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: vuln@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