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

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

[Greg Kroah-Hartman]

From: Hui Peng <benquike@gmail.com>

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.

Reported-by: Hui Peng <benquike@gmail.com>
Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
Signed-off-by: Hui Peng <benquike@gmail.com>
Signed-off-by: Mathias Payer <mathias.payer@nebelwelt.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/usb/hso.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 184c24baca15..168f9081d4ea 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -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 */

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

[David Miller]

From: Greg KH <gregkh@linuxfoundation.org>
Date: Sun, 9 Dec 2018 17:32:45 +0100

> +	} else {
>  		port_spec = hso_get_config_data(interface);
> +		if (IS_ERR_VALUE((long)port_spec))
> +			goto exit;

'port_spec' is an 'int', it makes no sense to cast it 3 times all the
way back to 'int' to figure out if it is a negative error value or
not. (--> long --> void * --> int)

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

[Mathias Payer]

Hi David,

>> +	} else {
>>  		port_spec = hso_get_config_data(interface);
>> +		if (IS_ERR_VALUE((long)port_spec))
>> +			goto exit;
> 
> 'port_spec' is an 'int', it makes no sense to cast it 3 times all the
> way back to 'int' to figure out if it is a negative error value or
> not. (--> long --> void * --> int)
> 

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

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

[Mathias Payer]

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

Thanks,
Mathias

On 12/9/18 5:32 PM, Greg KH wrote:
> From: Hui Peng <benquike@gmail.com>
> 
> 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.
> 
> Reported-by: Hui Peng <benquike@gmail.com>
> Reported-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Hui Peng <benquike@gmail.com>
> Signed-off-by: Mathias Payer <mathias.payer@nebelwelt.net>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/net/usb/hso.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
> index 184c24baca15..168f9081d4ea 100644
> --- a/drivers/net/usb/hso.c
> +++ b/drivers/net/usb/hso.c
> @@ -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 */
>

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

[David Miller]

From: Mathias Payer <mathias.payer@nebelwelt.net>
Date: Sun, 9 Dec 2018 21:02:25 +0100

> 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.

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.