ux_device_class_cdc_acm_read() overflowing buffer

[JonTBeckett]

Hello, I am using a device with an STM32 Arm Cortex M33 based processor as a CDC ACM device and the ux_device_class_cdc_acm_read() function is reading more bytes than requested_length. The call is requesting 5 bytes be read but the actual_length is 0x40 (wMaxPacketSize) upon return. After stepping through with a debugger, the transfer_request->ux_slave_transfer_request_requested_length is appropriately set to 5 and remains so for the duration of the call. The transfer_request->ux_slave_transfer_request_actual_length remains 0 until after the call to _ux_utility_semaphore_get() call in the ux_dcd_stm32_transfer_request.c file, at which point it is set to the aforementioned 0x40.

I will also note that the host machine sent 158 bytes to the device. So the app is simply request the first 5 bytes of that larger message.

Based on the USBX documentation for this function, I expect it to respect the requested_length and not overflow the provided buffer. Is this a correct understanding?

Thank you for any answers you can provide.

Regards, Jonathan Beckett

[xiaocq2001]

Hi, when host send 158 bytes to device, it's packaged to 64, 64, 30, and send to device packet by packet, so on device side two full packet and a 30 byte packet can be received.

In such a case, if device side USB controller is configured to accept only 5 bytes, buffer overflow is expected to be generated by hardware and handled by controller driver (DCD).

But buffer length of 5 is not recommended, since in such a case the data from host is missed.

The recommended way is to read length of full packets and check actual bytes returned. E.g., read 64 byte for full speed and 512 bytes for high speed, the data is then saved in buffer and you can pick up first 5 for further process. The remaining packets are NAKed before read start again, so no data is lost.

[JonTBeckett]

So if I am understanding your response correctly, the requested_length provided to ux_device_class_cdc_acm_read() should always be at least max packet size? Meaning that there is a chance that ux_device_class_cdc_acm_read will overflow myBuff in the following example if the host has sent more than 5 bytes?

extern UX_SLAVE_CLASS_CDC_ACM* my_cdc_acm;
void read_five_bytes(void)
{
    UCHAR myBuff[5];
    ULONG actual_length;

    ux_device_class_cdc_acm_read(my_cdc_acm, myBuff, 5, &actual_length);
}

This isn't the behavior I would expect based on the USBX documentation which states "This functions reads raw bulk data from host, so it keeps pending until buffer is full or host terminates the transfer by a short packet (including Zero Length Packet)."

In my situation, it is a third-party library that reads 5 bytes. So the only solution is to wrap the USBX read and have my own buffer of 64 bytes (I am using Full Speed), read into that buffer instead of the buffer provided by the third-party library, and then copy the correct amount of data to the third-party library's buffer.

Am I understanding this correctly?

[xiaocq2001]

Yes. You are correct, you should wrap the USBX read so that the third-party library can process data not aligned to USB packet.

[JonTBeckett]

Interesting. That's what I will do then. I would recommend updating your documentation to indicate this behavior. Overflowing a buffer is serious bug.

Thank you for the clarification.

[JonTBeckett]

I just found Issue #66 which is basically the same problem. Based on that I will just reiterate that the USBX documentation does not express this behavior.

[xiaocq2001]

Discussion is done here. The document will be updated later.

Feel free to open tickets if there are issues.

[JonTBeckett]

Hi again, I'm new to working with USB stacks with USBX being the only one I've ever used. It has come to my attention that other embedded USB stacks do not have these buffer overflow issues.

Is there a reason you have chosen to force consumers to only read multiples of the max packet size? At the very least it seems like there should be some protection within your interface (ux_device_class_cdc_acm_read()) against buffer overflows.

The more I work with USBX, the more little quirks I see like this that begin to concern me. The product I'm working on is in early development and hasn't been put through rigorous testing and I'm afraid with little undocumented oddities like this, USBX may not hold up and will require added work down the road to work around undocumented issues like this.

[xiaocq2001]

The reason to recommend read multiples of the max packet size is to avoid data loss.

USB build data in packets, so if you decide to read just 5 bytes it's 5 bytes in a packet and the data follows is lost (in case there is overflow supported by software) or even you will get an overflow (in case the hardware support assigning bytes for transfer length and can generate overflow error). Both case cause command failure or data loss.

You may not see similar issue for other class since on CDC-ACM you are facing raw data on USB pipes.

[PaulCerna]

I will be switching my project to USBX in the next few weeks... I am really puzzled by the idea that it is ok to overrun the length of a buffer provided for the read. if the requirement is to provide at least 64 bytes long buffer, then the read function must return an error if the caller passes in a shorter buffer. Buffer overflows being one of the main causes of cybersecurity incidents, having this vulnerability in the code is to me simply not an option. Would you, please, reconsider the answer to this ticket ?

[xiaocq2001]

I just recommend not providing length of buffer smaller than packet size to avoid losing data.

For overflow, it should be supported in lower level, or even by hardware, since the expected length is passed to hardware to receive USB packet, in such a case, the hardware should discard USB packet larger than the expected size and report overflow error. When low level controller driver gets the overflow error, it reports to CDC to indicate there is overflow.

In your case, it seems the low level driver does not correctly generate such overflow error. Let's check with ST on the issue.

[JonTBeckett]

So in the situation I came across, the host has sent 100+ bytes, the third-party library asks for 5 bytes using ux_device_class_cdc_acm_read(), the low level driver returns 64 bytes to ux_device_class_cdc_acm_read(), then ux_device_class_cdc_acm_read() indicates it has read 64 bytes.

So I will agree that the overflow is happening in the low lever driver. I believe I was initially expecting USBX to abstract that type of issue away from the user by buffering the data coming from the low lever driving and then providing only what the user requested. I expected that based on the documentation for ux_device_class_cdc_acm_read(). That’s why I was initially satisfied with your original answer of updating the documentation. But then some coworkers mentioned that some previous USB middleware (based on the same low level drivers) don’t have these buffer overflow issues.

I agree with PaulCerna though that at the very least, there should be some sort of checking and error response when the buffer provided to ux_device_class_cdc_acm_read() isn’t on a an integer multiple of maxPacketSize.

I’ll continue to dig into the relationship between USBX and the ST low level driver to understand the problem better. If I find that they have an issue I will reach out to them. But like I said, my coworkers indicated that other USB CDC ACM middleware they’ve used in the past on top of those same ST low lever drivers did not have these buffer overflow issues.