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

[Johan Hovold <johan@kernel.org>]

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