USB HOST library is not thread safe

[BDurs]

USB HOST library is not thread safe (RTOS mode). Documentation UM1720 does not warn about it. It even gives false belief it is safe.

5.3 Using the host library in RTOS mode

[...]

When the RTOS mode is used the host core background process runs as a separate RTOS task. The communication between the application task and the host core task uses the RTOS message queue mechanism. The CMSIS RTOS APIs are used to support the RTOS mode.

[...]

MSC class does not provide protection against NULL pointer dereference and modification of deallocated memory (free).

I did not check other classess but I assume the bug is also there as the defect is generic.

The problem is in:

MSC_HandleTypeDef *MSC_Handle = (MSC_HandleTypeDef *) phost->pActiveClass->pData;

pActiveClass and pData are controlled by USBH_Process. It assignes address tp pActiveClass and malloc/free pData.

RTOS can switch from thread reading a file (application) to the USBH_Process thread, execute HOST_DEV_DISCONNECTED case and go back to file reading thread.

Is ST aware of this bug and when it may be repaired?

A good solution would be that file reading thread would not read directly but pass a request to the USBH_Process via message queue mechanism.

Then USBH_Process would do the job. This solution is safe because there would be only one thread which uses and modifies the data.

Generally CLASS code and CORE code inter-operationality in buggy.

USBH_StatusTypeDef  USBH_Process(USBH_HandleTypeDef *phost)
{
  __IO USBH_StatusTypeDef status = USBH_FAIL;
  uint8_t idx = 0U;
 
  /* check for Host pending port disconnect event */
  if (phost->device.is_disconnected == 1U)
  {
    phost->gState = HOST_DEV_DISCONNECTED;
  }
[...]
    case HOST_DEV_DISCONNECTED :
      phost->device.is_disconnected = 0U;
 
      DeInitStateMachine(phost);
 
      /* Re-Initilaize Host for new Enumeration */
      if (phost->pActiveClass != NULL)
      {
        phost->pActiveClass->DeInit(phost);  // COMMENT: It deallocates phost->pActiveClass->pData (i.e. MSC_HandleTypeDef).
        phost->pActiveClass = NULL;
        // COMMENT: Now RTOS can switch back to file reading thread and we have a big problem.
      }
[...]
}

USBH_StatusTypeDef USBH_MSC_Read(USBH_HandleTypeDef *phost,
                                 uint8_t lun,
                                 uint32_t address,
                                 uint8_t *pbuf,
                                 uint32_t length)
{
  uint32_t timeout;
  MSC_HandleTypeDef *MSC_Handle = (MSC_HandleTypeDef *) phost->pActiveClass->pData;
  // COMMENT: BUG: Possible NULL pointer dereference
 
  if ((phost->device.is_connected == 0U) ||
      (phost->gState != HOST_CLASS) ||
      (MSC_Handle->unit[lun].state != MSC_IDLE))
  {
    return  USBH_FAIL;
  }
 
  MSC_Handle->state = MSC_READ;
  MSC_Handle->unit[lun].state = MSC_READ;
  MSC_Handle->rw_lun = lun;
 
  USBH_MSC_SCSI_Read(phost, lun, address, pbuf, length);
 
  timeout = phost->Timer;
 
  // COMMENT: USBH_MSC_RdWrProcess dereferences pActiveClass and uses pData without any check. It is done in a loop, so very often.
  while (USBH_MSC_RdWrProcess(phost, lun) == USBH_BUSY)
  {
    if (((phost->Timer - timeout) > (10000U * length)) || (phost->device.is_connected == 0U))
    {
      MSC_Handle->state = MSC_IDLE;
      return USBH_FAIL;
    }
  }
  MSC_Handle->state = MSC_IDLE;
 
  return USBH_OK;
}

What is also bad in this library is that USBH_MSC_RdWrProcess is not releasing the CPU - no semaphore, no message queue. It causes a drop in performance, especially when thread has high priority.

[Pavel A.]

Yes, not thread safe. Consider other, commercial quality, supported USB libraries. This one is free.

Compatibility of malloc with RTOS is a unrelated issue, it was discussed here - please search.

-- pa

[#3]

:|

The fact that it is free cannot be a justification for not doing it well, it may be interesting that the source code is free and that the community can help to improve the code, just as it happens with the Arduino platform.

There are examples of use of libraries that are from years ago and it seems that they have not received updates, so it is difficult for the user to have to get around all the situations that have already been reported.

If the user has to search for solutions with each step he takes, a final product will never be reached, and as a result only a sample of components will be acquired, and large-scale acquisition will not be done, because it was not possible to reach the final product.

If everyone who needs to finish a project, has to buy expensive tools like MDK Keil, then few products will be made.

[Pavel A.]

> If everyone who needs to finish a project, has to buy expensive tools like MDK Keil, then few products will be made.

Even the paid Keil library does not have good USB support for host mode (hubs, for example).

It takes a professional, focused team to develop and support such library.

-- pa

[TDK]

> If the user has to search for solutions with each step he takes, a final product will never be reached, and as a result only a sample of components will be acquired, and large-scale acquisition will not be done, because it was not possible to reach the final product.

Depends on the user I guess. There are plenty of projects out there, and plenty of them use HAL, so it's not quite as doom-and-gloom as you suggest.

The USB library is not perfect, but that doesn't mean it's useless. Same with HAL. The USB stack is complicated enough to handle correctly without adding in RTOS.

Heck, the majority of the HAL library is not thread-safe. But if you know the limitations, it can speed up development time.

  #define __HAL_LOCK(__HANDLE__)                                           \
                                do{                                        \
                                    if((__HANDLE__)->Lock == HAL_LOCKED)   \
                                    {                                      \
                                       return HAL_BUSY;                    \
                                    }                                      \
                                    else                                   \
                                    {                                      \
                                       (__HANDLE__)->Lock = HAL_LOCKED;    \
                                    }                                      \
                                  }while (0)

[Amel NASRI]

Hello @BDurs​ ,

Please note that ST is already aware of this limitation and the lock mechanism will be reworked in STM32Cube, as confirmed by @MMAST.1​ in https://community.st.com/s/question/0D50X0000C5Tns8SQC/bug-stm32-hal-driver-lock-mechanism-is-not-interrupt-safe.

-Amel

[NAgha.1]

Hello

what is the solution of USBH_MSC_RdWrProcess stucked in a loop for 3 seconds problem ?