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 zmkfirmware/zmk#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 zephyrproject-rtos#71299.

Signed-off-by: Keeley Hoek <[email protected]>

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) {