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

[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).

> 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. So for consistency (and safety in case of
future expansion) I went with the macros in all cases.

> 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.

> 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.

>>                               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.
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.

>> -     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).