From 8e6b7f373eda9756abdd4e4103664e40a25ce25b Mon Sep 17 00:00:00 2001 From: Kartik Naik Date: Sat, 31 Oct 2026 17:51:21 +0530 Subject: [PATCH] msc device: check READ10/WRITE10 length against the LUN block size rdwr10_validate_cmd() only checked that the block size implied by the CBW, dCBWDataTransferLength / TransferLength, is non-zero and fits 16 bits. proc_read10_cmd() and proc_write10_host_data() then split the transfer into lba and offset with that host-chosen size, while the read10/write10 callbacks address the LUN in the block size reported by tud_msc_capacity_cb(). A WRITE10 of one 512-byte block with a 4096-byte data stage reaches tud_msc_write10_cb() with the same lba and offset 512..3584, and the cdc_msc example writes the host's data past the end of its RAM disk. Fail a READ10/WRITE10 whose transfer length is not block_count times the LUN's block size: a larger one with a stall and full residue (cases 5/11, as #3968 does for cases 4/9), a smaller one with phase error (cases 7/13). --- docs/reference/class/msc.rst | 3 + src/class/msc/msc_device.c | 17 +++++- .../test/device/msc/test_msc_device.c | 56 +++++++++++++++++++ 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/docs/reference/class/msc.rst b/docs/reference/class/msc.rst index 2b6933be38..390ac87717 100644 --- a/docs/reference/class/msc.rst +++ b/docs/reference/class/msc.rst @@ -55,6 +55,9 @@ and ``tud_msc_scsi_cb()``. The other callbacks below have weak defaults. Read and write callbacks may cover only part of a logical block; honor both ``lba`` and ``offset`` instead of assuming one callback per block. +A READ10/WRITE10 whose transfer length is not its block count times the block +size from ``tud_msc_capacity_cb()`` fails before reaching them, so ``offset`` +is always less than that block size. ``TUD_MSC_RET_ERROR`` from read10/write10 overwrites the stored sense with NOT READY / MEDIUM NOT PRESENT. For other command callbacks, such as ``tud_msc_scsi_cb()``, set useful sense data with ``tud_msc_set_sense()`` diff --git a/src/class/msc/msc_device.c b/src/class/msc/msc_device.c index 912a5c84ef..c13cffb7a9 100644 --- a/src/class/msc/msc_device.c +++ b/src/class/msc/msc_device.c @@ -174,7 +174,22 @@ static uint8_t rdwr10_validate_cmd(msc_cbw_t const* cbw) { TU_LOG_DRV(" Computed block size = 0. SCSI case 7 Hi < Di (READ10) or case 13 Ho < Do (WRIT10)\r\n"); status = MSC_CSW_STATUS_PHASE_ERROR; } else { - // nothing to do + // lba and offset passed to read10/write10 callbacks are in the LUN's block size, the transfer length + // must be exactly block_count of those blocks or offset runs past the end of the block + uint32_t lun_block_count = 0; + uint16_t lun_block_size = 0; + tud_msc_capacity_cb(cbw->lun, &lun_block_count, &lun_block_size); + uint32_t const lun_bytes = (uint32_t) block_count * lun_block_size; + + if (cbw->total_bytes > lun_bytes) { + TU_LOG_DRV(" SCSI case 5 Hi > Di (READ10) or case 11 Ho > Do (WRITE10)\r\n"); + status = MSC_CSW_STATUS_FAILED; + } else if (cbw->total_bytes < lun_bytes) { + TU_LOG_DRV(" SCSI case 7 Hi < Di (READ10) or case 13 Ho < Do (WRITE10)\r\n"); + status = MSC_CSW_STATUS_PHASE_ERROR; + } else { + // nothing to do + } } } diff --git a/test/unit-test/test/device/msc/test_msc_device.c b/test/unit-test/test/device/msc/test_msc_device.c index 8e4e9ba0c7..83d1ed1482 100644 --- a/test/unit-test/test/device/msc/test_msc_device.c +++ b/test/unit-test/test/device/msc/test_msc_device.c @@ -389,6 +389,62 @@ void test_msc_read10_block_size_over_16bit(void) tud_task(); } +// The transfer length must be block count x the LUN's block size: 1024 bytes for one 512-byte block would reach +// tud_msc_write10_cb() with offset 512, writing past the last block of the disk +void test_msc_write10_length_over_lun_block_size(void) +{ + uint32_t const total_bytes = 2 * DISK_BLOCK_SIZE; + + msc_cbw_t cbw_write10 = + { + .signature = MSC_CBW_SIGNATURE, + .tag = 0xCAFECAFE, + .total_bytes = total_bytes, + .lun = 0, + .dir = 0, + .cmd_len = sizeof(scsi_write10_t) + }; + + scsi_write10_t cmd_write10 = + { + .cmd_code = SCSI_CMD_WRITE_10, + .lba = tu_htonl(DISK_BLOCK_NUM - 1), + .block_count = tu_htons(1) + }; + + memcpy(cbw_write10.command, &cmd_write10, cbw_write10.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_write10, 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 11 (Ho > Do): no data, STALL Bulk-Out, then a failed CSW with full residue + msc_csw_t const csw = + { + .signature = MSC_CSW_SIGNATURE, + .tag = 0xCAFECAFE, + .data_residue = total_bytes, + .status = MSC_CSW_STATUS_FAILED + }; + + dcd_edpt_stall_Expect(rhport, EDPT_MSC_OUT); + dcd_edpt_xfer_ExpectWithArrayAndReturn(rhport, EDPT_MSC_IN, (uint8_t*) &csw, sizeof(msc_csw_t), sizeof(msc_csw_t), false, true); + + tud_task(); +} + //--------------------------------------------------------------------+ // SystemView spans on the early returns //--------------------------------------------------------------------+