[U-Boot] [PATCH v2] usb: Use well-known descriptor sizes when parsing configuration

[Marek Vasut <marex@denx.de>]

Dear Julius Werner,

> > Mulling over this some more, I suspect if the device does have incorrect
> > config descriptor, we should just ignore the device because it's broken
> > piece of junk.
> 
> I can change it if you insist, but I'd like to keep it to make the
> code look more consistent (since later on with the interface/endpoint
> descriptors, I think ignoring errors makes more sense).

How would that make the code more consistent ? It seems if the device can not 
even provide valid config ep descriptor, the device is broken beyond salvation.

> > Looking at this, the memcpy is incorrect in the first place. It shouldn't
> > memcpy into dev->config, but into dev->config.desc . And in turn, you an
> > do memcpy(&dev->config.desc, buffer, sizeof(dev->config.desc));
> > 
> > Which is even better, since you always take into account the size of the
> > structure member. This would be worth fixing the right way.
> 
> The sizeof() thing is true for the configuration descriptor, but not
> for some others (e.g. endpoint) because U-Boot reserves fields for
> it's own stuff behind that.

Urgh, then the structure defining the descriptor shall be separated out.

> So for consistency (and safety in case of
> future expansion) I went with the macros in all cases.

Dang.

> > Urgh, who the heck wrote this code. Damn that's a mess. Anyway, I suspect
> > this might still explode if we go over USB_BUFSIZ in wTotalLength .
> > Worth fixing.
> 
> usb_get_configuration_no() now makes sure we don't read more than
> USB_BUFSIZ and stores the actually read amount in wTotalLength so we
> can use it without worrying here.

OK

> > Would be nice to clean this up into "understandable" format by defining a
> > variable for the &buffer[index] and than just simply comparing this var-
> > 
> >>bInterfaceNumber and curr_if_num .
> 
> Agreed, but let's clean this up one patch at a time.

Would you do a series on this maybe?

> >>                               if_desc = &dev->config.if_desc[ifno];
> >>                               dev->config.no_of_if++;
> >> 
> >> -                             memcpy(if_desc, &buffer[index],
> >> buffer[index]); +                             if (buffer[index] !=
> >> USB_DT_INTERFACE_SIZE) +                                    
> >> printf("Ignoring invalid USB IF length" +                              
> >>               " (%d)\n", buffer[index]);
> > 
> > Let's just ignore the descriptor if it's incorrect.
> 
> Jokes about Chinese crap aside, I would try to err on the side of
> making it work if in any way possible, as long as the code stays safe.

But obviously the descriptor is broken.

> This is firmware so we often can't do much error recovery... either we
> can work with what we have, or we won't boot. Although I don't think
> these cases will happen in practice.

So, let's just ignore broken descriptors.

> >> -     result = usb_get_descriptor(dev, USB_DT_CONFIG, cfgno, buffer,
> >> tmp); -     debug("get_conf_no %d Result %d, wLength %d\n", cfgno,
> >> result, tmp); +     result = usb_get_descriptor(dev, USB_DT_CONFIG,
> >> cfgno, buffer, length); +     debug("get_conf_no %d Result %d, wLength
> >> %d\n", cfgno, result, length); +     config->wTotalLength = length; /*
> >> validated, with CPU byte order */
> > 
> > The above assignment is new, why ?
> 
> This is the sanitization of wTotalLength I mentioned above. Maybe not
> the most obvious way to do it, but it's convenient since you have to
> pass the actually read length from here to usb_parse_config() somehow
> (to avoid things like reading into the leftover descriptors of a
> previously enumerated device).

Document this properly then.

Best regards,
Marek Vasut