| Message ID | 20240104034942.102500-1-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted |
| Commit | f9a86fb118530fea5a87dd7f88e90c31c989043f |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | [1/1] mmc: Avoid buffer overrun in mmc_startup() | expand |
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@de.adit-jv.com> > Fixes: 272cc70b211e ("Add MMC Framework") > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@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);
On 1/4/24 12: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@de.adit-jv.com> > Fixes: 272cc70b211e ("Add MMC Framework") > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> Best Regards, Jaehoon Chung > --- > 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 */ > }; > > /* 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);
On 1/4/24 12: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@de.adit-jv.com> > Fixes: 272cc70b211e ("Add MMC Framework") > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> Applied to u-boot-mmc/master. Best Regards, Jaehoon Chung > --- > 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 */ > }; > > /* 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);
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 */ }; /* 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);
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@de.adit-jv.com> Fixes: 272cc70b211e ("Add MMC Framework") Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- drivers/mmc/mmc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)