[PATCH 3/6] USB: hub: fix SS hub-descriptor handling

[PATCH 3/6] USB: hub: fix SS hub-descriptor handling

[Johan Hovold]

A SuperSpeed hub descriptor does not have any variable-length fields so
bail out when reading a short descriptor.

This avoids parsing and leaking two bytes of uninitialised slab data
through sysfs removable-attributes.

Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes")
Cc: stable <stable@vger.kernel.org>     # 2.6.39
Cc: John Youn <John.Youn@synopsys.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/hub.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 9dca59ef18b3..3ff1e9f89f2d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
 			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
 			dtype << 8, 0, data, size,
 			USB_CTRL_GET_TIMEOUT);
-		if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2))
+		if (hub_is_superspeed(hdev)) {
+			if (ret == size)
+				return ret;
+		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
 			return ret;
+		}
 	}
 	return -EINVAL;
 }
@@ -1321,7 +1325,7 @@ static int hub_configure(struct usb_hub *hub,
 
 	/* Request the entire hub descriptor.
 	 * hub->descriptor can handle USB_MAXCHILDREN ports,
-	 * but the hub can/will return fewer bytes here.
+	 * but a (non-SS) hub can/will return fewer bytes here.
 	 */
 	ret = get_hub_descriptor(hdev, hub->descriptor);
 	if (ret < 0) {
-- 
2.13.0

[PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Johan Hovold]

Add missing sanity check on the non-SuperSpeed hub-descriptor length in
order to avoid parsing and leaking two bytes of uninitialised slab data
through sysfs removable-attributes (or a compound-device debug
statement).

Note that we only make sure that the DeviceRemovable field is always
present (and specifically ignore the unused PortPwrCtrlMask field) in
order to continue support any hubs with non-compliant descriptors. As a
further safeguard, the descriptor buffer is also cleared.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable <stable@vger.kernel.org>     # 2.6.12
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/core/hub.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3ff1e9f89f2d..f77a4ebde7d5 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev)
 }
 
 /* USB 2.0 spec Section 11.24.4.5 */
-static int get_hub_descriptor(struct usb_device *hdev, void *data)
+static int get_hub_descriptor(struct usb_device *hdev,
+		struct usb_hub_descriptor *desc)
 {
 	int i, ret, size;
 	unsigned dtype;
@@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
 	for (i = 0; i < 3; i++) {
 		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
 			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
-			dtype << 8, 0, data, size,
+			dtype << 8, 0, desc, size,
 			USB_CTRL_GET_TIMEOUT);
 		if (hub_is_superspeed(hdev)) {
 			if (ret == size)
 				return ret;
-		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
+		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
+			/* Make sure we have the DeviceRemovable field. */
+			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
+			if (ret < size)
+				return -EMSGSIZE;
 			return ret;
 		}
 	}
@@ -1317,7 +1322,7 @@ static int hub_configure(struct usb_hub *hub,
 	}
 	mutex_init(&hub->status_mutex);
 
-	hub->descriptor = kmalloc(sizeof(*hub->descriptor), GFP_KERNEL);
+	hub->descriptor = kzalloc(sizeof(*hub->descriptor), GFP_KERNEL);
 	if (!hub->descriptor) {
 		ret = -ENOMEM;
 		goto fail;
-- 
2.13.0

Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling

[Alan Stern]

On Wed, 10 May 2017, Johan Hovold wrote:

> A SuperSpeed hub descriptor does not have any variable-length fields so
> bail out when reading a short descriptor.

You mean: bail out when reading a descriptor that is not exactly the
right length.  The existing code already bails out when it reads a
short descriptor.

Alan Stern

> This avoids parsing and leaking two bytes of uninitialised slab data
> through sysfs removable-attributes.
> 
> Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes")
> Cc: stable <stable@vger.kernel.org>     # 2.6.39
> Cc: John Youn <John.Youn@synopsys.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/core/hub.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 9dca59ef18b3..3ff1e9f89f2d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
>  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
>  			dtype << 8, 0, data, size,
>  			USB_CTRL_GET_TIMEOUT);
> -		if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2))
> +		if (hub_is_superspeed(hdev)) {
> +			if (ret == size)
> +				return ret;
> +		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
>  			return ret;
> +		}
>  	}
>  	return -EINVAL;
>  }
> @@ -1321,7 +1325,7 @@ static int hub_configure(struct usb_hub *hub,
>  
>  	/* Request the entire hub descriptor.
>  	 * hub->descriptor can handle USB_MAXCHILDREN ports,
> -	 * but the hub can/will return fewer bytes here.
> +	 * but a (non-SS) hub can/will return fewer bytes here.
>  	 */
>  	ret = get_hub_descriptor(hdev, hub->descriptor);
>  	if (ret < 0) {
> 

Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Alan Stern]

On Wed, 10 May 2017, Johan Hovold wrote:

> Add missing sanity check on the non-SuperSpeed hub-descriptor length in
> order to avoid parsing and leaking two bytes of uninitialised slab data
> through sysfs removable-attributes (or a compound-device debug
> statement).
> 
> Note that we only make sure that the DeviceRemovable field is always
> present (and specifically ignore the unused PortPwrCtrlMask field) in
> order to continue support any hubs with non-compliant descriptors. As a
> further safeguard, the descriptor buffer is also cleared.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable <stable@vger.kernel.org>     # 2.6.12
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/core/hub.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 3ff1e9f89f2d..f77a4ebde7d5 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev)
>  }
>  
>  /* USB 2.0 spec Section 11.24.4.5 */
> -static int get_hub_descriptor(struct usb_device *hdev, void *data)
> +static int get_hub_descriptor(struct usb_device *hdev,
> +		struct usb_hub_descriptor *desc)
>  {
>  	int i, ret, size;
>  	unsigned dtype;
> @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
>  	for (i = 0; i < 3; i++) {
>  		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
>  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> -			dtype << 8, 0, data, size,
> +			dtype << 8, 0, desc, size,
>  			USB_CTRL_GET_TIMEOUT);
>  		if (hub_is_superspeed(hdev)) {
>  			if (ret == size)
>  				return ret;
> -		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> +		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
> +			/* Make sure we have the DeviceRemovable field. */
> +			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
> +			if (ret < size)
> +				return -EMSGSIZE;

The logic could be simplified a little.  Since we don't really care 
about the return code when an error occurs, you could just do:

		} else if (ret >= USB_DT_HUB_NONVAR_SIZE +
				desc->bNbrPorts / 8 + 1) {
			/* We have the entire DeviceRemovable field. */
 			return ret;
 		}

Alan Stern

Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling

[Johan Hovold]

On Wed, May 10, 2017 at 10:04:32AM -0400, Alan Stern wrote:
> On Wed, 10 May 2017, Johan Hovold wrote:
> 
> > A SuperSpeed hub descriptor does not have any variable-length fields so
> > bail out when reading a short descriptor.
> 
> You mean: bail out when reading a descriptor that is not exactly the
> right length.  The existing code already bails out when it reads a
> short descriptor.

No, the current code happily accepts a 9-byte descriptor, while an SS
descriptor is always 12 bytes. And since we request 12 bytes for SS
hubs, the patch description is correct.

> > This avoids parsing and leaking two bytes of uninitialised slab data
> > through sysfs removable-attributes.
> > 
> > Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes")
> > Cc: stable <stable@vger.kernel.org>     # 2.6.39
> > Cc: John Youn <John.Youn@synopsys.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/core/hub.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 9dca59ef18b3..3ff1e9f89f2d 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> >  			dtype << 8, 0, data, size,
> >  			USB_CTRL_GET_TIMEOUT);
> > -		if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2))
> > +		if (hub_is_superspeed(hdev)) {
> > +			if (ret == size)
> > +				return ret;
> > +		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> >  			return ret;
> > +		}
> >  	}
> >  	return -EINVAL;
> >  }

Thanks,
Johan

Re: [PATCH 3/6] USB: hub: fix SS hub-descriptor handling

[Alan Stern]

On Wed, 10 May 2017, Johan Hovold wrote:

> On Wed, May 10, 2017 at 10:04:32AM -0400, Alan Stern wrote:
> > On Wed, 10 May 2017, Johan Hovold wrote:
> > 
> > > A SuperSpeed hub descriptor does not have any variable-length fields so
> > > bail out when reading a short descriptor.
> > 
> > You mean: bail out when reading a descriptor that is not exactly the
> > right length.  The existing code already bails out when it reads a
> > short descriptor.
> 
> No, the current code happily accepts a 9-byte descriptor, while an SS
> descriptor is always 12 bytes. And since we request 12 bytes for SS
> hubs, the patch description is correct.

My mistake; you are right.

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

> 
> > > This avoids parsing and leaking two bytes of uninitialised slab data
> > > through sysfs removable-attributes.
> > > 
> > > Fixes: dbe79bbe9dcb ("USB 3.0 Hub Changes")
> > > Cc: stable <stable@vger.kernel.org>     # 2.6.39
> > > Cc: John Youn <John.Youn@synopsys.com>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > >  drivers/usb/core/hub.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > > index 9dca59ef18b3..3ff1e9f89f2d 100644
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -380,8 +380,12 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> > >  			dtype << 8, 0, data, size,
> > >  			USB_CTRL_GET_TIMEOUT);
> > > -		if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2))
> > > +		if (hub_is_superspeed(hdev)) {
> > > +			if (ret == size)
> > > +				return ret;
> > > +		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> > >  			return ret;
> > > +		}
> > >  	}
> > >  	return -EINVAL;
> > >  }
> 
> Thanks,
> Johan
> 
> 

Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Johan Hovold]

On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote:
> On Wed, 10 May 2017, Johan Hovold wrote:
> 
> > Add missing sanity check on the non-SuperSpeed hub-descriptor length in
> > order to avoid parsing and leaking two bytes of uninitialised slab data
> > through sysfs removable-attributes (or a compound-device debug
> > statement).
> > 
> > Note that we only make sure that the DeviceRemovable field is always
> > present (and specifically ignore the unused PortPwrCtrlMask field) in
> > order to continue support any hubs with non-compliant descriptors. As a
> > further safeguard, the descriptor buffer is also cleared.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable <stable@vger.kernel.org>     # 2.6.12
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/core/hub.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index 3ff1e9f89f2d..f77a4ebde7d5 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -362,7 +362,8 @@ static void usb_set_lpm_parameters(struct usb_device *udev)
> >  }
> >  
> >  /* USB 2.0 spec Section 11.24.4.5 */
> > -static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > +static int get_hub_descriptor(struct usb_device *hdev,
> > +		struct usb_hub_descriptor *desc)
> >  {
> >  	int i, ret, size;
> >  	unsigned dtype;
> > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> >  	for (i = 0; i < 3; i++) {
> >  		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> > -			dtype << 8, 0, data, size,
> > +			dtype << 8, 0, desc, size,
> >  			USB_CTRL_GET_TIMEOUT);
> >  		if (hub_is_superspeed(hdev)) {
> >  			if (ret == size)
> >  				return ret;
> > -		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> > +		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
> > +			/* Make sure we have the DeviceRemovable field. */
> > +			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
> > +			if (ret < size)
> > +				return -EMSGSIZE;
> 
> The logic could be simplified a little.  Since we don't really care 
> about the return code when an error occurs, you could just do:
> 
> 		} else if (ret >= USB_DT_HUB_NONVAR_SIZE +
> 				desc->bNbrPorts / 8 + 1) {
> 			/* We have the entire DeviceRemovable field. */
>  			return ret;
>  		}

Sure, that would work, but I it doesn't feel right to access bNbrPorts
without first verifying we got the non-variable fields.

I considered dropping the +2 bit, but decided to keep it in the unlikely
even that there are quirky devices out there that rely on it (e.g. first
read always return 7 bytes). Spelling it out makes it sound overly
conservative though. How about I drop that instead?

Thanks,
Johan

Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Johan Hovold]

On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote:
> On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote:
> > On Wed, 10 May 2017, Johan Hovold wrote:

> > >  /* USB 2.0 spec Section 11.24.4.5 */
> > > -static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > > +static int get_hub_descriptor(struct usb_device *hdev,
> > > +		struct usb_hub_descriptor *desc)
> > >  {
> > >  	int i, ret, size;
> > >  	unsigned dtype;
> > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > >  	for (i = 0; i < 3; i++) {
> > >  		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> > >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> > > -			dtype << 8, 0, data, size,
> > > +			dtype << 8, 0, desc, size,
> > >  			USB_CTRL_GET_TIMEOUT);
> > >  		if (hub_is_superspeed(hdev)) {
> > >  			if (ret == size)
> > >  				return ret;
> > > -		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> > > +		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
> > > +			/* Make sure we have the DeviceRemovable field. */
> > > +			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
> > > +			if (ret < size)
> > > +				return -EMSGSIZE;
> > 
> > The logic could be simplified a little.  Since we don't really care 
> > about the return code when an error occurs, you could just do:
> > 
> > 		} else if (ret >= USB_DT_HUB_NONVAR_SIZE +
> > 				desc->bNbrPorts / 8 + 1) {
> > 			/* We have the entire DeviceRemovable field. */
> >  			return ret;
> >  		}
> 
> Sure, that would work, but I it doesn't feel right to access bNbrPorts
> without first verifying we got the non-variable fields.
> 
> I considered dropping the +2 bit, but decided to keep it in the unlikely
> even that there are quirky devices out there that rely on it (e.g. first
> read always return 7 bytes). Spelling it out makes it sound overly
> conservative though. How about I drop that instead?

Then again, a non-SS hub descriptor is always at least
(USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes
sense anyway.

Johan

Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Alan Stern]

On Wed, 10 May 2017, Johan Hovold wrote:

> On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote:
> > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote:
> > > On Wed, 10 May 2017, Johan Hovold wrote:
> 
> > > >  /* USB 2.0 spec Section 11.24.4.5 */
> > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > > > +static int get_hub_descriptor(struct usb_device *hdev,
> > > > +		struct usb_hub_descriptor *desc)
> > > >  {
> > > >  	int i, ret, size;
> > > >  	unsigned dtype;
> > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > > >  	for (i = 0; i < 3; i++) {
> > > >  		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> > > >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> > > > -			dtype << 8, 0, data, size,
> > > > +			dtype << 8, 0, desc, size,
> > > >  			USB_CTRL_GET_TIMEOUT);
> > > >  		if (hub_is_superspeed(hdev)) {
> > > >  			if (ret == size)
> > > >  				return ret;
> > > > -		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> > > > +		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
> > > > +			/* Make sure we have the DeviceRemovable field. */
> > > > +			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
> > > > +			if (ret < size)
> > > > +				return -EMSGSIZE;
> > > 
> > > The logic could be simplified a little.  Since we don't really care 
> > > about the return code when an error occurs, you could just do:
> > > 
> > > 		} else if (ret >= USB_DT_HUB_NONVAR_SIZE +
> > > 				desc->bNbrPorts / 8 + 1) {
> > > 			/* We have the entire DeviceRemovable field. */
> > >  			return ret;
> > >  		}
> > 
> > Sure, that would work, but I it doesn't feel right to access bNbrPorts
> > without first verifying we got the non-variable fields.

Accessing desc->bNbrPorts won't do any harm, even if it wasn't sent.  
After all, you now initialize the hub descriptor to all 0's.  And even 
if the field contained garbage, it would just make this test less 
likely to succeed.

> > I considered dropping the +2 bit, but decided to keep it in the unlikely
> > even that there are quirky devices out there that rely on it (e.g. first
> > read always return 7 bytes). Spelling it out makes it sound overly
> > conservative though. How about I drop that instead?
> 
> Then again, a non-SS hub descriptor is always at least
> (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes
> sense anyway.

Personally, I wouldn't worry about it.  It's your decision.

Alan Stern

Re: [PATCH 4/6] USB: hub: fix non-SS hub-descriptor handling

[Johan Hovold]

On Wed, May 10, 2017 at 11:11:37AM -0400, Alan Stern wrote:
> On Wed, 10 May 2017, Johan Hovold wrote:
> 
> > On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote:
> > > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote:
> > > > On Wed, 10 May 2017, Johan Hovold wrote:
> > 
> > > > >  /* USB 2.0 spec Section 11.24.4.5 */
> > > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > > > > +static int get_hub_descriptor(struct usb_device *hdev,
> > > > > +		struct usb_hub_descriptor *desc)
> > > > >  {
> > > > >  	int i, ret, size;
> > > > >  	unsigned dtype;
> > > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data)
> > > > >  	for (i = 0; i < 3; i++) {
> > > > >  		ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0),
> > > > >  			USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB,
> > > > > -			dtype << 8, 0, data, size,
> > > > > +			dtype << 8, 0, desc, size,
> > > > >  			USB_CTRL_GET_TIMEOUT);
> > > > >  		if (hub_is_superspeed(hdev)) {
> > > > >  			if (ret == size)
> > > > >  				return ret;
> > > > > -		} else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) {
> > > > > +		} else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) {
> > > > > +			/* Make sure we have the DeviceRemovable field. */
> > > > > +			size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1;
> > > > > +			if (ret < size)
> > > > > +				return -EMSGSIZE;
> > > > 
> > > > The logic could be simplified a little.  Since we don't really care 
> > > > about the return code when an error occurs, you could just do:
> > > > 
> > > > 		} else if (ret >= USB_DT_HUB_NONVAR_SIZE +
> > > > 				desc->bNbrPorts / 8 + 1) {
> > > > 			/* We have the entire DeviceRemovable field. */
> > > >  			return ret;
> > > >  		}
> > > 
> > > Sure, that would work, but I it doesn't feel right to access bNbrPorts
> > > without first verifying we got the non-variable fields.
> 
> Accessing desc->bNbrPorts won't do any harm, even if it wasn't sent.  
> After all, you now initialize the hub descriptor to all 0's.  And even 
> if the field contained garbage, it would just make this test less 
> likely to succeed.

I know, but since it's not immediately obvious (and may set a bad
example), I think we should avoid it.

> > > I considered dropping the +2 bit, but decided to keep it in the unlikely
> > > even that there are quirky devices out there that rely on it (e.g. first
> > > read always return 7 bytes). Spelling it out makes it sound overly
> > > conservative though. How about I drop that instead?
> > 
> > Then again, a non-SS hub descriptor is always at least
> > (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes
> > sense anyway.
> 
> Personally, I wouldn't worry about it.  It's your decision.

Now I remember that dropping the +2 would also mean that we start
accepting descriptors without a PortPwrCtrlMask field. I'll just leave
it in for now.

Thanks,
Johan