[RFC PATCH 0/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[RFC PATCH 0/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

Here is a small series fixing an 'unsigned int' overflow. I think the patch
descriptions say it all.

Tested on a Renesas Salvator XS board (R-Car M3N) with no regressions.

Sent as RFC to give Shimoda-san some time to approve this approach.

Thanks!

   Wolfram


Wolfram Sang (2):
  mmc: tmio: introduce macro for max block size
  mmc: renesas_sdhi: prevent overflow for max_req_size

 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 8 ++++----
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 2 +-
 drivers/mmc/host/tmio_mmc.h                   | 2 ++
 drivers/mmc/host/tmio_mmc_core.c              | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

-- 
2.11.0

[RFC PATCH 1/2] mmc: tmio: introduce macro for max block size

[Wolfram Sang]

We will need it later for other calculations.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/mmc/host/tmio_mmc.h      | 2 ++
 drivers/mmc/host/tmio_mmc_core.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 2adb0d24360f..64e00d8f3a88 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -105,6 +105,8 @@
 		TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT)
 #define TMIO_MASK_IRQ     (TMIO_MASK_READOP | TMIO_MASK_WRITEOP | TMIO_MASK_CMD)
 
+#define TMIO_MAX_BLK_SIZE 512
+
 struct tmio_mmc_data;
 struct tmio_mmc_host;
 
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 595949f1f001..dfc15ce41191 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1186,7 +1186,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
 	mmc->caps2 |= pdata->capabilities2;
 	mmc->max_segs = pdata->max_segs ? : 32;
-	mmc->max_blk_size = 512;
+	mmc->max_blk_size = TMIO_MAX_BLK_SIZE;
 	mmc->max_blk_count = pdata->max_blk_count ? :
 		(PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs;
 	mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
-- 
2.11.0

Re: [RFC PATCH 1/2] mmc: tmio: introduce macro for max block size

[Ulf Hansson]

On Thu, 14 Mar 2019 at 23:32, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> We will need it later for other calculations.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/tmio_mmc.h      | 2 ++
>  drivers/mmc/host/tmio_mmc_core.c | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
> index 2adb0d24360f..64e00d8f3a88 100644
> --- a/drivers/mmc/host/tmio_mmc.h
> +++ b/drivers/mmc/host/tmio_mmc.h
> @@ -105,6 +105,8 @@
>                 TMIO_STAT_CARD_REMOVE | TMIO_STAT_CARD_INSERT)
>  #define TMIO_MASK_IRQ     (TMIO_MASK_READOP | TMIO_MASK_WRITEOP | TMIO_MASK_CMD)
>
> +#define TMIO_MAX_BLK_SIZE 512
> +
>  struct tmio_mmc_data;
>  struct tmio_mmc_host;
>
> diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
> index 595949f1f001..dfc15ce41191 100644
> --- a/drivers/mmc/host/tmio_mmc_core.c
> +++ b/drivers/mmc/host/tmio_mmc_core.c
> @@ -1186,7 +1186,7 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
>         mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
>         mmc->caps2 |= pdata->capabilities2;
>         mmc->max_segs = pdata->max_segs ? : 32;
> -       mmc->max_blk_size = 512;
> +       mmc->max_blk_size = TMIO_MAX_BLK_SIZE;
>         mmc->max_blk_count = pdata->max_blk_count ? :
>                 (PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs;
>         mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
> --
> 2.11.0
>

[RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

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(-)

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

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 378 bytes --]


> > >   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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]


> > 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Wolfram Sang]

> 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?

Re: [RFC PATCH 2/2] mmc: renesas_sdhi: prevent overflow for max_req_size

[Ulf Hansson]

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
>