Message ID | 1374108919-32300-1-git-send-email-jwerner@chromium.org |
---|---|
State | Superseded |
Delegated to: | Marek Vasut |
Headers | show |
Hi Julius, On Wed, 17 Jul 2013 17:55:19 -0700, Julius Werner <jwerner@chromium.org> wrote: > The existing USB configuration parsing code relies on the descriptors' > own length values when reading through the configuration blob. Since the > size of those descriptors is always well-defined, we should rather use > the known sizes instead of trusting device-provided values to be > correct. Also adds some safety to potential out-of-order descriptors. > > Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 > Signed-off-by: Julius Werner <jwerner@chromium.org> > --- Please do not forget patch history. > common/usb.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++---------- > common/usb_hub.c | 14 ++++------- > 2 files changed, 64 insertions(+), 23 deletions(-) > > diff --git a/common/usb.c b/common/usb.c > index 55fff5b..ab7bafe 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) > /******************************************************************************* > * Parse the config, located in buffer, and fills the dev->config structure. > * Note that all little/big endian swapping are done automatically. > + * (wTotalLength has already been swapped when it was read.) > */ > static int usb_parse_config(struct usb_device *dev, > unsigned char *buffer, int cfgno) > @@ -361,24 +362,39 @@ static int usb_parse_config(struct usb_device *dev, > head->bDescriptorType); > return -1; > } > - memcpy(&dev->config, buffer, buffer[0]); > - le16_to_cpus(&(dev->config.desc.wTotalLength)); > + if (buffer[0] != USB_DT_CONFIG_SIZE) > + printf("Ignoring invalid USB CFG length (%d)\n", buffer[0]); > + memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE); > dev->config.no_of_if = 0; From a security / robustness standpoint, - if the descriptor length field is found to be abnormal, then the code should not process the packet at all. Here it seems it only warns then goes on to use the descriptor. - if for some reason (although I don't see any) the descriptor must be processed despite its wrong length, then only the bytes at positions 0..length-1 should be considered; bytes beyond the descriptor's announced length should be deemed undefined (and, I might add, should not be even be read from the device). > index = dev->config.desc.bLength; > /* Ok the first entry must be a configuration entry, > * now process the others */ > head = (struct usb_descriptor_header *) &buffer[index]; > - while (index + 1 < dev->config.desc.wTotalLength) { > + while (index + 1 < dev->config.desc.wTotalLength && head->bLength) { Does this change fall into either "using the well-defined length" or "adding safety to out-of-order descriptors" category? If none, then there should be a third category of change documented in the commit message. > switch (head->bDescriptorType) { > case USB_DT_INTERFACE: > + if (index + USB_DT_INTERFACE_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB IF descriptor overflowed buffer!\n"); > + break; > + } > if (((struct usb_interface_descriptor *) \ > &buffer[index])->bInterfaceNumber != curr_if_num) { > /* this is a new interface, copy new desc */ > ifno = dev->config.no_of_if; > + if (ifno >= USB_MAXINTERFACES) { > + puts("Too many USB interfaces!\n"); > + /* try to go on with what we have */ > + return 1; > + } > 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]); > + memcpy(if_desc, &buffer[index], > + USB_DT_INTERFACE_SIZE); Again here, a wrong length is detected yet the descriptor is used. > if_desc->no_of_ep = 0; > if_desc->num_altsetting = 1; > curr_if_num = > @@ -392,12 +408,29 @@ static int usb_parse_config(struct usb_device *dev, > } > break; > case USB_DT_ENDPOINT: > + if (index + USB_DT_ENDPOINT_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB EP descriptor overflowed buffer!\n"); > + break; > + } > + if (ifno < 0) { > + puts("Endpoint descriptor out of order!\n"); > + break; > + } > epno = dev->config.if_desc[ifno].no_of_ep; > if_desc = &dev->config.if_desc[ifno]; > + if (epno > USB_MAXENDPOINTS) { > + printf("Interface %d has too many endpoints!\n", > + if_desc->desc.bInterfaceNumber); > + return 1; > + } > /* found an endpoint */ > if_desc->no_of_ep++; > + if (buffer[index] != USB_DT_ENDPOINT_SIZE) > + printf("Ignoring invalid USB EP length (%d)\n", > + buffer[index]); Ditto. > memcpy(&if_desc->ep_desc[epno], > - &buffer[index], buffer[index]); > + &buffer[index], USB_DT_ENDPOINT_SIZE); > ep_wMaxPacketSize = get_unaligned(&dev->config.\ > if_desc[ifno].\ > ep_desc[epno].\ > @@ -410,9 +443,21 @@ static int usb_parse_config(struct usb_device *dev, > debug("if %d, ep %d\n", ifno, epno); > break; > case USB_DT_SS_ENDPOINT_COMP: > + if (index + USB_DT_SS_EP_COMP_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB EPC descriptor overflowed buffer!\n"); > + break; > + } > + if (ifno < 0 || epno < 0) { > + puts("EPC descriptor out of order!\n"); > + break; > + } > if_desc = &dev->config.if_desc[ifno]; > + if (buffer[index] != USB_DT_SS_EP_COMP_SIZE) > + printf("Ignoring invalid USB EPC length (%d)\n", > + buffer[index]); Ditto. > memcpy(&if_desc->ss_ep_comp_desc[epno], > - &buffer[index], buffer[index]); > + &buffer[index], USB_DT_SS_EP_COMP_SIZE); > break; > default: > if (head->bLength == 0) > @@ -491,7 +536,7 @@ int usb_get_configuration_no(struct usb_device *dev, > unsigned char *buffer, int cfgno) > { > int result; > - unsigned int tmp; > + unsigned int length; > struct usb_config_descriptor *config; > > config = (struct usb_config_descriptor *)&buffer[0]; > @@ -505,16 +550,18 @@ int usb_get_configuration_no(struct usb_device *dev, > "(expected %i, got %i)\n", 9, result); > return -1; > } > - tmp = le16_to_cpu(config->wTotalLength); > + length = le16_to_cpu(config->wTotalLength); > > - if (tmp > USB_BUFSIZ) { > - printf("usb_get_configuration_no: failed to get " \ > - "descriptor - too long: %d\n", tmp); > + if (length > USB_BUFSIZ) { > + printf("%s: failed to get descriptor - too long: %d\n", > + __func__, length); > return -1; > } > > - 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 */ > + > return result; > } > > diff --git a/common/usb_hub.c b/common/usb_hub.c > index 774ba63..d30962e 100644 > --- a/common/usb_hub.c > +++ b/common/usb_hub.c > @@ -320,7 +320,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) > > static int usb_hub_configure(struct usb_device *dev) > { > - int i; > + int i, length; > ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); > unsigned char *bitmap; > short hubCharacteristics; > @@ -341,20 +341,14 @@ static int usb_hub_configure(struct usb_device *dev) > } > descriptor = (struct usb_hub_descriptor *)buffer; > > - /* silence compiler warning if USB_BUFSIZ is > 256 [= sizeof(char)] */ > - i = descriptor->bLength; > - if (i > USB_BUFSIZ) { > - debug("usb_hub_configure: failed to get hub " \ > - "descriptor - too long: %d\n", descriptor->bLength); > - return -1; > - } > + length = min(descriptor->bLength, sizeof(struct usb_hub_descriptor)); > > - if (usb_get_hub_descriptor(dev, buffer, descriptor->bLength) < 0) { > + if (usb_get_hub_descriptor(dev, buffer, length) < 0) { > debug("usb_hub_configure: failed to get hub " \ > "descriptor 2nd giving up %lX\n", dev->status); This message seems less clear than the one it replaces; I suspect "failed to get hub descriptor 2nd giving up" is not a valid sentence. > return -1; > } > - memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength); > + memcpy((unsigned char *)&hub->desc, buffer, length); I'm fine with this change here, as it does just what's required: limiting the memcpy() to the receiving buffer size. This IMO is what should be done also in the other memcpy()s. > /* adjust 16bit values */ > put_unaligned(le16_to_cpu(get_unaligned( > &descriptor->wHubCharacteristics)), Amicalement,
> From a security / robustness standpoint, > > - if the descriptor length field is found to be abnormal, then the code > should not process the packet at all. Here it seems it only warns > then goes on to use the descriptor. Weren't you the guy who was so worried about poor Chinese devices who don't follow the spec just two mails ago? This code just tries to make the most of a broken response while keeping the host safe. Since the descriptors are just put into a structure and left there for the driver to decide which ones it's interested in anyway, more is usually better than less. > - if for some reason (although I don't see any) the descriptor must be > processed despite its wrong length, then only the bytes at positions > 0..length-1 should be considered; bytes beyond the descriptor's > announced length should be deemed undefined (and, I might add, > should not be even be read from the device). This is not reading from the device, it's memcpy()'ing between an already read buffer and another data structure. memcpy()'ing one byte less into an uninitialized struct doesn't make the last byte any less undefined than copying garbage. > Does this change fall into either "using the well-defined length" > or "adding safety to out-of-order descriptors" category? If none, then > there should be a third category of change documented in the commit > message. It's another thing about the length... it prevents the code from running into an endless loop when it encounters a descriptor with length 0. >> - if (usb_get_hub_descriptor(dev, buffer, descriptor->bLength) < 0) { >> + if (usb_get_hub_descriptor(dev, buffer, length) < 0) { >> debug("usb_hub_configure: failed to get hub " \ >> "descriptor 2nd giving up %lX\n", dev->status); > > This message seems less clear than the one it replaces; I suspect > "failed to get hub descriptor 2nd giving up" is not a valid sentence. I'm only touching the condition... if you don't like the message, I'd be happy to review your patch for it.
Dear Julius Werner, > > From a security / robustness standpoint, > > > > - if the descriptor length field is found to be abnormal, then the code > > > > should not process the packet at all. Here it seems it only warns > > then goes on to use the descriptor. > > Weren't you the guy who was so worried about poor Chinese devices who > don't follow the spec just two mails ago? That would have been me, not Albert. I suspect what happens below (and will likely happen onwards, if we don't cut the crap soon) is a bullshit-fueled discussion about nothing, so just can it. It's totally unproductive and drifting away from the discussion about patch. ML is not the right place for duking out this crap ... Please, go, break a tartelette and fix your quarrel elsewhere. Best regards, Marek Vasut
Dear Julius Werner, > The existing USB configuration parsing code relies on the descriptors' > own length values when reading through the configuration blob. Since the > size of those descriptors is always well-defined, we should rather use > the known sizes instead of trusting device-provided values to be > correct. Also adds some safety to potential out-of-order descriptors. > > Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 > Signed-off-by: Julius Werner <jwerner@chromium.org> > --- I finally got time to properly look into the patch, sorry for the delay. > common/usb.c | 73 > ++++++++++++++++++++++++++++++++++++++++++++++---------- common/usb_hub.c > | 14 ++++------- > 2 files changed, 64 insertions(+), 23 deletions(-) > > diff --git a/common/usb.c b/common/usb.c > index 55fff5b..ab7bafe 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) > /************************************************************************* > ****** * Parse the config, located in buffer, and fills the dev->config > structure. * Note that all little/big endian swapping are done > automatically. + * (wTotalLength has already been swapped when it was > read.) > */ > static int usb_parse_config(struct usb_device *dev, > unsigned char *buffer, int cfgno) > @@ -361,24 +362,39 @@ static int usb_parse_config(struct usb_device *dev, > head->bDescriptorType); > return -1; > } > - memcpy(&dev->config, buffer, buffer[0]); > - le16_to_cpus(&(dev->config.desc.wTotalLength)); > + if (buffer[0] != USB_DT_CONFIG_SIZE) > + printf("Ignoring invalid USB CFG length (%d)\n", buffer[0]); 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. > + memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE); > dev->config.no_of_if = 0; 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. Looking through the usage of these structures, you opened quite the ugly can of worms here :( > index = dev->config.desc.bLength; > /* Ok the first entry must be a configuration entry, > * now process the others */ > head = (struct usb_descriptor_header *) &buffer[index]; > - while (index + 1 < dev->config.desc.wTotalLength) { > + while (index + 1 < dev->config.desc.wTotalLength && head->bLength) { 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. > switch (head->bDescriptorType) { > case USB_DT_INTERFACE: > + if (index + USB_DT_INTERFACE_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB IF descriptor overflowed buffer!\n"); > + break; > + } Can you maybe use sizeof(struct usb_interface_descriptor) ? As a future proposal, we might really want to get rid of the USB_DT_INTERFACE_SIZE in favor of using sizeof(), that'd be much less error prone. > if (((struct usb_interface_descriptor *) \ > &buffer[index])->bInterfaceNumber != curr_if_num) { 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 . > /* this is a new interface, copy new desc */ > ifno = dev->config.no_of_if; > + if (ifno >= USB_MAXINTERFACES) { > + puts("Too many USB interfaces!\n"); > + /* try to go on with what we have */ > + return 1; > + } OK > 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. > + memcpy(if_desc, &buffer[index], > + USB_DT_INTERFACE_SIZE); > if_desc->no_of_ep = 0; > if_desc->num_altsetting = 1; > curr_if_num = > @@ -392,12 +408,29 @@ static int usb_parse_config(struct usb_device *dev, > } > break; > case USB_DT_ENDPOINT: > + if (index + USB_DT_ENDPOINT_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB EP descriptor overflowed buffer!\n"); > + break; > + } > + if (ifno < 0) { > + puts("Endpoint descriptor out of order!\n"); > + break; > + } See my rambling above, otherwise I agree. > epno = dev->config.if_desc[ifno].no_of_ep; > if_desc = &dev->config.if_desc[ifno]; > + if (epno > USB_MAXENDPOINTS) { > + printf("Interface %d has too many endpoints!\n", > + if_desc->desc.bInterfaceNumber); > + return 1; > + } > /* found an endpoint */ > if_desc->no_of_ep++; > + if (buffer[index] != USB_DT_ENDPOINT_SIZE) > + printf("Ignoring invalid USB EP length (%d)\n", > + buffer[index]); > memcpy(&if_desc->ep_desc[epno], > - &buffer[index], buffer[index]); > + &buffer[index], USB_DT_ENDPOINT_SIZE); Again, sizeof() ? > ep_wMaxPacketSize = get_unaligned(&dev->config.\ > if_desc[ifno].\ > ep_desc[epno].\ > @@ -410,9 +443,21 @@ static int usb_parse_config(struct usb_device *dev, > debug("if %d, ep %d\n", ifno, epno); > break; > case USB_DT_SS_ENDPOINT_COMP: > + if (index + USB_DT_SS_EP_COMP_SIZE > > + dev->config.desc.wTotalLength) { > + puts("USB EPC descriptor overflowed buffer!\n"); > + break; > + } > + if (ifno < 0 || epno < 0) { > + puts("EPC descriptor out of order!\n"); > + break; > + } > if_desc = &dev->config.if_desc[ifno]; > + if (buffer[index] != USB_DT_SS_EP_COMP_SIZE) > + printf("Ignoring invalid USB EPC length (%d)\n", > + buffer[index]); > memcpy(&if_desc->ss_ep_comp_desc[epno], > - &buffer[index], buffer[index]); > + &buffer[index], USB_DT_SS_EP_COMP_SIZE); > break; > default: > if (head->bLength == 0) > @@ -491,7 +536,7 @@ int usb_get_configuration_no(struct usb_device *dev, > unsigned char *buffer, int cfgno) > { > int result; > - unsigned int tmp; > + unsigned int length; > struct usb_config_descriptor *config; > > config = (struct usb_config_descriptor *)&buffer[0]; > @@ -505,16 +550,18 @@ int usb_get_configuration_no(struct usb_device *dev, > "(expected %i, got %i)\n", 9, result); > return -1; > } > - tmp = le16_to_cpu(config->wTotalLength); > + length = le16_to_cpu(config->wTotalLength); > > - if (tmp > USB_BUFSIZ) { > - printf("usb_get_configuration_no: failed to get " \ > - "descriptor - too long: %d\n", tmp); > + if (length > USB_BUFSIZ) { > + printf("%s: failed to get descriptor - too long: %d\n", > + __func__, length); > return -1; > } > > - 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 ? > return result; > } > > diff --git a/common/usb_hub.c b/common/usb_hub.c > index 774ba63..d30962e 100644 > --- a/common/usb_hub.c > +++ b/common/usb_hub.c > @@ -320,7 +320,7 @@ void usb_hub_port_connect_change(struct usb_device > *dev, int port) > > static int usb_hub_configure(struct usb_device *dev) > { > - int i; > + int i, length; > ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); > unsigned char *bitmap; > short hubCharacteristics; > @@ -341,20 +341,14 @@ static int usb_hub_configure(struct usb_device *dev) > } > descriptor = (struct usb_hub_descriptor *)buffer; > > - /* silence compiler warning if USB_BUFSIZ is > 256 [= sizeof(char)] */ > - i = descriptor->bLength; > - if (i > USB_BUFSIZ) { > - debug("usb_hub_configure: failed to get hub " \ > - "descriptor - too long: %d\n", descriptor->bLength); > - return -1; > - } > + length = min(descriptor->bLength, sizeof(struct usb_hub_descriptor)); > > - if (usb_get_hub_descriptor(dev, buffer, descriptor->bLength) < 0) { > + if (usb_get_hub_descriptor(dev, buffer, length) < 0) { > debug("usb_hub_configure: failed to get hub " \ > "descriptor 2nd giving up %lX\n", dev->status); > return -1; > } > - memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength); > + memcpy((unsigned char *)&hub->desc, buffer, length); > /* adjust 16bit values */ > put_unaligned(le16_to_cpu(get_unaligned( > &descriptor->wHubCharacteristics)), OK
> 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).
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
> 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. Okay, sure, it's not important enough to argue about. Will resubmit it this 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. Yes, maybe. But let's please not blow this patch up any more than it already is. >> > 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? On second thought, we already have the variable head (respectively head->bLength) to point there... I can just use that instead. > So, let's just ignore broken descriptors. Done. > Document this properly then. I'm already adding a comment to usb_parse_config() to point that out... I'll clarify that this includes sanitization in addition to byte swapping.
Hi Julius, > > 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. > > Okay, sure, it's not important enough to argue about. Will resubmit it this > 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. > > Yes, maybe. But let's please not blow this patch up any more than it > already is. > > >> > 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? > > On second thought, we already have the variable head (respectively > head->bLength) to point there... I can just use that instead. > > > So, let's just ignore broken descriptors. > > Done. > > > Document this properly then. > > I'm already adding a comment to usb_parse_config() to point that > out... I'll clarify that this includes sanitization in addition to > byte swapping. THanks a lot! I'm glad you're cleaning this horror up. Best regards, Marek Vasut
diff --git a/common/usb.c b/common/usb.c index 55fff5b..ab7bafe 100644 --- a/common/usb.c +++ b/common/usb.c @@ -341,6 +341,7 @@ static int usb_set_maxpacket(struct usb_device *dev) /******************************************************************************* * Parse the config, located in buffer, and fills the dev->config structure. * Note that all little/big endian swapping are done automatically. + * (wTotalLength has already been swapped when it was read.) */ static int usb_parse_config(struct usb_device *dev, unsigned char *buffer, int cfgno) @@ -361,24 +362,39 @@ static int usb_parse_config(struct usb_device *dev, head->bDescriptorType); return -1; } - memcpy(&dev->config, buffer, buffer[0]); - le16_to_cpus(&(dev->config.desc.wTotalLength)); + if (buffer[0] != USB_DT_CONFIG_SIZE) + printf("Ignoring invalid USB CFG length (%d)\n", buffer[0]); + memcpy(&dev->config, buffer, USB_DT_CONFIG_SIZE); dev->config.no_of_if = 0; index = dev->config.desc.bLength; /* Ok the first entry must be a configuration entry, * now process the others */ head = (struct usb_descriptor_header *) &buffer[index]; - while (index + 1 < dev->config.desc.wTotalLength) { + while (index + 1 < dev->config.desc.wTotalLength && head->bLength) { switch (head->bDescriptorType) { case USB_DT_INTERFACE: + if (index + USB_DT_INTERFACE_SIZE > + dev->config.desc.wTotalLength) { + puts("USB IF descriptor overflowed buffer!\n"); + break; + } if (((struct usb_interface_descriptor *) \ &buffer[index])->bInterfaceNumber != curr_if_num) { /* this is a new interface, copy new desc */ ifno = dev->config.no_of_if; + if (ifno >= USB_MAXINTERFACES) { + puts("Too many USB interfaces!\n"); + /* try to go on with what we have */ + return 1; + } 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]); + memcpy(if_desc, &buffer[index], + USB_DT_INTERFACE_SIZE); if_desc->no_of_ep = 0; if_desc->num_altsetting = 1; curr_if_num = @@ -392,12 +408,29 @@ static int usb_parse_config(struct usb_device *dev, } break; case USB_DT_ENDPOINT: + if (index + USB_DT_ENDPOINT_SIZE > + dev->config.desc.wTotalLength) { + puts("USB EP descriptor overflowed buffer!\n"); + break; + } + if (ifno < 0) { + puts("Endpoint descriptor out of order!\n"); + break; + } epno = dev->config.if_desc[ifno].no_of_ep; if_desc = &dev->config.if_desc[ifno]; + if (epno > USB_MAXENDPOINTS) { + printf("Interface %d has too many endpoints!\n", + if_desc->desc.bInterfaceNumber); + return 1; + } /* found an endpoint */ if_desc->no_of_ep++; + if (buffer[index] != USB_DT_ENDPOINT_SIZE) + printf("Ignoring invalid USB EP length (%d)\n", + buffer[index]); memcpy(&if_desc->ep_desc[epno], - &buffer[index], buffer[index]); + &buffer[index], USB_DT_ENDPOINT_SIZE); ep_wMaxPacketSize = get_unaligned(&dev->config.\ if_desc[ifno].\ ep_desc[epno].\ @@ -410,9 +443,21 @@ static int usb_parse_config(struct usb_device *dev, debug("if %d, ep %d\n", ifno, epno); break; case USB_DT_SS_ENDPOINT_COMP: + if (index + USB_DT_SS_EP_COMP_SIZE > + dev->config.desc.wTotalLength) { + puts("USB EPC descriptor overflowed buffer!\n"); + break; + } + if (ifno < 0 || epno < 0) { + puts("EPC descriptor out of order!\n"); + break; + } if_desc = &dev->config.if_desc[ifno]; + if (buffer[index] != USB_DT_SS_EP_COMP_SIZE) + printf("Ignoring invalid USB EPC length (%d)\n", + buffer[index]); memcpy(&if_desc->ss_ep_comp_desc[epno], - &buffer[index], buffer[index]); + &buffer[index], USB_DT_SS_EP_COMP_SIZE); break; default: if (head->bLength == 0) @@ -491,7 +536,7 @@ int usb_get_configuration_no(struct usb_device *dev, unsigned char *buffer, int cfgno) { int result; - unsigned int tmp; + unsigned int length; struct usb_config_descriptor *config; config = (struct usb_config_descriptor *)&buffer[0]; @@ -505,16 +550,18 @@ int usb_get_configuration_no(struct usb_device *dev, "(expected %i, got %i)\n", 9, result); return -1; } - tmp = le16_to_cpu(config->wTotalLength); + length = le16_to_cpu(config->wTotalLength); - if (tmp > USB_BUFSIZ) { - printf("usb_get_configuration_no: failed to get " \ - "descriptor - too long: %d\n", tmp); + if (length > USB_BUFSIZ) { + printf("%s: failed to get descriptor - too long: %d\n", + __func__, length); return -1; } - 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 */ + return result; } diff --git a/common/usb_hub.c b/common/usb_hub.c index 774ba63..d30962e 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -320,7 +320,7 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) static int usb_hub_configure(struct usb_device *dev) { - int i; + int i, length; ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); unsigned char *bitmap; short hubCharacteristics; @@ -341,20 +341,14 @@ static int usb_hub_configure(struct usb_device *dev) } descriptor = (struct usb_hub_descriptor *)buffer; - /* silence compiler warning if USB_BUFSIZ is > 256 [= sizeof(char)] */ - i = descriptor->bLength; - if (i > USB_BUFSIZ) { - debug("usb_hub_configure: failed to get hub " \ - "descriptor - too long: %d\n", descriptor->bLength); - return -1; - } + length = min(descriptor->bLength, sizeof(struct usb_hub_descriptor)); - if (usb_get_hub_descriptor(dev, buffer, descriptor->bLength) < 0) { + if (usb_get_hub_descriptor(dev, buffer, length) < 0) { debug("usb_hub_configure: failed to get hub " \ "descriptor 2nd giving up %lX\n", dev->status); return -1; } - memcpy((unsigned char *)&hub->desc, buffer, descriptor->bLength); + memcpy((unsigned char *)&hub->desc, buffer, length); /* adjust 16bit values */ put_unaligned(le16_to_cpu(get_unaligned( &descriptor->wHubCharacteristics)),
The existing USB configuration parsing code relies on the descriptors' own length values when reading through the configuration blob. Since the size of those descriptors is always well-defined, we should rather use the known sizes instead of trusting device-provided values to be correct. Also adds some safety to potential out-of-order descriptors. Change-Id: I16f69dfdd6793aa0fe930b5148d4521f3e5c3090 Signed-off-by: Julius Werner <jwerner@chromium.org> --- common/usb.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++---------- common/usb_hub.c | 14 ++++------- 2 files changed, 64 insertions(+), 23 deletions(-)