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