[PATCH 1/1] mmc: Avoid buffer overrun in mmc_startup()

[Dragan Simic]

On 2024-01-04 04:49, Heinrich Schuchardt wrote:
> If the CSD register contains a reserved value (4 - 7) in bits 0:2 of 
> the
> TRAN_SPEED field, a buffer overrun occurs. Resize the mapping table.
> 
> According to the original report
> https://lore.kernel.org/u-boot/20180826231332.2491-11-erosca@de.adit-jv.com/
> reserved values have been observed resulting in a buffer overrun.
> 
> Reported-by: Eugeniu Rosca <erosca at de.adit-jv.com>
> Fixes: 272cc70b211e ("Add MMC Framework")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
>  drivers/mmc/mmc.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index d96db7a0f8..00f4964aae 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1570,13 +1570,20 @@ static int sd_read_ssr(struct mmc *mmc)
>  	return 0;
>  }
>  #endif
> -/* frequency bases */
> -/* divided by 10 to be nice to platforms without floating point */
> +/*
> + * TRAN_SPEED bits 0:2 encode the frequency unit:
> + * 0 = 100KHz, 1 = 1MHz, 2 = 10MHz, 3 = 100MHz, values 4 - 7 are 
> reserved.
> + * The values in fbase[] are divided by 10 to avoid floats in 
> multiplier[].
> + */
>  static const int fbase[] = {
>  	10000,
>  	100000,
>  	1000000,
>  	10000000,
> +	0,	/* reserved */
> +	0,	/* reserved */
> +	0,	/* reserved */
> +	0,	/* reserved */
>  };

This makes me wonder what frequency unit(s) should actually be used when 
the reserved values are encountered?  What happens if zero is used in 
that case?  I had a brief look at drivers/mmc/mmc.c and using zeros 
doesn't seem like the best approach.

>  /* Multiplier values for TRAN_SPEED.  Multiplied by 10 to be nice
> @@ -2560,6 +2567,8 @@ static int mmc_startup(struct mmc *mmc)
>  	mult = multipliers[((cmd.response[0] >> 3) & 0xf)];
> 
>  	mmc->legacy_speed = freq * mult;
> +	if (!mmc->legacy_speed)
> +		log_debug("TRAN_SPEED: reserved value");
>  	mmc_select_mode(mmc, MMC_LEGACY);
> 
>  	mmc->dsr_imp = ((cmd.response[1] >> 12) & 0x1);