Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: David Heinzelmann <heinzelmann.david@gmail.com>
Cc: linux-usb@vger.kernel.org
Subject: Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
Date: Mon, 7 Oct 2019 10:01:47 -0400 (EDT)
Message-ID: <Pine.LNX.4.44L0.1910071001230.1513-100000@iolanthe.rowland.org> (raw)
In-Reply-To: <20191007084701.GA9937@dhe-pc>

On Mon, 7 Oct 2019, David Heinzelmann wrote:

> Hi,
> 
> I hope it all fits now.
> 
> David
> 
> 
> From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
> From: David Heinzelmann <heinzelmann.david@gmail.com>
> Date: Fri, 4 Oct 2019 12:28:36 +0200
> Subject: [PATCH v4] usb: hub: Check device descriptor before resusciation
> 
> If a device connected to an xHCI host controller disconnects from the USB bus
> and then reconnects, e.g. triggered by a firmware update, then the host
> controller automatically activates the connection and the port is enabled. The
> implementation of hub_port_connect_change() assumes that if the port is
> enabled then nothing has changed. There is no check if the USB descriptors
> have changed. As a result, the kernel's internal copy of the descriptors ends
> up being incorrect and the device doesn't work properly anymore.
> 
> The solution to the problem is for hub_port_connect_change() always to
> check whether the device's descriptors have changed before resuscitating
> an enabled port.
> 
> Signed-off-by: David Heinzelmann <heinzelmann.david@gmail.com>

Acked-by: Alan Stern <stern@rowland.harvard.edu>


> ---
> Changes in v4:
>  - changed commit description
> Changes in v3:
>  - changed commit message and description
>  - fix code style
> Changes in v2:
>  - fix logic error to handle return code from usb_get_device_descriptor()
>    properly
>  - fix line endings
> ---
>  drivers/usb/core/hub.c | 196 +++++++++++++++++++++++------------------
>  1 file changed, 111 insertions(+), 85 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 236313f41f4a..fdcfa85b5b12 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4930,6 +4930,91 @@ hub_power_remaining(struct usb_hub *hub)
>  	return remaining;
>  }
>  
> +
> +static int descriptors_changed(struct usb_device *udev,
> +		struct usb_device_descriptor *old_device_descriptor,
> +		struct usb_host_bos *old_bos)
> +{
> +	int		changed = 0;
> +	unsigned	index;
> +	unsigned	serial_len = 0;
> +	unsigned	len;
> +	unsigned	old_length;
> +	int		length;
> +	char		*buf;
> +
> +	if (memcmp(&udev->descriptor, old_device_descriptor,
> +			sizeof(*old_device_descriptor)) != 0)
> +		return 1;
> +
> +	if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
> +		return 1;
> +	if (udev->bos) {
> +		len = le16_to_cpu(udev->bos->desc->wTotalLength);
> +		if (len != le16_to_cpu(old_bos->desc->wTotalLength))
> +			return 1;
> +		if (memcmp(udev->bos->desc, old_bos->desc, len))
> +			return 1;
> +	}
> +
> +	/* Since the idVendor, idProduct, and bcdDevice values in the
> +	 * device descriptor haven't changed, we will assume the
> +	 * Manufacturer and Product strings haven't changed either.
> +	 * But the SerialNumber string could be different (e.g., a
> +	 * different flash card of the same brand).
> +	 */
> +	if (udev->serial)
> +		serial_len = strlen(udev->serial) + 1;
> +
> +	len = serial_len;
> +	for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> +		old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> +		len = max(len, old_length);
> +	}
> +
> +	buf = kmalloc(len, GFP_NOIO);
> +	if (!buf)
> +		/* assume the worst */
> +		return 1;
> +
> +	for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> +		old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> +		length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
> +				old_length);
> +		if (length != old_length) {
> +			dev_dbg(&udev->dev, "config index %d, error %d\n",
> +					index, length);
> +			changed = 1;
> +			break;
> +		}
> +		if (memcmp(buf, udev->rawdescriptors[index], old_length)
> +				!= 0) {
> +			dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
> +				index,
> +				((struct usb_config_descriptor *) buf)->
> +					bConfigurationValue);
> +			changed = 1;
> +			break;
> +		}
> +	}
> +
> +	if (!changed && serial_len) {
> +		length = usb_string(udev, udev->descriptor.iSerialNumber,
> +				buf, serial_len);
> +		if (length + 1 != serial_len) {
> +			dev_dbg(&udev->dev, "serial string error %d\n",
> +					length);
> +			changed = 1;
> +		} else if (memcmp(buf, udev->serial, length) != 0) {
> +			dev_dbg(&udev->dev, "serial string changed\n");
> +			changed = 1;
> +		}
> +	}
> +
> +	kfree(buf);
> +	return changed;
> +}
> +
>  static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
>  		u16 portchange)
>  {
> @@ -5167,7 +5252,9 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
>  {
>  	struct usb_port *port_dev = hub->ports[port1 - 1];
>  	struct usb_device *udev = port_dev->child;
> +	struct usb_device_descriptor descriptor;
>  	int status = -ENODEV;
> +	int retval;
>  
>  	dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus,
>  			portchange, portspeed(hub, portstatus));
> @@ -5188,7 +5275,30 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
>  	if ((portstatus & USB_PORT_STAT_CONNECTION) && udev &&
>  			udev->state != USB_STATE_NOTATTACHED) {
>  		if (portstatus & USB_PORT_STAT_ENABLE) {
> -			status = 0;		/* Nothing to do */
> +			/*
> +			 * USB-3 connections are initialized automatically by
> +			 * the hostcontroller hardware. Therefore check for
> +			 * changed device descriptors before resuscitating the
> +			 * device.
> +			 */
> +			descriptor = udev->descriptor;
> +			retval = usb_get_device_descriptor(udev,
> +					sizeof(udev->descriptor));
> +			if (retval < 0) {
> +				dev_dbg(&udev->dev,
> +						"can't read device descriptor %d\n",
> +						retval);
> +			} else {
> +				if (descriptors_changed(udev, &descriptor,
> +						udev->bos)) {
> +					dev_dbg(&udev->dev,
> +							"device descriptor has changed\n");
> +					/* for disconnect() calls */
> +					udev->descriptor = descriptor;
> +				} else {
> +					status = 0; /* Nothing to do */
> +				}
> +			}
>  #ifdef CONFIG_PM
>  		} else if (udev->state == USB_STATE_SUSPENDED &&
>  				udev->persist_enabled) {
> @@ -5550,90 +5660,6 @@ void usb_hub_cleanup(void)
>  	usb_deregister(&hub_driver);
>  } /* usb_hub_cleanup() */
>  
> -static int descriptors_changed(struct usb_device *udev,
> -		struct usb_device_descriptor *old_device_descriptor,
> -		struct usb_host_bos *old_bos)
> -{
> -	int		changed = 0;
> -	unsigned	index;
> -	unsigned	serial_len = 0;
> -	unsigned	len;
> -	unsigned	old_length;
> -	int		length;
> -	char		*buf;
> -
> -	if (memcmp(&udev->descriptor, old_device_descriptor,
> -			sizeof(*old_device_descriptor)) != 0)
> -		return 1;
> -
> -	if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
> -		return 1;
> -	if (udev->bos) {
> -		len = le16_to_cpu(udev->bos->desc->wTotalLength);
> -		if (len != le16_to_cpu(old_bos->desc->wTotalLength))
> -			return 1;
> -		if (memcmp(udev->bos->desc, old_bos->desc, len))
> -			return 1;
> -	}
> -
> -	/* Since the idVendor, idProduct, and bcdDevice values in the
> -	 * device descriptor haven't changed, we will assume the
> -	 * Manufacturer and Product strings haven't changed either.
> -	 * But the SerialNumber string could be different (e.g., a
> -	 * different flash card of the same brand).
> -	 */
> -	if (udev->serial)
> -		serial_len = strlen(udev->serial) + 1;
> -
> -	len = serial_len;
> -	for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> -		old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> -		len = max(len, old_length);
> -	}
> -
> -	buf = kmalloc(len, GFP_NOIO);
> -	if (!buf)
> -		/* assume the worst */
> -		return 1;
> -
> -	for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> -		old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> -		length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
> -				old_length);
> -		if (length != old_length) {
> -			dev_dbg(&udev->dev, "config index %d, error %d\n",
> -					index, length);
> -			changed = 1;
> -			break;
> -		}
> -		if (memcmp(buf, udev->rawdescriptors[index], old_length)
> -				!= 0) {
> -			dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
> -				index,
> -				((struct usb_config_descriptor *) buf)->
> -					bConfigurationValue);
> -			changed = 1;
> -			break;
> -		}
> -	}
> -
> -	if (!changed && serial_len) {
> -		length = usb_string(udev, udev->descriptor.iSerialNumber,
> -				buf, serial_len);
> -		if (length + 1 != serial_len) {
> -			dev_dbg(&udev->dev, "serial string error %d\n",
> -					length);
> -			changed = 1;
> -		} else if (memcmp(buf, udev->serial, length) != 0) {
> -			dev_dbg(&udev->dev, "serial string changed\n");
> -			changed = 1;
> -		}
> -	}
> -
> -	kfree(buf);
> -	return changed;
> -}
> -
>  /**
>   * usb_reset_and_verify_device - perform a USB port reset to reinitialize a device
>   * @udev: device to reset (not in SUSPENDED or NOTATTACHED state)
> 


  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-20 10:36 David Heinzelmann
2019-09-20  8:55 ` Greg KH
2019-09-20 13:17   ` David Heinzelmann
2019-09-20 12:15     ` Greg KH
2019-09-20 15:33       ` David Heinzelmann
2019-09-23 14:49         ` Alan Stern
2019-09-24 10:01           ` David Heinzelmann
2019-09-25 14:20             ` Alan Stern
2019-09-30  7:26               ` David Heinzelmann
2019-09-30 14:25                 ` Alan Stern
2019-10-04 13:23                   ` David Heinzelmann
2019-10-04 14:17                     ` Alan Stern
2019-10-07  8:47                       ` David Heinzelmann
2019-10-07 14:01                         ` Alan Stern [this message]
2019-10-07 15:35                           ` Greg KH
2019-10-08  8:09                             ` [PATCH v4] usb: hub: Check device descriptor before resusciation David Heinzelmann
2019-10-08 12:55                               ` Greg KH
2019-10-08 16:10                                 ` David Heinzelmann
2019-10-08 15:17                                   ` Greg KH
2019-10-09  4:46                             ` [PATCH v5] " David Heinzelmann

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.44L0.1910071001230.1513-100000@iolanthe.rowland.org \
    --to=stern@rowland.harvard.edu \
    --cc=heinzelmann.david@gmail.com \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git