Fix transfer lengths that wrap when narrowed to 16 bits

src/class/msc/msc_device.c

@@ -167,7 +167,7 @@ static uint8_t rdwr10_validate_cmd(msc_cbw_t const* cbw) {
     } else if (SCSI_CMD_WRITE_10 == cbw->command[0] && is_data_in(cbw->dir)) {
       TU_LOG_DRV("  SCSI case 8 (Hi <> Do)\r\n");
       status = MSC_CSW_STATUS_PHASE_ERROR;
-    } else if (0 == block_count) {
+    } else if (0 == block_count || (cbw->total_bytes >> 16) >= block_count) { // or block size > 16-bit, without a divide
       TU_LOG_DRV("  SCSI case 4 Hi > Dn (READ10) or case 9 Ho > Dn (WRITE10) \r\n");
       status = MSC_CSW_STATUS_FAILED;
     } else if (cbw->total_bytes / block_count == 0) {

src/class/msc/msc_host.c

@@ -43,6 +43,7 @@ typedef struct {
   // SCSI command data
   uint8_t stage;
   void* buffer;
+  uint32_t data_xferred;
   tuh_msc_complete_cb_t complete_cb;
   uintptr_t complete_arg;
 
@@ -68,6 +69,21 @@ TU_ATTR_ALWAYS_INLINE static inline msch_epbuf_t* get_epbuf(uint8_t daddr) {
   return &_msch_epbuf[daddr - 1];
 }
 
+// usbh_edpt_xfer() length is 16-bit: a larger data stage is split into transfers of this size,
+// a multiple of every bulk max packet size so that only the last one can end short
+#define MSCH_DATA_XFER_MAX 0xFC00u
+
+static bool data_stage_xfer(uint8_t daddr, msch_interface_t* p_msc, const msc_cbw_t* cbw) {
+  const uint8_t  ep_data = (cbw->dir & TUSB_DIR_IN_MASK) ? p_msc->ep_in : p_msc->ep_out;
+  const uint16_t len     = (uint16_t) tu_min32(cbw->total_bytes - p_msc->data_xferred, MSCH_DATA_XFER_MAX);
+  return usbh_edpt_xfer(daddr, ep_data, (uint8_t*) p_msc->buffer + p_msc->data_xferred, len);
+}
+
+static bool status_stage_xfer(uint8_t daddr, msch_interface_t* p_msc, msc_csw_t* csw) {
+  p_msc->stage = MSC_STAGE_STATUS;
+  return usbh_edpt_xfer(daddr, p_msc->ep_in, (uint8_t*) csw, (uint16_t) sizeof(msc_csw_t));
+}
+
 //--------------------------------------------------------------------+
 // Weak stubs: invoked if no strong implementation is available
 //--------------------------------------------------------------------+
@@ -222,7 +238,9 @@ bool tuh_msc_read10(uint8_t dev_addr, uint8_t lun, void* buffer, uint32_t lba, u
   msc_cbw_t cbw;
   cbw_init(&cbw, lun);
 
-  cbw.total_bytes = block_count * p_msc->capacity[lun].block_size;
+  const uint32_t block_size = p_msc->capacity[lun].block_size;
+  TU_VERIFY(block_count <= UINT32_MAX / tu_max32(block_size, 1)); // CBW data length is 32-bit
+  cbw.total_bytes = block_count * block_size;
   cbw.dir = TUSB_DIR_IN_MASK;
   cbw.cmd_len = sizeof(scsi_read10_t);
 
@@ -244,7 +262,9 @@ bool tuh_msc_write10(uint8_t dev_addr, uint8_t lun, void const* buffer, uint32_t
   msc_cbw_t cbw;
   cbw_init(&cbw, lun);
 
-  cbw.total_bytes = block_count * p_msc->capacity[lun].block_size;
+  const uint32_t block_size = p_msc->capacity[lun].block_size;
+  TU_VERIFY(block_count <= UINT32_MAX / tu_max32(block_size, 1)); // CBW data length is 32-bit
+  cbw.total_bytes = block_count * block_size;
   cbw.dir         = TUSB_DIR_OUT;
   cbw.cmd_len     = sizeof(scsi_write10_t);
 
@@ -318,16 +338,21 @@ bool msch_xfer_cb(uint8_t dev_addr, uint8_t ep_addr, xfer_result_t event, uint32
       if (cbw->total_bytes && p_msc->buffer) {
         // Data stage if any
         p_msc->stage = MSC_STAGE_DATA;
-        uint8_t const ep_data = (cbw->dir & TUSB_DIR_IN_MASK) ? p_msc->ep_in : p_msc->ep_out;
-        TU_ASSERT(usbh_edpt_xfer(dev_addr, ep_data, p_msc->buffer, (uint16_t) cbw->total_bytes));
-        break;
+        p_msc->data_xferred = 0;
+        TU_ASSERT(data_stage_xfer(dev_addr, p_msc, cbw));
+      } else {
+        TU_ASSERT(status_stage_xfer(dev_addr, p_msc, csw));
       }
-      TU_ATTR_FALLTHROUGH; // fallthrough to data stage
+      break;
 
     case MSC_STAGE_DATA:
-      // Status stage
-      p_msc->stage = MSC_STAGE_STATUS;
-      TU_ASSERT(usbh_edpt_xfer(dev_addr, p_msc->ep_in, (uint8_t*) csw, (uint16_t) sizeof(msc_csw_t)));
+      p_msc->data_xferred += xferred_bytes;
+      if (event == XFER_RESULT_SUCCESS && xferred_bytes == MSCH_DATA_XFER_MAX &&
+          p_msc->data_xferred < cbw->total_bytes) {
+        TU_ASSERT(data_stage_xfer(dev_addr, p_msc, cbw));
+      } else {
+        TU_ASSERT(status_stage_xfer(dev_addr, p_msc, csw));
+      }
       break;
 
     case MSC_STAGE_STATUS:

src/class/mtp/mtp_device.c

@@ -90,6 +90,8 @@ typedef struct {
   TU_ATTR_ALIGNED(4) uint8_t control_buf[CFG_TUD_MTP_EP_CONTROL_BUFSIZE];
 } mtpd_interface_t;
 
+TU_VERIFY_STATIC(CFG_TUD_MTP_EP_BUFSIZE <= UINT16_MAX, "usbd_edpt_xfer() length is 16-bit");
+
 typedef struct {
   TUD_EPBUF_DEF(buf, CFG_TUD_MTP_EP_BUFSIZE);
   TUD_EPBUF_TYPE_DEF(mtp_event_t, buf_event);
@@ -223,6 +225,7 @@ bool tud_mtp_data_receive(mtp_container_info_t *p_container) {
 
 bool tud_mtp_response_send(mtp_container_info_t* p_container) {
   mtpd_interface_t* p_mtp = &_mtpd_itf;
+  TU_VERIFY(p_container->header->len <= CFG_TUD_MTP_EP_BUFSIZE);
   p_mtp->phase = MTP_PHASE_RESPONSE;
   p_container->header->type = MTP_CONTAINER_TYPE_RESPONSE_BLOCK;
   p_container->header->transaction_id = p_mtp->command.header.transaction_id;

src/class/vendor/vendor_device.c

@@ -253,7 +253,7 @@ uint32_t tud_vendor_n_write(uint8_t idx, const void *buffer, uint32_t bufsize) {
   vendord_interface_t *p_itf = &_vendord_itf[idx];
 
   #if CFG_TUD_VENDOR_TXRX_BUFFERED
-  return tu_edpt_stream_write(&p_itf->tx_stream, buffer, (uint16_t)bufsize);
+  return tu_edpt_stream_write(&p_itf->tx_stream, buffer, bufsize);
 
   #else
   // non-fifo mode: direct transfer (ep_in is 0 while an altsetting without a bulk IN ep is active)

src/host/usbh.c

@@ -1170,7 +1170,7 @@ bool tuh_edpt_xfer(tuh_xfer_t* xfer) {
   uint8_t const daddr = xfer->daddr;
   uint8_t const ep_addr = xfer->ep_addr;
 
-  TU_VERIFY(daddr && ep_addr);
+  TU_VERIFY(daddr && ep_addr && xfer->buflen <= UINT16_MAX);
   TU_VERIFY(usbh_edpt_claim(daddr, ep_addr));
 
   if (!usbh_edpt_xfer_with_callback(daddr, ep_addr, xfer->buffer, (uint16_t) xfer->buflen,

src/tusb.c

@@ -406,7 +406,7 @@ uint32_t tu_edpt_stream_write_xfer(tu_edpt_stream_t *s) {
 
 uint32_t tu_edpt_stream_write(tu_edpt_stream_t *s, const void *buffer, uint32_t bufsize) {
   TU_VERIFY(bufsize > 0);
-  const uint16_t ret = tu_fifo_write_n(&s->ff, buffer, (uint16_t) bufsize);
+  const uint16_t ret = tu_fifo_write_n(&s->ff, buffer, (uint16_t) tu_min32(bufsize, UINT16_MAX));
 
   // flush if fifo has more than packet size or
   // in rare case: fifo depth is configured too small (which never reach packet size)
@@ -448,7 +448,7 @@ uint32_t tu_edpt_stream_read_xfer(tu_edpt_stream_t *s) {
 }
 
 uint32_t tu_edpt_stream_read(tu_edpt_stream_t *s, void *buffer, uint32_t bufsize) {
-  const uint32_t num_read = tu_fifo_read_n(&s->ff, buffer, (uint16_t)bufsize);
+  const uint32_t num_read = tu_fifo_read_n(&s->ff, buffer, (uint16_t) tu_min32(bufsize, UINT16_MAX));
   tu_edpt_stream_read_xfer(s);
   return num_read;
 }

test/unit-test/project.yml

@@ -139,6 +139,10 @@
       - CFG_TUSB_DEBUG=0
       - CFG_TUH_AUDIO_EPIN_BUFSIZE=1024
       - CFG_TUH_AUDIO_EPOUT_BUFSIZE=1024
+    :test_msc_host:
+      - CFG_TUH_ENABLED=1
+      - CFG_TUH_MSC=1
+      - CFG_TUSB_DEBUG=0
   :release: []
 
   # Enable to inject name of a test as a unique compilation symbol into its respective executable build.

test/unit-test/test/device/msc/test_msc_device.c

@@ -282,3 +282,73 @@ void test_msc(void)
 
   tud_task();
 }
+
+// A block size computed from the CBW that does not fit 16 bits must fail, not wrap (66048 / 1 would read as 512)
+void test_msc_read10_block_size_over_16bit(void)
+{
+  uint32_t const total_bytes = 65536 + 512;
+
+  msc_cbw_t cbw_read10 =
+  {
+    .signature = MSC_CBW_SIGNATURE,
+    .tag = 0xCAFECAFE,
+    .total_bytes = total_bytes,
+    .lun = 0,
+    .dir = TUSB_DIR_IN_MASK,
+    .cmd_len = sizeof(scsi_read10_t)
+  };
+
+  scsi_read10_t cmd_read10 =
+  {
+      .cmd_code    = SCSI_CMD_READ_10,
+      .lba         = tu_htonl(0),
+      .block_count = tu_htons(1)
+  };
+
+  memcpy(cbw_read10.command, &cmd_read10, cbw_read10.cmd_len);
+
+  desc_configuration = data_desc_configuration;
+  uint8_t const* desc_ep = tu_desc_next(tu_desc_next(desc_configuration));
+
+  dcd_event_setup_received(rhport, (uint8_t*) &request_set_configuration, false);
+
+  dcd_edpt_open_ExpectAndReturn(rhport, (tusb_desc_endpoint_t const *) desc_ep, true);
+  dcd_edpt_open_ExpectAndReturn(rhport, (tusb_desc_endpoint_t const *) tu_desc_next(desc_ep), true);
+
+  dcd_edpt_xfer_ExpectAndReturn(rhport, EDPT_MSC_OUT, NULL, sizeof(msc_cbw_t), false, true);
+  dcd_edpt_xfer_IgnoreArg_buffer();
+  dcd_edpt_xfer_ReturnMemThruPtr_buffer( (uint8_t*) &cbw_read10, sizeof(msc_cbw_t));
+
+  dcd_event_xfer_complete(rhport, EDPT_MSC_OUT, sizeof(msc_cbw_t), 0, true);
+
+  dcd_edpt_xfer_ExpectAndReturn(rhport, EDPT_CTRL_IN, NULL, 0, false, true);
+
+  // case 4 (Hi > Dn): no data, STALL Bulk-In, then a failed CSW with full residue after the host clears it
+  dcd_edpt_stall_Expect(rhport, EDPT_MSC_IN);
+
+  tud_task();
+
+  tusb_control_request_t const request_clear_halt =
+  {
+    .bmRequestType = 0x02,
+    .bRequest      = TUSB_REQ_CLEAR_FEATURE,
+    .wValue        = TUSB_REQ_FEATURE_EDPT_HALT,
+    .wIndex        = EDPT_MSC_IN,
+    .wLength       = 0
+  };
+
+  msc_csw_t const csw =
+  {
+    .signature    = MSC_CSW_SIGNATURE,
+    .tag          = 0xCAFECAFE,
+    .data_residue = total_bytes,
+    .status       = MSC_CSW_STATUS_FAILED
+  };
+
+  dcd_event_setup_received(rhport, (uint8_t const*) &request_clear_halt, false);
+  dcd_edpt_clear_stall_Expect(rhport, EDPT_MSC_IN);
+  dcd_edpt_xfer_ExpectWithArrayAndReturn(rhport, EDPT_MSC_IN, (uint8_t*) &csw, sizeof(msc_csw_t), sizeof(msc_csw_t), false, true);
+  dcd_edpt_xfer_ExpectAndReturn(rhport, EDPT_CTRL_IN, NULL, 0, false, true);
+
+  tud_task();
+}

test/unit-test/test/device/usbd/test_usbd.c

@@ -563,3 +563,52 @@ void test_usbd_stream_write_zlp_after_full_packet(void)
   dcd_edpt_xfer_ExpectAndReturn(rhport, 0x82, NULL, 0, false, true);
   TEST_ASSERT_TRUE(tu_edpt_stream_write_zlp_if_needed(&stream, 64));
 }
+
+//--------------------------------------------------------------------+
+// Endpoint stream wide requests
+//--------------------------------------------------------------------+
+
+// The stream API takes a uint32_t request but the fifo API is uint16_t: narrowed, 65536 becomes 0
+// and 65537 becomes 1, so the request must be clamped to UINT16_MAX first. A closed stream
+// (ep_addr 0) never claims an endpoint, so no DCD expectation is needed.
+enum { WIDE_REQ_MAX = 65537 };
+static uint8_t wide_src[WIDE_REQ_MAX];
+static uint8_t wide_dst[WIDE_REQ_MAX];
+
+static void wide_stream_setup(tu_edpt_stream_t *stream, bool overwritable, uint8_t *ff_buf) {
+  memset(stream, 0, sizeof(*stream));
+  TEST_ASSERT_TRUE(tu_edpt_stream_init(stream, false, true, overwritable, ff_buf, 64, NULL));
+}
+
+void test_usbd_stream_wide_request(void)
+{
+  static const uint32_t sizes[] = { 65535, 65536, WIDE_REQ_MAX };
+  uint8_t ff_buf[64];
+  tu_edpt_stream_t stream;
+
+  for (uint32_t i = 0; i < WIDE_REQ_MAX; i++) {
+    wide_src[i] = (uint8_t) (i * 7u + 3u);
+  }
+
+  for (size_t i = 0; i < TU_ARRAY_SIZE(sizes); i++) {
+    // write: the fifo takes the first 64 bytes of the request
+    wide_stream_setup(&stream, false, ff_buf);
+    TEST_ASSERT_EQUAL_UINT32(64, tu_edpt_stream_write(&stream, wide_src, sizes[i]));
+    TEST_ASSERT_EQUAL_UINT16(64, tu_fifo_count(&stream.ff));
+    TEST_ASSERT_EQUAL_MEMORY(wide_src, ff_buf, 64);
+
+    // read: the 64 queued bytes come out and nothing past them is written
+    memset(wide_dst, 0xA5, sizeof(wide_dst));
+    TEST_ASSERT_EQUAL_UINT32(64, tu_edpt_stream_read(&stream, wide_dst, sizes[i]));
+    TEST_ASSERT_EQUAL_UINT16(0, tu_fifo_count(&stream.ff));
+    TEST_ASSERT_EQUAL_MEMORY(wide_src, wide_dst, 64);
+    TEST_ASSERT_EQUAL_UINT8(0xA5, wide_dst[64]);
+  }
+
+  // overwritable: the fifo keeps the tail of the clamped 65535-byte request, not of the whole one
+  wide_stream_setup(&stream, true, ff_buf);
+  TEST_ASSERT_EQUAL_UINT32(64, tu_edpt_stream_write(&stream, wide_src, 65536));
+  TEST_ASSERT_EQUAL_UINT16(64, tu_fifo_count(&stream.ff));
+  TEST_ASSERT_EQUAL_UINT32(64, tu_edpt_stream_read(&stream, wide_dst, 64));
+  TEST_ASSERT_EQUAL_MEMORY(wide_src + 65535 - 64, wide_dst, 64);
+}