Potential bug in USB_DeviceGetDescriptor() function used in many usb_examples projects...

cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 

Potential bug in USB_DeviceGetDescriptor() function used in many usb_examples projects...

704 Views
EdSutter
Senior Contributor II

I was looking at the function USB_DeviceGetDescriptor() in the file usb_examlpes/usb_device_composite_hid_audio_unified_lite/usb_device_descriptor.c...

(note that this probably applies to all the projects that have this function)

The code that is used in the switch case USB_DESCRIPTOR_TYPE_STRING seems at best to be confusing, and potentially hazardous if the device supports more than one language...

As best I can tell, the host passes the device its language ID in setup->wIndex. The device looks for a match in the languageList, and if found, it returns the requested string. If this is not the purpose of this snippet of code then everything that I say below is bogus so stop here (and reply with an explanation please, and feel free to call me an idiot)...

The loop uses USB_DEVICE_STRING_COUNT for no real reason, but worse than that, could potentially be hazardous.  Following is the code from the SDK...

 

    usb_status_t error      = kStatus_USB_Success;
    uint8_t descriptorType  = (uint8_t)((setup->wValue & 0xFF00U) >> 8U);
    uint8_t descriptorIndex = (uint8_t)((setup->wValue & 0x00FFU));

    if (USB_REQUEST_STANDARD_GET_DESCRIPTOR != setup->bRequest)
    {
        return kStatus_USB_InvalidRequest;
    }

    switch (descriptorType)
    {
        case USB_DESCRIPTOR_TYPE_STRING:
            /* Get string descriptor */
            if (0U == descriptorIndex)
            {
                *buffer = (uint8_t *)g_UsbDeviceLanguageList.languageString;
                *length = g_UsbDeviceLanguageList.stringLength;
            }
            else
            {
                uint8_t languageIndex = 0U;
                uint8_t stringIndex   = USB_DEVICE_STRING_COUNT;

                for (; languageIndex < USB_DEVICE_LANGUAGE_COUNT; languageIndex++)
                {
                    if (setup->wIndex == g_UsbDeviceLanguageList.languageList[languageIndex].languageId)
                    {
                        if (descriptorIndex < USB_DEVICE_STRING_COUNT)
                        {
                            stringIndex = descriptorIndex;
                        }
                        break;
                    }
                }

                if (USB_DEVICE_STRING_COUNT == stringIndex)
                {
                    return kStatus_USB_InvalidRequest;
                }
                *buffer = (uint8_t *)g_UsbDeviceLanguageList.languageList[languageIndex].string[stringIndex];
                *length = g_UsbDeviceLanguageList.languageList[languageIndex].length[stringIndex];
            }
            break;

 

 

The error condition is hit if the incoming language index (setup->wIndex) does not find a match with the targets' set of supported languages. This naturally occurs if the loop increments through to its max value; but instead of testing for that, the code tests for...

(USB_DEVICE_STRING_COUNT == languageIndex)

Several confusing points IMHO here:
1. Its confusing (just test for languageIndex >= USB_DEVICE_LANGUAGE_COUNT)
2. If the target supports several languages but only a few strings, then the loop could legitimately have languageIndex == USB_DEVICE_STRING_COUNT.

For anyone that agrees, here's what I use in place of the above code (I believe this fixes the error case only encountered if multiple languages are defined):

 

    uint8_t descriptorType  = (uint8_t)((setup->wValue & 0xFF00U) >> 8U);
    uint8_t stringIdx       = (uint8_t)((setup->wValue & 0x00FFU));
    usb_status_t ret        = kStatus_USB_Success;

    if (USB_REQUEST_STANDARD_GET_DESCRIPTOR != setup->bRequest)
    {
        return kStatus_USB_InvalidRequest;
    }
    switch (descriptorType)
    {
        case USB_DESCRIPTOR_TYPE_STRING:
            /* Get string descriptor */
            if (0U == stringIdx)
            {
                *buffer = (uint8_t *)g_UsbDeviceLanguageList.languageString;
                *length = g_UsbDeviceLanguageList.stringLength;
            }
            else
            {
                uint8_t languageIdx;

                if (stringIdx >= USB_DEVICE_STRING_COUNT)
                    return kStatus_USB_InvalidRequest;

                for (languageIdx=0; languageIdx < USB_DEVICE_LANGUAGE_COUNT; languageIdx++)
                {
                    if (setup->wIndex == g_UsbDeviceLanguageList.languageList[languageIdx].languageId)
                        break;
                }

                if (languageIdx >= USB_DEVICE_LANGUAGE_COUNT)
                    return kStatus_USB_InvalidRequest;
					
                *buffer = (uint8_t *)g_UsbDeviceLanguageList.languageList[languageIndex].string[stringIndex];
                *length = g_UsbDeviceLanguageList.languageList[languageIndex].length[stringIndex];
            }
            break;

 

 

Labels (1)
Tags (1)
0 Kudos
3 Replies

674 Views
danielchen
NXP TechSupport
NXP TechSupport

It seems your code is based on an old version.  The latest version is SDK 2.9.2.

 

Regards

Daniel

 

 

0 Kudos

663 Views
EdSutter
Senior Contributor II

The only difference between what I show and the latest SDK (actually its 2.9.3) is a variable name change.

There is no change to the logic, so I believe the issue I mention still applies.

Ed

0 Kudos

685 Views
danielchen
NXP TechSupport
NXP TechSupport

Hi 

 

Thank you very much for your feedback. I reported the potential issue to the software team.

 

Regards

Daniel

0 Kudos