USB: hso: Fix OOB memory access in hso_probe/hso_get_config_data

From: Hui Peng <[email protected]>

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data. Added a length check for both locations and updated hso_probe to bail on error.

---
 drivers/net/usb/hso.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

index 184c24baca15..168f9081d4ea 100644
@@ -2807,6 +2807,12 @@ static int hso_get_config_data(struct usb_interface *interface)
 		return -EIO;
 	}
 
+	/* check if we have a valid interface */
+	if (if_num > 16) {
+		kfree(config_data);
+		return -EINVAL;
+	}
+
 	switch (config_data[if_num]) {
 	case 0x0:
 		result = 0;

@@ -2877,10 +2883,18 @@ static int hso_probe(struct usb_interface *interface,
 
 	/* Get the interface/port specification from either driver_info or from
 	 * the device itself */
-	if (id->driver_info)
+	if (id->driver_info) {
+		/* if_num is controlled by the device, driver_info is a 0 terminated
+		 * array. Make sure, the access is in bounds! */
+		for (i = 0; i <= if_num; ++i)
+			if (((u32 *)(id->driver_info))[i] == 0)
+				goto exit;
 		port_spec = ((u32 *)(id->driver_info))[if_num];
-	else
+	} else {
 		port_spec = hso_get_config_data(interface);
+		if (IS_ERR_VALUE((long)port_spec))
+			goto exit;
+	}
 
 	/* Check if we need to switch to alt interfaces prior to port
 	 * configuration */

From: Greg KH <[email protected]>
Date: Sun, 9 Dec 2018 17:32:45 +0100

Hi David,

Passing an int to the macro results in a compiler warning. One option would be to test for the individual errors (instead of using the macro) with the drawback that future extensions that return different errors may be missed. Another alternative is to test for negative values with the drawback that this bypasses the existing test macros.

Also, the macro does not cast back to int but unsigned long:

https://elixir.bootlin.com/linux/v4.20-rc5/source/include/linux/err.h#L22
#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >= (unsigned long)-MAX_ERRNO)

The cast chain is int -> long -> void * -> unsigned long.

What other check would you propose?

Thanks,
Mathias

From: Mathias Payer <[email protected]>
Date: Sun, 9 Dec 2018 21:02:25 +0100

The whole kernel is full of situations where an int is returned and if it's negative it's an error. Why is this location so different?

Just check < 0 and be done with it.

OK, whatever you prefer.

I've attached the updated patch. (Greg, please add your Signed-off-by).

Best,
Mathias

commit 78e4a03cd3ae7c8f5957a170bc054fd89d7600f1

Author: Mathias Payer <[email protected]>
Date:   Sun Dec 9 16:45:09 2018 +0100

NET: hso: Fix OOB memory access in hso_probe/hso_get_config_data in hso.c

From: Hui Peng <[email protected]>

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data. Added a length check for both locations and updated hso_probe to bail on error.

This issue has been assigned CVE-2018-19985.

index 184c24baca15..d6916f787fce 100644
@@ -2807,6 +2807,12 @@ static int hso_get_config_data(struct usb_interface *interface)
 		return -EIO;
 	}
 
+	/* check if we have a valid interface */
+	if (if_num > 16) {
+		kfree(config_data);
+		return -EINVAL;
+	}
+
 	switch (config_data[if_num]) {
 	case 0x0:
 		result = 0;

@@ -2877,10 +2883,18 @@ static int hso_probe(struct usb_interface *interface,
 
 	/* Get the interface/port specification from either driver_info or from
 	 * the device itself */
-	if (id->driver_info)
+	if (id->driver_info) {
+		/* if_num is controlled by the device, driver_info is a 0 terminated
+		 * array. Make sure, the access is in bounds! */
+		for (i = 0; i <= if_num; ++i)
+			if (((u32 *)(id->driver_info))[i] == 0)
+				goto exit;
 		port_spec = ((u32 *)(id->driver_info))[if_num];
-	else
+	} else {
 		port_spec = hso_get_config_data(interface);
+		if (port_spec < 0)
+			goto exit;
+	}
 
 	/* Check if we need to switch to alt interfaces prior to port
 	 * configuration */

From: Mathias Payer <[email protected]>
Date: Sun, 9 Dec 2018 21:17:58 +0100

Patches should be posted inline, not as attachments as per process/submitting-patches.rst

Sorry! And thanks for your patience, we're going through a learning experience. Inlined updated patch with the discussed changes:

commit 78e4a03cd3ae7c8f5957a170bc054fd89d7600f1

Author: Mathias Payer <[email protected]>
Date:   Sun Dec 9 16:45:09 2018 +0100

NET: hso: Fix OOB memory access in hso_probe/hso_get_config_data in hso.c

From: Hui Peng <[email protected]>

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data. Added a length check for both locations and updated hso_probe to bail on error.

This issue has been assigned CVE-2018-19985.

result = 0;
@@ -2877,10 +2883,18 @@ static int hso_probe(struct usb_interface *interface,

/* Get the interface/port specification from either driver_info or from
* the device itself */
- if (id->driver_info)
+ if (id->driver_info) {
+ /* if_num is controlled by the device, driver_info is a 0 terminated
+ * array. Make sure, the access is in bounds! */
+ for (i = 0; i <= if_num; ++i)
+ if (((u32 *)(id->driver_info))[i] == 0)
+ goto exit;
port_spec = ((u32 *)(id->driver_info))[if_num];
- else
+ } else {
port_spec = hso_get_config_data(interface);
+ if (port_spec < 0)
+ goto exit;
+ }

/* Check if we need to switch to alt interfaces prior to port
* configuration */

index 184c24baca15..d6916f787fce 100644
@@ -2807,6 +2807,12 @@ static int hso_get_config_data(struct usb_interface

*interface)
return -EIO;
}

+ /* check if we have a valid interface */
+ if (if_num > 16) {
+ kfree(config_data);
+ return -EINVAL;
+ }
+
switch (config_data[if_num]) {
case 0x0:

My fault, I was tired and suggested the other way...

Patches can't be attached, I'll fix this up and resend, thanks.

greg k-h

FYI, this issue has been assigned CVE-2018-19985.

Thanks,
Mathias

From: Hui Peng <[email protected]>

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data. Added a length check for both locations and updated hso_probe to bail on error.

This issue has been assigned CVE-2018-19985.

---
v2: fixed error check to just be < 0
Added CVE to changelog text

 drivers/net/usb/hso.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

index 184c24baca15..d6916f787fce 100644
@@ -2807,6 +2807,12 @@ static int hso_get_config_data(struct usb_interface *interface)
 		return -EIO;
 	}
 
+	/* check if we have a valid interface */
+	if (if_num > 16) {
+		kfree(config_data);
+		return -EINVAL;
+	}
+
 	switch (config_data[if_num]) {
 	case 0x0:
 		result = 0;

@@ -2877,10 +2883,18 @@ static int hso_probe(struct usb_interface *interface,
 
 	/* Get the interface/port specification from either driver_info or from
 	 * the device itself */
-	if (id->driver_info)
+	if (id->driver_info) {
+		/* if_num is controlled by the device, driver_info is a 0 terminated
+		 * array. Make sure, the access is in bounds! */
+		for (i = 0; i <= if_num; ++i)
+			if (((u32 *)(id->driver_info))[i] == 0)
+				goto exit;
 		port_spec = ((u32 *)(id->driver_info))[if_num];
-	else
+	} else {
 		port_spec = hso_get_config_data(interface);
+		if (port_spec < 0)
+			goto exit;
+	}
 
 	/* Check if we need to switch to alt interfaces prior to port
 	 * configuration */

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data.

Add a length check for both locations and update hso_probe to bail on error.

Sebastian

Thanks, will update the changelog and add resend.

greg k-h

From: Hui Peng <[email protected]>

The function hso_probe reads if_num from the USB device (as an u8) and uses it without a length check to index an array, resulting in an OOB memory read in hso_probe or hso_get_config_data.

Add a length check for both locations and updated hso_probe to bail on error.

This issue has been assigned CVE-2018-19985.

---

v3: redid the changelog text based on review comments from Sebastian

v2: fixed error check to just be < 0
Added CVE to changelog text

 drivers/net/usb/hso.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

index 184c24baca15..d6916f787fce 100644
@@ -2807,6 +2807,12 @@ static int hso_get_config_data(struct usb_interface *interface)
 		return -EIO;
 	}
 
+	/* check if we have a valid interface */
+	if (if_num > 16) {
+		kfree(config_data);
+		return -EINVAL;
+	}
+
 	switch (config_data[if_num]) {
 	case 0x0:
 		result = 0;

@@ -2877,10 +2883,18 @@ static int hso_probe(struct usb_interface *interface,
 
 	/* Get the interface/port specification from either driver_info or from
 	 * the device itself */
-	if (id->driver_info)
+	if (id->driver_info) {
+		/* if_num is controlled by the device, driver_info is a 0 terminated
+		 * array. Make sure, the access is in bounds! */
+		for (i = 0; i <= if_num; ++i)
+			if (((u32 *)(id->driver_info))[i] == 0)
+				goto exit;
 		port_spec = ((u32 *)(id->driver_info))[if_num];
-	else
+	} else {
 		port_spec = hso_get_config_data(interface);
+		if (port_spec < 0)
+			goto exit;
+	}
 
 	/* Check if we need to switch to alt interfaces prior to port
 	 * configuration */

From: Greg KH <[email protected]>
Date: Wed, 12 Dec 2018 12:42:24 +0100

Applied and queued up for -stable, thanks Greg.