Memory corruption on USDHC non-DMA transfer when D-Cache enabled

[tchebb]

The USDHC data interrupt handler, USDHC_TransferHandleData(), contains an unconditional D-Cache invalidate on completed USDHC transfer, presumably to ensure the CPU sees data written by DMA if the line was pulled back into the cache since the clean+invalidate in USDHC_TransferNonBlocking():

mcuxsdk-core/drivers/usdhc/fsl_usdhc.c

Lines 2358 to 2364 in 3b91d61

	#if defined(FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL) && FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL
	if (handle->data->rxData != NULL)
	{
	DCACHE_InvalidateByRange((uintptr_t)(handle->data->rxData),
	(handle->data->blockSize) * (handle->data->blockCount));
	}
	#endif

But that causes significant memory unsafety: if the transfer did not use DMA, the transfer function never cleaned it to begin with, meaning the new invalidate discards writes elsewhere in that line from an arbitrarily long period prior to the transfer! This happens when the if branch here is taken:

mcuxsdk-core/drivers/usdhc/fsl_usdhc.c

Lines 1929 to 1950 in 3b91d61

	/* if the DMA desciptor configure fail or not needed , disable it */
	if (error != kStatus_Success)
	{
	/* disable DMA, using polling mode in this situation */
	USDHC_EnableInternalDMA(base, false);
	enDMA = false;
	}
	#if defined(FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL) && FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL
	else
	{
	if (data->txData != NULL)
	{
	/* clear the DCACHE */
	DCACHE_CleanByRange((uintptr_t)data->txData, (data->blockSize) * (data->blockCount));
	}
	else
	{
	/* clear the DCACHE */
	DCACHE_CleanInvalidateByRange((uintptr_t)data->rxData, (data->blockSize) * (data->blockCount));
	}
	}
	#endif

In fact, I don't believe the invalidate is safe even when DMA is used: I don't see any requirement that DMA buffers be the only thing in their cache line, so it may still discard writes, just over a bounded time period.

I have a 100% reproducible crash in Zephyr due to stack corruption caused by this bug when using the Infineon AIROC Wi-Fi driver on an i.MX RT1061.

[yvesll]

@mcuxted can you take a look?

[mcuxted]

Hello @yangbolu1991 , could you please help usdhc? thanks.

[yangbolu1991]

@tchebb I'm not sure what you meant by mentioning USDHC_TransferNonBlocking(), and non-DMA.
The Commit 3b91d61 you mentioned was using DCACHE_InvalidateByRange in USDHC_TransferDataBlocking() for only DMA RX (under if (enDMA) and if (data->rxData != NULL)).
Will this cause problem?
Thanks.

[tchebb]

Wow, I really messed up this report. There is a bug, but it's not from commit 3b91d61, it's not a recent regression (as far as I can tell), and I linked the wrong code snippet. I've edited my original comment to hopefully now be accurate.

The failing code path is follows:

1. USDHC_TransferNonBlocking() is called for an RX transfer with FSL_USDHC_ENABLE_SCATTER_GATHER_TRANSFER unset and FSL_SDK_ENABLE_DRIVER_CACHE_CONTROL set.
2. No DMA config is passed or USDHC_SetAdmaTableConfig() fails.
3. USDHC_TransferNonBlocking() skips calling DCACHE_CleanInvalidateByRange() because DMA is not in use.
4. USDHC_TransferNonBlocking() calls USDHC_SendCommand() to kick off the transfer.
5. The transfer completes and hardware calls USDHC_TransferHandleIRQ(), which calls USDHC_TransferHandleData().
6. USDHC_TransferHandleData() sees kUSDHC_DataCompleteFlag and calls DCACHE_InvalidateByRange() on handle->data->rxData, discarding dirty data and causing memory corruption.

[yangbolu1991]

@tchebb I see...
Going through usdhc driver and sdmmc stack in SDK, I think there are two sets of cache management.
One is in usdhc driver, and the other one is in sdmmc stack.

Maybe sdk examples are using cache management in sdmmc stack with usdhc cache management disabled.
https://github.com/nxp-mcuxpresso/mcuxsdk-middleware-sdmmc/blob/mcux_main/host/usdhc/fsl_sdmmc_host.h#L207

For zephyr, it has own sdmmc stack and only usdhc drivers used.
I think this issue should be handled very carefully in SDK in case much platforms are affected. And it may be not easy to do effective test. This may need more investigation and a fix plan.

Thanks.

[tchebb]

I've posted #25 to fix the worst of this behavior (invalidating the cache when DMA isn't in use at all).

It doesn't fix this issue,

In fact, I don't believe the invalidate is safe even when DMA is used: I don't see any requirement that DMA buffers be the only thing in their cache line, so it may still discard writes, just over a bounded time period.

but after reading more Zephyr code and comparing with other vendors' SDKs, I now believe that would be user error. The SDK can't know if there's unrelated data in the buffer's cache line, so it's up to the caller to ensure there isn't.

The Zephyr AIROC Wi-Fi driver doesn't put buffers in their own cache lines, so I'll need to fix that too.

[yangbolu1991]

Thanks a lot. @tchebb
I will cherry-pick the fix-up into NXP internal repo and make sure it's in next release.
Then do we need to keep this ticket open?

[tchebb]

No, you can close this once it's released. Thanks!

[yangbolu1991]

Patch will be in next release on 18/May/26.
Thanks.

[yangbolu1991]

We can close it as patch integrated.
296b3d3

Thanks.

[tchebb]

Will it ever be on the main branch? Or is that not used for new development?