CommunityGD32Cores / ArduinoCore-GD32

Arduino core for GD32 devices, community developed, based on original GigaDevice's core
Other
85 stars 33 forks source link

Fix race condition causing buffer overrun in tight read loop. #79

Closed bjc closed 1 year ago

bjc commented 1 year ago

This patch fixes a race condition present when calling ‘USBCore::pop’ in a tight loop, such as in ‘Stream::timedPeek’.

The issue occurs because, when the USB peripheral gets a packet from the host, it does the following two things, in order:

1) set a flag on the endpoint to return a NAK status to the host for subsequent OUT packets, and then 2) set the interrupt pending flag and associated registers indicating that new data is available.

This is not done atomically, and is handled by the peripheral concurrently with the CPU executing the firmware. As a result, there is a time between steps 1 and 2 where, after the peripheral has set the status flag to return a NAK, firmware code can overwrite that flag to specify that the endpoint is a valid state to receive more data, and then the interrupt is fired. Assuming no other changes are made to the peripheral's registers, after this point, an OUT data packet to the endpoint may come in immediately after current interrupt is serviced and, as a result, continue writing past the end of a full transaction buffer.

The previous version of ‘USBCore::pop’ blindly set the endpoint status to say that it was available for OUT packets, assuming that as long as its data buffer was empty, and that interrupts were disabled for the duration of its operation, it was in the correct state. However, if host data is coming in fast enough, and the calls to ‘pop’ are also fast enough, then it can trigger this race, because the USB peripheral can change the endpoint status outside of interrupt context and concurrently to firmware code.

This patch addresses that issue be only setting the endpoint status once, the first time more data is requested after the buffer is empty.

algernon commented 1 year ago

FWIW, we've been running the same patch on Keyboardio Model 100s for the past week or so, with great success.

maxgerhardt commented 1 year ago

Tested to be working on Windows and GD32F303CC, merging! :)