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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot