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
