[PATCH master 0/2] mci: sdhci: fix memory corruption on DMA

[PATCH master 0/2] mci: sdhci: fix memory corruption on DMA

[Ahmad Fatoum]

Recent changes to teach SDHCI 64-bit support inadvertently changed
the address used for cache maintenance away from the DMA buffer address
with the result that unrelated cache lines were dropped and memory
corruption. This series fixes this. I must admit I don't understand
how the SDMA boundary mechanism works, so I did not change too much
about it.

Ahmad Fatoum (2):
  mci: sdhci: unmap the DMA buffers actually used
  mci: sdhci: hardcode SDMA boundary for DMA

 drivers/mci/sdhci.c | 9 ++-------
 drivers/mci/sdhci.h | 3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.39.2

[PATCH master 1/2] mci: sdhci: unmap the DMA buffers actually used

[Ahmad Fatoum]

At the end of sdhci_transfer_data_dma, sdhci_set_sdma_addr is called to
set the next DMA address. Recently, the computation of the next DMA
address was changed and instead of storing the next SDMA address into a
dedicated local variable as before, it was stored into the existing `dma'
variable. The dma variable is passed later though to dma_unmap_single(),
so clobbering it results in a loss of cache coherency and thus potential
memory corruption.

It's worth noting that this next SDMA address is not actually used for
DMA: Like Linux, barebox doesn't make use of this feature to chain (?) DMA
requests, so we actually invalidated memory buffers that were never used
for DMA.

Fixes: 76aa243aad95 ("mci: sdhci: Add 64-bit DMA addressing suport for V4 mode")
Fixes: 88f101358167 ("mci: sdhci: Force DMA update to the next block boundary")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/sdhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index 9829a78cb6c5..ef36a9c1b38a 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -295,14 +295,14 @@ int sdhci_transfer_data_dma(struct sdhci *sdhci, struct mci_data *data,
 			int boundary_cfg = (sdhci->sdma_boundary >> 12) & 0x7;
 			dma_addr_t boundary_size = 4096 << boundary_cfg;
 			/* Force update to the next DMA block boundary. */
-			dma = (dma & ~(boundary_size - 1)) + boundary_size;
+			dma_addr_t next = (dma & ~(boundary_size - 1)) + boundary_size;
 
 			/*
 			 * DMA engine has stopped on buffer boundary. Acknowledge
 			 * the interrupt and kick the DMA engine again.
 			 */
 			sdhci_write32(sdhci, SDHCI_INT_STATUS, SDHCI_INT_DMA);
-			sdhci_set_sdma_addr(sdhci, dma);
+			sdhci_set_sdma_addr(sdhci, next);
 		}
 
 		if (irqstat & SDHCI_INT_XFER_COMPLETE)
-- 
2.39.2

[PATCH master 2/2] mci: sdhci: hardcode SDMA boundary for DMA

[Ahmad Fatoum]

Given that we are not using the feature, there's no reason to compute
the boundary when polling for DMA completion. Therefore let's assume
the initial default of 512K remains unchanged. The sdhci::sdma_boundary
member is still left in place as the i.MX eSDHC driver sets it to zero
to affect what sdhci_setup_data_pio() writes into SDHCI_BLOCK_SIZE as
the i.MX repurposes doesn't implement the boundary feature and reserves
the bits in the SDHCI_BLOCK_SIZE register.

Fixes: 88f101358167 ("mci: sdhci: Force DMA update to the next block boundary")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/sdhci.c | 9 ++-------
 drivers/mci/sdhci.h | 3 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index ef36a9c1b38a..6ad11b8099fa 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -292,17 +292,12 @@ int sdhci_transfer_data_dma(struct sdhci *sdhci, struct mci_data *data,
 		 * some controllers are faulty, don't trust them.
 		 */
 		if (irqstat & SDHCI_INT_DMA) {
-			int boundary_cfg = (sdhci->sdma_boundary >> 12) & 0x7;
-			dma_addr_t boundary_size = 4096 << boundary_cfg;
-			/* Force update to the next DMA block boundary. */
-			dma_addr_t next = (dma & ~(boundary_size - 1)) + boundary_size;
-
 			/*
 			 * DMA engine has stopped on buffer boundary. Acknowledge
 			 * the interrupt and kick the DMA engine again.
 			 */
 			sdhci_write32(sdhci, SDHCI_INT_STATUS, SDHCI_INT_DMA);
-			sdhci_set_sdma_addr(sdhci, next);
+			sdhci_set_sdma_addr(sdhci, ALIGN(dma, SDHCI_DEFAULT_BOUNDARY_SIZE));
 		}
 
 		if (irqstat & SDHCI_INT_XFER_COMPLETE)
@@ -671,7 +666,7 @@ int sdhci_setup_host(struct sdhci *host)
 	if (host->caps & SDHCI_CAN_DO_8BIT)
 		mci->host_caps |= MMC_CAP_8_BIT_DATA;
 
-	host->sdma_boundary = SDHCI_DMA_BOUNDARY_512K;
+	host->sdma_boundary = SDHCI_DEFAULT_BOUNDARY_ARG;
 
 	if (sdhci_can_64bit_dma(host))
 		host->flags |= SDHCI_USE_64_BIT_DMA;
diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
index f3ffd62dff18..b5e6a7619d75 100644
--- a/drivers/mci/sdhci.h
+++ b/drivers/mci/sdhci.h
@@ -5,6 +5,7 @@
 #include <pbl.h>
 #include <dma.h>
 #include <linux/iopoll.h>
+#include <linux/sizes.h>
 
 #define SDHCI_DMA_ADDRESS					0x00
 #define SDHCI_BLOCK_SIZE__BLOCK_COUNT				0x04
@@ -18,6 +19,8 @@
 #define  SDHCI_DMA_BOUNDARY_8K			SDHCI_DMA_BOUNDARY(1)
 #define  SDHCI_DMA_BOUNDARY_4K			SDHCI_DMA_BOUNDARY(0)
 #define  SDHCI_DMA_BOUNDARY(x)			(((x) & 0x7) << 12)
+#define  SDHCI_DEFAULT_BOUNDARY_SIZE		SZ_512K
+#define  SDHCI_DEFAULT_BOUNDARY_ARG		SDHCI_DMA_BOUNDARY_512K
 #define  SDHCI_TRANSFER_BLOCK_SIZE(x)		((x) & 0xfff)
 #define SDHCI_BLOCK_COUNT					0x06
 #define SDHCI_ARGUMENT						0x08
-- 
2.39.2

Re: [PATCH master 0/2] mci: sdhci: fix memory corruption on DMA

[Sascha Hauer]

On Mon, Sep 11, 2023 at 02:11:54PM +0200, Ahmad Fatoum wrote:
> Recent changes to teach SDHCI 64-bit support inadvertently changed
> the address used for cache maintenance away from the DMA buffer address
> with the result that unrelated cache lines were dropped and memory
> corruption. This series fixes this. I must admit I don't understand
> how the SDMA boundary mechanism works, so I did not change too much
> about it.
> 
> Ahmad Fatoum (2):
>   mci: sdhci: unmap the DMA buffers actually used
>   mci: sdhci: hardcode SDMA boundary for DMA

Applied, thanks

Sascha

> 
>  drivers/mci/sdhci.c | 9 ++-------
>  drivers/mci/sdhci.h | 3 +++
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |