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

[Johan Hovold <johan@kernel.org>]

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