From 69dc31d00f99f53295aa59d70340ce54ae80b440 Mon Sep 17 00:00:00 2001 From: Keeley Hoek Date: Wed, 10 Apr 2024 02:51:54 -0400 Subject: [PATCH] drivers: nrfx: fix USB in endpoint data race The function `usb_dc_ep_write()` races against its caller because it does not copy the passed data buffer as expected by its contract, and as the other drivers do. Thus the TX data may change from underneath the driver while it is pending. Alongside the output endpoint RX buffers which already exist, we define an input endpoint TX buffer for each endpoint and copy into TX data into it before transmitting. The new buffer is protected by the same lock which prevents a write being issued while an existing write is in progress. This bug was discovered on a Kinesis Adv360 keyboard running ZMK, and was observed to very reliably cause keys with a sufficiently high keycode (hence last to be transmitted) to be dropped. With Wireshark two TX messages were recorded on each keypress (corresponding to key press and key release), but both messages contained the same contents (no keys pressed). Only a key press-release combination generated by a macro-like mode of ZMK is fast enough to trigger the bug. My proposed fix to ZMK, the PR https://github.com/zmkfirmware/zmk/pull/2257, simply copies the data into a temporary buffer before the call and immediately fixed the problem. This commit also fixes the bug now using a vanilla copy of ZMK, and has been tested to work on real hardware when backported to ZMK's Zephyr fork. Closes #71299. Signed-off-by: Keeley Hoek --- drivers/usb/device/usb_dc_nrfx.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/usb/device/usb_dc_nrfx.c b/drivers/usb/device/usb_dc_nrfx.c index d447afc10aae7e..30f8e21ec54b62 100644 --- a/drivers/usb/device/usb_dc_nrfx.c +++ b/drivers/usb/device/usb_dc_nrfx.c @@ -124,6 +124,7 @@ struct nrf_usbd_ep_buf { */ struct nrf_usbd_ep_ctx { struct nrf_usbd_ep_cfg cfg; + uint8_t *in_buf; struct nrf_usbd_ep_buf out_buf; volatile bool read_complete; volatile bool read_pending; @@ -202,9 +203,18 @@ K_MEM_SLAB_DEFINE(fifo_elem_slab, FIFO_ELEM_SZ, #define EP_ISOIN_INDEX CFG_EPIN_CNT #define EP_ISOOUT_INDEX (CFG_EPIN_CNT + CFG_EP_ISOIN_CNT + CFG_EPOUT_CNT) +#define EP_IN_BUF_MAX_SZ 1024UL #define EP_OUT_BUF_MAX_SZ 64UL #define ISO_EP_OUT_BUF_MAX_SZ 1024UL +/** + * @brief Input endpoint buffers + * Used as buffers for the input endpoints' data transfer + * Max buffers size possible: 1024 Bytes + */ +static uint8_t ep_in_bufs[CFG_EPIN_CNT][EP_IN_BUF_MAX_SZ] + __aligned(sizeof(uint32_t)); + /** * @brief Output endpoint buffers * Used as buffers for the output endpoints' data transfer @@ -632,6 +642,8 @@ static int eps_ctx_init(void) ep_ctx = in_endpoint_ctx(i); __ASSERT_NO_MSG(ep_ctx); ep_ctx_reset(ep_ctx); + + ep_ctx->in_buf = ep_in_bufs[i]; } for (i = 0U; i < CFG_EPOUT_CNT; i++) { @@ -1621,7 +1633,7 @@ int usb_dc_ep_flush(const uint8_t ep) } int usb_dc_ep_write(const uint8_t ep, const uint8_t *const data, - const uint32_t data_len, uint32_t *const ret_bytes) + uint32_t data_len, uint32_t *const ret_bytes) { LOG_DBG("ep_write: ep 0x%02x, len %d", ep, data_len); struct nrf_usbd_ctx *ctx = get_usbd_ctx(); @@ -1689,8 +1701,13 @@ int usb_dc_ep_write(const uint8_t ep, const uint8_t *const data, return 0; } + if (data_len > EP_IN_BUF_MAX_SZ) { + data_len = EP_IN_BUF_MAX_SZ; + } + memcpy(ep_ctx->in_buf, data, data_len); + ep_ctx->write_in_progress = true; - NRF_USBD_COMMON_TRANSFER_IN(transfer, data, data_len, 0); + NRF_USBD_COMMON_TRANSFER_IN(transfer, ep_ctx->in_buf, data_len, 0); nrfx_err_t err = nrf_usbd_common_ep_transfer(ep_addr_to_nrfx(ep), &transfer); if (err != NRFX_SUCCESS) {