Re: usb/serial/visor: slab-out-of-bounds in palm_os_3_probe

[Johan Hovold]

On Fri, Sep 29, 2017 at 10:37:55AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 28, 2017 at 07:57:46PM +0200, Andrey Konovalov wrote:
> > Hi!
> > 
> > I've got the following report while fuzzing the kernel with syzkaller.
> > 
> > On commit dc972a67cc54585bd83ad811c4e9b6ab3dcd427e (4.14-rc2+).
> > 
> > There's no check on the connection_info->num_ports value when
> > iterating over ports.
> > 
> > usb 1-1: Handspring Visor / Palm OS: port 162, is for unknown use
> > usb 1-1: Handspring Visor / Palm OS: port 81, is for unknown use
> > ==================================================================
> > BUG: KASAN: slab-out-of-bounds in palm_os_3_probe+0x4e4/0x570
> > Read of size 1 at addr ffff8800686daa26 by task kworker/0:1/24

Thanks for the report, Andrey.

> Ah, nice catch, this bug is _old_, sorry about that.
> 
> The patch below should resolve this.  It looks bigger than it really is,
> as I'm just moving the error checking higher up in the function, and
> loosing an indentation for when there is invalid data.
> 
> Can you let me know if this solves the issue?

And thanks for fixing this up, Greg. Will you send a proper patch that I
can apply?

A couple of comments below.

> diff --git a/drivers/usb/serial/visor.c b/drivers/usb/serial/visor.c
> index 9f3317a940ef..18c31bae0e10 100644
> --- a/drivers/usb/serial/visor.c
> +++ b/drivers/usb/serial/visor.c
> @@ -338,47 +338,48 @@ static int palm_os_3_probe(struct usb_serial *serial,
>  		goto exit;
>  	}
>  
> -	if (retval == sizeof(*connection_info)) {
> -			connection_info = (struct visor_connection_info *)
> -							transfer_buffer;
> -
> -		num_ports = le16_to_cpu(connection_info->num_ports);
> -		for (i = 0; i < num_ports; ++i) {
> -			switch (
> -			   connection_info->connections[i].port_function_id) {
> -			case VISOR_FUNCTION_GENERIC:
> -				string = "Generic";
> -				break;
> -			case VISOR_FUNCTION_DEBUGGER:
> -				string = "Debugger";
> -				break;
> -			case VISOR_FUNCTION_HOTSYNC:
> -				string = "HotSync";
> -				break;
> -			case VISOR_FUNCTION_CONSOLE:
> -				string = "Console";
> -				break;
> -			case VISOR_FUNCTION_REMOTE_FILE_SYS:
> -				string = "Remote File System";
> -				break;
> -			default:
> -				string = "unknown";
> -				break;
> -			}
> -			dev_info(dev, "%s: port %d, is for %s use\n",
> -				serial->type->description,
> -				connection_info->connections[i].port, string);
> -		}
> +	if (retval != sizeof(*connection_info)) {
> +		retval = -EINVAL;

Since we're refusing to bind here -ENODEV may be more appropriate. And
I think a dev_err() (or dev_warn()) is warranted too.

> +		goto exit;

So far we have not been bailing probe when the returned connection-info size
wasn't the expected one (we'd only skip the dev_info() above). I'm sure
this is fine, but the tightened check could potentially cause issues if
there are devices returning additional fields.

>  	}
> -	/*
> -	* Handle devices that report invalid stuff here.
> -	*/
> +
> +	connection_info = (struct visor_connection_info *)transfer_buffer;
> +
> +	num_ports = le16_to_cpu(connection_info->num_ports);
> +
> +	/* Handle devices that report invalid stuff here. */
>  	if (num_ports == 0 || num_ports > 2) {
>  		dev_warn(dev, "%s: No valid connect info available\n",
>  			serial->type->description);
>  		num_ports = 2;
>  	}
>  
> +	for (i = 0; i < num_ports; ++i) {
> +		switch (
> +		   connection_info->connections[i].port_function_id) {

Perhaps you can merge the above two lines now while at it.

> +		case VISOR_FUNCTION_GENERIC:
> +			string = "Generic";
> +			break;
> +		case VISOR_FUNCTION_DEBUGGER:
> +			string = "Debugger";
> +			break;
> +		case VISOR_FUNCTION_HOTSYNC:
> +			string = "HotSync";
> +			break;
> +		case VISOR_FUNCTION_CONSOLE:
> +			string = "Console";
> +			break;
> +		case VISOR_FUNCTION_REMOTE_FILE_SYS:
> +			string = "Remote File System";
> +			break;
> +		default:
> +			string = "unknown";
> +			break;
> +		}
> +		dev_info(dev, "%s: port %d, is for %s use\n",
> +			serial->type->description,
> +			connection_info->connections[i].port, string);
> +	}
>  	dev_info(dev, "%s: Number of ports: %d\n", serial->type->description,
>  		num_ports);
>  

Thanks,
Johan