usb: device: msc: usb_dc_stm32_isr causing memory data access violation fault

[akaiserUS]

Describe the bug

When usb_dc_stm32_isr is invoked and performs a USB DC Reset (by calling msd_init()) it clears the curr_offset to zero. If this ISR is activated after memoryWrite() has been called and has updated curr_offset to a block size (thus thread_op will be set to THREAD_OP_WRITE_QUEUED), then when thread_memory_write_done() is invoked the curr_offset will be reset to 0 which will result in overflowed_len being set to a negative number. Since it is treated as a unsigned integer it results in memmove attempting to move a very large block of data resulting in a memory access violation, and thus a fault and reset.

• Target: STM32F4112G
• Workaround:

	if (BLOCK_SIZE > (curr_offset + size)) {
		overflowed_len = 0;
	}

Commit that changed this behavior was: 6d4266a

To Reproduce

Not fully sure how to consistently reproduce, but the following may work often.

1. Build zephyr code for a board that has USB mass storage capabilities (I am using the STM32F4112G chipset).
2. Flash the board with the built code and use a debugger (I am using a Segger)
3. Put a breakpoint at the following lines of code:

thread_op = THREAD_OP_WRITE_QUEUED; /* write_queued */

in

static void memoryWrite(uint8_t *buf, uint16_t size)
{
	if (curr_lba >= block_count) {
		LOG_WRN("Attempt to write past end of device: lba=%u", curr_lba);
		fail();
		return;
	}

	/* we fill an array in RAM of 1 block before writing it in memory */
	for (int i = 0; i < size; i++) {
		page[curr_offset + i] = buf[i];
	}

	/* if the array is filled, write it in memory */
	if (curr_offset + size >= BLOCK_SIZE) {
		if (!(disk_access_status(disk_pdrv) & DISK_STATUS_WR_PROTECT)) {
			LOG_DBG("Disk WRITE Qd %u", curr_lba);
			thread_op = THREAD_OP_WRITE_QUEUED; /* write_queued */
			defered_wr_sz = size;
			k_sem_give(&disk_wait_sem);
			return;
		}
	}

	curr_offset += size;
	length -= size;
	csw.DataResidue -= size;

	if ((!length) || (stage != MSC_PROCESS_CBW)) {
		csw.Status = (stage == MSC_ERROR) ? CSW_FAILED : CSW_PASSED;
		sendCSW();
	}
}

and

curr_offset = 0U;

in

static void msd_init(void)
{
	(void)memset((void *)&cbw, 0, sizeof(struct CBW));
	(void)memset((void *)&csw, 0, sizeof(struct CSW));
	(void)memset(page, 0, sizeof(page));
	curr_lba = 0U;
	length = 0U;
	curr_offset = 0U;
}

and

memmove(page, &page[BLOCK_SIZE], overflowed_len);

in

static void thread_memory_write_done(void)
{
	uint32_t size = defered_wr_sz;
	size_t overflowed_len = (curr_offset + size) - BLOCK_SIZE;

	if (overflowed_len > 0) {
		memmove(page, &page[BLOCK_SIZE], overflowed_len);
	}

	curr_offset = overflowed_len;
	curr_lba += 1;
	length -= size;
	csw.DataResidue -= size;

	if (!length) {
		if (disk_access_ioctl(disk_pdrv, DISK_IOCTL_CTRL_SYNC, NULL)) {
			LOG_ERR("!! Disk cache sync error !!");
		}
	}

	if ((!length) || (stage != MSC_PROCESS_CBW)) {
		csw.Status = (stage == MSC_ERROR) ? CSW_FAILED : CSW_PASSED;
		sendCSW();
	}

	thread_op = THREAD_OP_WRITE_DONE;

	usb_ep_read_continue(mass_ep_data[MSD_OUT_EP_IDX].ep_addr);
}

When the breakpoint at

thread_op = THREAD_OP_WRITE_QUEUED; /* write_queued */

is reached wait about 20 seconds, before pressing play. (I have also the USB Console enabled and I am seeing the console on my computer show USB disconnected before continuing, perhaps this is what is invoking the USB ISR).

After resuming the reset should occur by hitting the breakpoint

curr_offset = 0U;

followed by

memmove(page, &page[BLOCK_SIZE], overflowed_len);

where overflowed_len should be a very large value, resulting in a memory access violation.

Stacks at the breakpoints are:

usb_dc_stm32_isr -> HAL_PCD_IRQHandler -> PCD_EP_OutXfrComplete_init -> HAL_PCD_DataOutStageCallback -> mass_storage_bulk_out -> memoryWrite

usb_dc_stm32_isr -> HAL_PCD_IRQHandler -> HAL_PCD_ResetCallback -> forward_status_cb -> mass_storage_status_cb -> msd_init

z_thread_entry -> mass_thread_main -> thread_memory_write_done

Expected behavior

When the USB ISR is invoked, after the memoryWrite call sets up the buffer for a write of a block size or greater, but before the thread memory write done is invoked, thread memory write should not attempt to write a block size of an invalid length.

Impact

Minimal impact with the patch I added to my code base, but might not be best long term solution.

Environment (please complete the following information):

• OS: MacOS
• Toolchain: Zephyr SDK
• Version: v3.4.0

[tmon-nordic]

Minimal impact with the patch I added to my code base, but might not be best long term solution.

Could you please submit this patch with a PR? This looks good enough for me because the old stack MSC implementation is doomed anyway.

The long term solution is to switch to next USB stack which has completely new MSC implementation which properly synchronizes accesses (and also properly reports errors instead of hardcoding an response to make test suite pass).