[PATCH] mmc: sdhci: Fix potential ADMA descriptor table overflow

[PATCH] mmc: sdhci: Fix potential ADMA descriptor table overflow

[Greg Malysa]

From: Ian Roberts <ian.roberts@timesys.com>

Change ADMA_TABLE_NO_ENTRIES to round the division up to fully
contain CONFIG_SYS_MMC_MAX_BLK_COUNT, fixing potential buffer overflow
of the ADMA descriptor table.

sdhci_prepare_adma_table() expecitily states it does _not_ check for
overflow as the descriptor table size is dependent on
CONFIG_SYS_MMC_MAX_BLK_COUNT. However, the ADMA_TABLE_NO_ENTRIES
calculation does not round up the divison, so with the current u-boot
 defaults:
max_mmc_transfer = (CONFIG_SYS_MMC_MAX_BLK_COUNT * MMC_MAX_BLOCK_LEN) =
65535 * 512 = 33553920 bytes.
ADMA_TABLE_NO_ENTRIES = max_mmc_transfer / ADMA_MAX_LEN =
33553920 / 65532, which does not divide cleanly.
actual_max_transfer = ADMA_TABLE_NO_ENTRIES * ADMA_MAX_LEN = 512 *
65532 = 33552384, which is smaller than max_mmc_transfer.
This can cause sdhci_prepare_adma_table() to write one extra
descriptor, overflowing the table when a transaction larger than
actual_max_transfer is issued.

Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
Signed-off-by: Ian Roberts <ian.roberts@timesys.com>

---


---
 include/sdhci.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/sdhci.h b/include/sdhci.h
index a1b74e3bd7..fbc0f0391c 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -11,6 +11,7 @@
 
 #include <linux/bitops.h>
 #include <linux/types.h>
+#include <linux/kernel.h>
 #include <asm/io.h>
 #include <mmc.h>
 #include <asm/gpio.h>
@@ -299,8 +300,8 @@ struct sdhci_ops {
 #else
 #define ADMA_DESC_LEN	8
 #endif
-#define ADMA_TABLE_NO_ENTRIES (CONFIG_SYS_MMC_MAX_BLK_COUNT * \
-			       MMC_MAX_BLOCK_LEN) / ADMA_MAX_LEN
+#define ADMA_TABLE_NO_ENTRIES DIV_ROUND_UP(CONFIG_SYS_MMC_MAX_BLK_COUNT * \
+			      MMC_MAX_BLOCK_LEN, ADMA_MAX_LEN)
 
 #define ADMA_TABLE_SZ (ADMA_TABLE_NO_ENTRIES * ADMA_DESC_LEN)
 
-- 
2.43.2

RE: [PATCH] mmc: sdhci: Fix potential ADMA descriptor table overflow

[Jaehoon Chung]

> -----Original Message-----
> From: Greg Malysa <greg.malysa@timesys.com>
> Sent: Tuesday, March 26, 2024 11:23 AM
> To: u-boot@lists.denx.de; Peng Fan <peng.fan@nxp.com>; Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Ian Roberts <ian.roberts@timesys.com>; Nathan Barrett-Morrison <nathan.morrison@timesys.com>; Greg
> Malysa <greg.malysa@timesys.com>; Sean Anderson <sean.anderson@seco.com>; Tom Rini
> <trini@konsulko.com>
> Subject: [PATCH] mmc: sdhci: Fix potential ADMA descriptor table overflow
> 
> From: Ian Roberts <ian.roberts@timesys.com>
> 
> Change ADMA_TABLE_NO_ENTRIES to round the division up to fully
> contain CONFIG_SYS_MMC_MAX_BLK_COUNT, fixing potential buffer overflow
> of the ADMA descriptor table.
> 
> sdhci_prepare_adma_table() expecitily states it does _not_ check for
> overflow as the descriptor table size is dependent on
> CONFIG_SYS_MMC_MAX_BLK_COUNT. However, the ADMA_TABLE_NO_ENTRIES
> calculation does not round up the divison, so with the current u-boot
>  defaults:
> max_mmc_transfer = (CONFIG_SYS_MMC_MAX_BLK_COUNT * MMC_MAX_BLOCK_LEN) =
> 65535 * 512 = 33553920 bytes.
> ADMA_TABLE_NO_ENTRIES = max_mmc_transfer / ADMA_MAX_LEN =
> 33553920 / 65532, which does not divide cleanly.
> actual_max_transfer = ADMA_TABLE_NO_ENTRIES * ADMA_MAX_LEN = 512 *
> 65532 = 33552384, which is smaller than max_mmc_transfer.
> This can cause sdhci_prepare_adma_table() to write one extra
> descriptor, overflowing the table when a transaction larger than
> actual_max_transfer is issued.
> 
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Ian Roberts <ian.roberts@timesys.com>


Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> 

Best Regards,
Jaehoon Chung

> 
> ---
> 
> 
> ---
>  include/sdhci.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sdhci.h b/include/sdhci.h
> index a1b74e3bd7..fbc0f0391c 100644
> --- a/include/sdhci.h
> +++ b/include/sdhci.h
> @@ -11,6 +11,7 @@
> 
>  #include <linux/bitops.h>
>  #include <linux/types.h>
> +#include <linux/kernel.h>
>  #include <asm/io.h>
>  #include <mmc.h>
>  #include <asm/gpio.h>
> @@ -299,8 +300,8 @@ struct sdhci_ops {
>  #else
>  #define ADMA_DESC_LEN	8
>  #endif
> -#define ADMA_TABLE_NO_ENTRIES (CONFIG_SYS_MMC_MAX_BLK_COUNT * \
> -			       MMC_MAX_BLOCK_LEN) / ADMA_MAX_LEN
> +#define ADMA_TABLE_NO_ENTRIES DIV_ROUND_UP(CONFIG_SYS_MMC_MAX_BLK_COUNT * \
> +			      MMC_MAX_BLOCK_LEN, ADMA_MAX_LEN)
> 
>  #define ADMA_TABLE_SZ (ADMA_TABLE_NO_ENTRIES * ADMA_DESC_LEN)
> 
> --
> 2.43.2