[PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[Torne (Richard Coles)]

From: "Torne (Richard Coles)" <torne@google.com>

MMC CSD info can specify very large, ridiculous timeouts, big enough to
overflow timeout_ns on 32-bit machines. This can result in the card
timing out on every operation because the wrapped timeout value is far
too small.

Fix the overflow by calculating the timeout in microseconds first and
capping the result at 2 seconds.  Cards specifying longer timeouts are
almost certainly insane, and host controllers generally cannot support
timeouts that long in any case.

2 seconds should be plenty of time for any card to actually function;
the timeout calculation code is already using 1 second as a "worst case"
timeout for cards running in SPI mode.

Signed-off-by: Torne (Richard Coles) <torne@google.com>
---
 drivers/mmc/core/core.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index ba821fe..368452e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -488,7 +488,7 @@ EXPORT_SYMBOL(mmc_wait_for_cmd);
  */
 void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 {
-	unsigned int mult;
+	unsigned int mult, timeout_us;
 
 	/*
 	 * SDIO cards only define an upper 1 s limit on access.
@@ -511,16 +511,26 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 	if (data->flags & MMC_DATA_WRITE)
 		mult <<= card->csd.r2w_factor;
 
-	data->timeout_ns = card->csd.tacc_ns * mult;
+	/*
+	 * The timeout in nanoseconds may overflow, so calculate it in
+	 * microseconds first. Cap it at two seconds both to avoid the overflow
+	 * and also because host controllers cannot generally generate timeouts
+	 * that long anyway.
+	 */
+	timeout_us = (card->csd.tacc_ns / 1000) * mult;
+	if (timeout_us < 2000000)
+		data->timeout_ns = card->csd.tacc_ns * mult;
+	else
+		data->timeout_ns = 2000000000;
+
 	data->timeout_clks = card->csd.tacc_clks * mult;
 
 	/*
 	 * SD cards also have an upper limit on the timeout.
 	 */
 	if (mmc_card_sd(card)) {
-		unsigned int timeout_us, limit_us;
+		unsigned int limit_us;
 
-		timeout_us = data->timeout_ns / 1000;
 		if (mmc_host_clk_rate(card->host))
 			timeout_us += data->timeout_clks * 1000 /
 				(mmc_host_clk_rate(card->host) / 1000);
-- 
1.7.7.3

Re: [PATCH V2] MMC: core: cap MMC card timeouts at 2 seconds.

[Torne (Richard Coles)]

On 29 May 2012 03:32, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Mon, 2012-05-28 at 18:31 +0100, Torne (Richard Coles) wrote:
>> From: "Torne (Richard Coles)" <torne@google.com>
>>
>> MMC CSD info can specify very large, ridiculous timeouts, big enough to
>> overflow timeout_ns on 32-bit machines. This can result in the card
>> timing out on every operation because the wrapped timeout value is far
>> too small.
>>
>> Fix the overflow by capping the result at 2 seconds.  Cards specifying
>> longer timeouts are almost certainly insane, and host controllers
>> generally cannot support timeouts that long in any case.
>>
>> 2 seconds should be plenty of time for any card to actually function;
>> the timeout calculation code is already using 1 second as a "worst case"
>> timeout for cards running in SPI mode.
>
> Needs a 'Signed-off-by'.

Oops, sorry. I'm still getting used to the patch workflow. Will resend.

-- 
Torne (Richard Coles)
torne@google.com

[PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[Torne (Richard Coles)]

From: "Torne (Richard Coles)" <torne@google.com>

MMC CSD info can specify very large, ridiculous timeouts, big enough to
overflow timeout_ns on 32-bit machines. This can result in the card
timing out on every operation because the wrapped timeout value is far
too small.

Fix the overflow by capping the result at 2 seconds.  Cards specifying
longer timeouts are almost certainly insane, and host controllers
generally cannot support timeouts that long in any case.

2 seconds should be plenty of time for any card to actually function;
the timeout calculation code is already using 1 second as a "worst case"
timeout for cards running in SPI mode.

Signed-off-by: Torne (Richard Coles) <torne@google.com>
---
 drivers/mmc/core/core.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0b6141d..3b4a9fc 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -512,7 +512,16 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
 	if (data->flags & MMC_DATA_WRITE)
 		mult <<= card->csd.r2w_factor;
 
-	data->timeout_ns = card->csd.tacc_ns * mult;
+	/*
+	 * The timeout in nanoseconds may overflow with some cards. Cap it at
+	 * two seconds both to avoid the overflow and also because host
+	 * controllers cannot generally generate timeouts that long anyway.
+	 */
+	if (card->csd.tacc_ns <= (2 * NSEC_PER_SEC) / mult)
+		data->timeout_ns = card->csd.tacc_ns * mult;
+	else
+		data->timeout_ns = 2 * NSEC_PER_SEC;
+
 	data->timeout_clks = card->csd.tacc_clks * mult;
 
 	/*
-- 
1.7.7.3

Re: [PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[Ulf Hansson]

Hi Richard,

On 29 May 2012 17:02, Torne (Richard Coles) <torne@google.com> wrote:
> From: "Torne (Richard Coles)" <torne@google.com>
>
> MMC CSD info can specify very large, ridiculous timeouts, big enough to
> overflow timeout_ns on 32-bit machines. This can result in the card
> timing out on every operation because the wrapped timeout value is far
> too small.
>
> Fix the overflow by capping the result at 2 seconds.  Cards specifying
> longer timeouts are almost certainly insane, and host controllers
> generally cannot support timeouts that long in any case.
>
> 2 seconds should be plenty of time for any card to actually function;
> the timeout calculation code is already using 1 second as a "worst case"
> timeout for cards running in SPI mode.
>
> Signed-off-by: Torne (Richard Coles) <torne@google.com>
> ---
>  drivers/mmc/core/core.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 0b6141d..3b4a9fc 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -512,7 +512,16 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
>        if (data->flags & MMC_DATA_WRITE)
>                mult <<= card->csd.r2w_factor;
>
> -       data->timeout_ns = card->csd.tacc_ns * mult;
> +       /*
> +        * The timeout in nanoseconds may overflow with some cards. Cap it at
> +        * two seconds both to avoid the overflow and also because host
> +        * controllers cannot generally generate timeouts that long anyway.
> +        */
> +       if (card->csd.tacc_ns <= (2 * NSEC_PER_SEC) / mult)
> +               data->timeout_ns = card->csd.tacc_ns * mult;
> +       else
> +               data->timeout_ns = 2 * NSEC_PER_SEC;
> +

The above looks OK to me, although if doing this for tacc_ns we might
want to do this for tacc_clks as well. Could we include that is this
patch as well?

>        data->timeout_clks = card->csd.tacc_clks * mult;
>
>        /*
> --
> 1.7.7.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kind regards
Ulf Hansson

Re: [PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[Torne (Richard Coles)]

On 31 May 2012 11:00, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> Hi Richard,
>
> On 29 May 2012 17:02, Torne (Richard Coles) <torne@google.com> wrote:
>> From: "Torne (Richard Coles)" <torne@google.com>
>>
>> MMC CSD info can specify very large, ridiculous timeouts, big enough to
>> overflow timeout_ns on 32-bit machines. This can result in the card
>> timing out on every operation because the wrapped timeout value is far
>> too small.
>>
>> Fix the overflow by capping the result at 2 seconds.  Cards specifying
>> longer timeouts are almost certainly insane, and host controllers
>> generally cannot support timeouts that long in any case.
>>
>> 2 seconds should be plenty of time for any card to actually function;
>> the timeout calculation code is already using 1 second as a "worst case"
>> timeout for cards running in SPI mode.
>>
>> Signed-off-by: Torne (Richard Coles) <torne@google.com>
>> ---
>>  drivers/mmc/core/core.c |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index 0b6141d..3b4a9fc 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -512,7 +512,16 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
>>        if (data->flags & MMC_DATA_WRITE)
>>                mult <<= card->csd.r2w_factor;
>>
>> -       data->timeout_ns = card->csd.tacc_ns * mult;
>> +       /*
>> +        * The timeout in nanoseconds may overflow with some cards. Cap it at
>> +        * two seconds both to avoid the overflow and also because host
>> +        * controllers cannot generally generate timeouts that long anyway.
>> +        */
>> +       if (card->csd.tacc_ns <= (2 * NSEC_PER_SEC) / mult)
>> +               data->timeout_ns = card->csd.tacc_ns * mult;
>> +       else
>> +               data->timeout_ns = 2 * NSEC_PER_SEC;
>> +
>
> The above looks OK to me, although if doing this for tacc_ns we might
> want to do this for tacc_clks as well. Could we include that is this
> patch as well?

timeout_clks can't overflow, that I can see: the highest possible
value for tacc_clks is 25500 (it's an 8 bit field specified in
100-clock increments) and the highest mult is 12800 (r2w_factor only
goes up to 128). This only makes 0x13747800 :)

>>        data->timeout_clks = card->csd.tacc_clks * mult;
>>
>>        /*
>> --
>> 1.7.7.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Kind regards
> Ulf Hansson



-- 
Torne (Richard Coles)
torne@google.com

Re: [PATCH] MMC: core: cap MMC card timeouts at 2 seconds.

[Ulf Hansson]

Hi Richard,

On 31 May 2012 18:15, Torne (Richard Coles) <torne@google.com> wrote:
> On 31 May 2012 11:00, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Hi Richard,
>>
>> On 29 May 2012 17:02, Torne (Richard Coles) <torne@google.com> wrote:
>>> From: "Torne (Richard Coles)" <torne@google.com>
>>>
>>> MMC CSD info can specify very large, ridiculous timeouts, big enough to
>>> overflow timeout_ns on 32-bit machines. This can result in the card
>>> timing out on every operation because the wrapped timeout value is far
>>> too small.
>>>
>>> Fix the overflow by capping the result at 2 seconds.  Cards specifying
>>> longer timeouts are almost certainly insane, and host controllers
>>> generally cannot support timeouts that long in any case.
>>>
>>> 2 seconds should be plenty of time for any card to actually function;
>>> the timeout calculation code is already using 1 second as a "worst case"
>>> timeout for cards running in SPI mode.
>>>
>>> Signed-off-by: Torne (Richard Coles) <torne@google.com>
>>> ---
>>>  drivers/mmc/core/core.c |   11 ++++++++++-
>>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> index 0b6141d..3b4a9fc 100644
>>> --- a/drivers/mmc/core/core.c
>>> +++ b/drivers/mmc/core/core.c
>>> @@ -512,7 +512,16 @@ void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card)
>>>        if (data->flags & MMC_DATA_WRITE)
>>>                mult <<= card->csd.r2w_factor;
>>>
>>> -       data->timeout_ns = card->csd.tacc_ns * mult;
>>> +       /*
>>> +        * The timeout in nanoseconds may overflow with some cards. Cap it at
>>> +        * two seconds both to avoid the overflow and also because host
>>> +        * controllers cannot generally generate timeouts that long anyway.
>>> +        */
>>> +       if (card->csd.tacc_ns <= (2 * NSEC_PER_SEC) / mult)
>>> +               data->timeout_ns = card->csd.tacc_ns * mult;
>>> +       else
>>> +               data->timeout_ns = 2 * NSEC_PER_SEC;
>>> +
>>
>> The above looks OK to me, although if doing this for tacc_ns we might
>> want to do this for tacc_clks as well. Could we include that is this
>> patch as well?
>
> timeout_clks can't overflow, that I can see: the highest possible
> value for tacc_clks is 25500 (it's an 8 bit field specified in
> 100-clock increments) and the highest mult is 12800 (r2w_factor only
> goes up to 128). This only makes 0x13747800 :)
>

Thanks for sorting this out and sorry for me being too lazy to read
the spec. :-)

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Ulf Hansson