| Message ID | 20190314223130.31802-3-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi: prevent overflow for max_req_size | expand |
Hi Wolfram, On Thu, Mar 14, 2019 at 11:35 PM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > max_req_size is calculated by 'max_blk_size * max_blk_count' in the TMIO > core. So, specifying U32_MAX as max_blk_count will overflow this > calculation. It will cause no harm in practice because the immense high > number will overflow into another immense high number. However, it is > not good coding practice, so calculate max_blk_count so that > max_req_size will fit into unsigned int on ARM32/64. > > Thanks to the Renesas BSP team for the bug report! > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for your patch! > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > @@ -95,8 +95,8 @@ static const struct renesas_sdhi_of_data of_rza2_compatible = { > .scc_offset = 0 - 0x1000, > .taps = rcar_gen3_scc_taps, > .taps_num = ARRAY_SIZE(rcar_gen3_scc_taps), > - /* DMAC can handle 0xffffffff blk count but only 1 segment */ > - .max_blk_count = 0xffffffff, > + /* DMAC can handle 32bit blk count but only 1 segment */ > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, I have mixed feelings about this change: 1. You're lying about the actual maximum (yes, there's a comment that mentions the real limit), 2. This fixes the problem in this single (set of) drivers only, while about every other driver (not the mmc core) calculates "max_blk_size * max_blk_count", too. 3. Some drivers use different limits (e.g. 2048, 4095, or 4096), so eventually having a common upper limit is not easy. BTW, drivers/mmc/host/usdhi6rol0.c does it the other way around: mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size; Gr{oetje,eeting}s, Geert
Hi Geert, > > - /* DMAC can handle 0xffffffff blk count but only 1 segment */ > > - .max_blk_count = 0xffffffff, > > + /* DMAC can handle 32bit blk count but only 1 segment */ > > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, > > I have mixed feelings about this change: > 1. You're lying about the actual maximum (yes, there's a comment that > mentions the real limit), We can't utilize the actual maximum without converting the MMC core to u64. Given that the above max_blk_count is still way beyond any practical value, I am OK with the above. > 2. This fixes the problem in this single (set of) drivers only, while about > every other driver (not the mmc core) calculates > "max_blk_size * max_blk_count", too. I glimpsed, too, and found various patterns. We could maybe add a warning to the MMC core, but other than that I fail to see a way to handle it in a generic way. I'll think about it some more. Or do you have an idea already? > 3. Some drivers use different limits (e.g. 2048, 4095, or 4096), so > eventually having a common upper limit is not easy. I don't understand this one. Which limit do you mean here? blk_size? > BTW, drivers/mmc/host/usdhi6rol0.c does it the other way around: > > mmc->max_blk_count = mmc->max_req_size / mmc->max_blk_size; I think we can't do this because of older SDHI instances. max_req_size is still 32 bit for them, but their blk_count register is only 16 bit. Thanks, Wolfram
Hi Wolfram, On Fri, Mar 15, 2019 at 9:29 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > - /* DMAC can handle 0xffffffff blk count but only 1 segment */ > > > - .max_blk_count = 0xffffffff, > > > + /* DMAC can handle 32bit blk count but only 1 segment */ > > > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, > > > > I have mixed feelings about this change: > > 1. You're lying about the actual maximum (yes, there's a comment that > > mentions the real limit), > > We can't utilize the actual maximum without converting the MMC core to > u64. Given that the above max_blk_count is still way beyond any > practical value, I am OK with the above. > > > 2. This fixes the problem in this single (set of) drivers only, while about > > every other driver (not the mmc core) calculates > > "max_blk_size * max_blk_count", too. > > I glimpsed, too, and found various patterns. We could maybe add a > warning to the MMC core, but other than that I fail to see a way to > handle it in a generic way. I'll think about it some more. Or do you > have an idea already? No idea, else I would have told you ;-) > > 3. Some drivers use different limits (e.g. 2048, 4095, or 4096), so > > eventually having a common upper limit is not easy. > > I don't understand this one. Which limit do you mean here? blk_size? Yes, blk_size. Gr{oetje,eeting}s, Geert
> > > 3. Some drivers use different limits (e.g. 2048, 4095, or 4096), so > > > eventually having a common upper limit is not easy. > > > > I don't understand this one. Which limit do you mean here? blk_size? > > Yes, blk_size. I am still confused. Which upper limit do you mean then? Because for blk_size and blk_count, they are both driver specific, or?
Hi Wolfram, On Fri, Mar 15, 2019 at 10:28 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > 3. Some drivers use different limits (e.g. 2048, 4095, or 4096), so > > > > eventually having a common upper limit is not easy. > > > > > > I don't understand this one. Which limit do you mean here? blk_size? > > > > Yes, blk_size. > > I am still confused. Which upper limit do you mean then? Because for TMIO_MAX_BLK_SIZE > blk_size and blk_count, they are both driver specific, or? Yes, they are driver-specific. So you cannot use a common upper limit. Gr{oetje,eeting}s, Geert
> > I am still confused. Which upper limit do you mean then? Because for > > TMIO_MAX_BLK_SIZE > > > blk_size and blk_count, they are both driver specific, or? > > Yes, they are driver-specific. So you cannot use a common upper limit. And this is why I think my patch has a valid approach. But I am still not sure I am getting your point.
Hi Wolfram, On Fri, Mar 15, 2019 at 10:39 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > > I am still confused. Which upper limit do you mean then? Because for > > > > TMIO_MAX_BLK_SIZE > > > > > blk_size and blk_count, they are both driver specific, or? > > > > Yes, they are driver-specific. So you cannot use a common upper limit. > > And this is why I think my patch has a valid approach. But I am still > not sure I am getting your point. My worry here is that if several drivers would do .max_blk_count = UINT_MAX / DRIVER_SPECIFIC_MAX_BLK_SIZE, Those drivers can safely calculate "max_blk_size * max_blk_count". People may start assuming this is always safe. while the core cannot do "GLOBAL_MAX_BLK_SIZE * max_blk_count" using 32-bit arithmetic, as the latter may still overflow, depending on the driver. But perhaps I'm just too overcautious... Gr{oetje,eeting}s, Geert
> My worry here is that if several drivers would do > > .max_blk_count = UINT_MAX / DRIVER_SPECIFIC_MAX_BLK_SIZE, > > Those drivers can safely calculate "max_blk_size * max_blk_count". > People may start assuming this is always safe. while the core cannot do > "GLOBAL_MAX_BLK_SIZE * max_blk_count" using 32-bit arithmetic, as the > latter may still overflow, depending on the driver. The core will not do this calculation. This is why the variable max_req_size exists. It will just use this variable which shall be setup by the driver which may impose other restrictions, too (And there can't be a GLOBAL_MAX_BLK_SIZE anyhow) So, when calculating max_req_size, it is the duty of the driver not to overflow. Or?
On Thu, 14 Mar 2019 at 23:32, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > max_req_size is calculated by 'max_blk_size * max_blk_count' in the TMIO > core. So, specifying U32_MAX as max_blk_count will overflow this > calculation. It will cause no harm in practice because the immense high > number will overflow into another immense high number. However, it is > not good coding practice, so calculate max_blk_count so that > max_req_size will fit into unsigned int on ARM32/64. > > Thanks to the Renesas BSP team for the bug report! > > Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Applied for next, thanks! Kind regards Uffe > --- > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 8 ++++---- > drivers/mmc/host/renesas_sdhi_sys_dmac.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > index 9dfafa2a90a3..af0288f04200 100644 > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > @@ -95,8 +95,8 @@ static const struct renesas_sdhi_of_data of_rza2_compatible = { > .scc_offset = 0 - 0x1000, > .taps = rcar_gen3_scc_taps, > .taps_num = ARRAY_SIZE(rcar_gen3_scc_taps), > - /* DMAC can handle 0xffffffff blk count but only 1 segment */ > - .max_blk_count = 0xffffffff, > + /* DMAC can handle 32bit blk count but only 1 segment */ > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, > .max_segs = 1, > }; > > @@ -110,8 +110,8 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = { > .scc_offset = 0x1000, > .taps = rcar_gen3_scc_taps, > .taps_num = ARRAY_SIZE(rcar_gen3_scc_taps), > - /* DMAC can handle 0xffffffff blk count but only 1 segment */ > - .max_blk_count = 0xffffffff, > + /* DMAC can handle 32bit blk count but only 1 segment */ > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, > .max_segs = 1, > }; > > diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c > index 02cd878e209f..bfbf36634faa 100644 > --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c > +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c > @@ -65,7 +65,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = { > .scc_offset = 0x0300, > .taps = rcar_gen2_scc_taps, > .taps_num = ARRAY_SIZE(rcar_gen2_scc_taps), > - .max_blk_count = 0xffffffff, > + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, > }; > > /* Definitions for sampling clocks */ > -- > 2.11.0 >
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c index 9dfafa2a90a3..af0288f04200 100644 --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c @@ -95,8 +95,8 @@ static const struct renesas_sdhi_of_data of_rza2_compatible = { .scc_offset = 0 - 0x1000, .taps = rcar_gen3_scc_taps, .taps_num = ARRAY_SIZE(rcar_gen3_scc_taps), - /* DMAC can handle 0xffffffff blk count but only 1 segment */ - .max_blk_count = 0xffffffff, + /* DMAC can handle 32bit blk count but only 1 segment */ + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, .max_segs = 1, }; @@ -110,8 +110,8 @@ static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = { .scc_offset = 0x1000, .taps = rcar_gen3_scc_taps, .taps_num = ARRAY_SIZE(rcar_gen3_scc_taps), - /* DMAC can handle 0xffffffff blk count but only 1 segment */ - .max_blk_count = 0xffffffff, + /* DMAC can handle 32bit blk count but only 1 segment */ + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, .max_segs = 1, }; diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c index 02cd878e209f..bfbf36634faa 100644 --- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c +++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c @@ -65,7 +65,7 @@ static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = { .scc_offset = 0x0300, .taps = rcar_gen2_scc_taps, .taps_num = ARRAY_SIZE(rcar_gen2_scc_taps), - .max_blk_count = 0xffffffff, + .max_blk_count = UINT_MAX / TMIO_MAX_BLK_SIZE, }; /* Definitions for sampling clocks */
max_req_size is calculated by 'max_blk_size * max_blk_count' in the TMIO core. So, specifying U32_MAX as max_blk_count will overflow this calculation. It will cause no harm in practice because the immense high number will overflow into another immense high number. However, it is not good coding practice, so calculate max_blk_count so that max_req_size will fit into unsigned int on ARM32/64. Thanks to the Renesas BSP team for the bug report! Reported-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/host/renesas_sdhi_internal_dmac.c | 8 ++++---- drivers/mmc/host/renesas_sdhi_sys_dmac.c | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)