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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
Hi Shuah,
Thanks a lot for the quick fixes.
Please, use this email address: [email protected]
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:[email protected]]
Sent: Thursday, December 7, 2017 10:17 PM
To: [email protected]; [email protected];
[email protected]
Cc: Shuah Khan <[email protected]>; [email protected];
[email protected]; [email protected]
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
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
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: [email protected]
>
> 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
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: [email protected]
> >
> > 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
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: [email protected]
>>>
>>> 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