* [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
@ 2012-05-25 9:12 Peter Korsgaard
2012-05-25 10:04 ` Nicolas Ferre
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Peter Korsgaard @ 2012-05-25 9:12 UTC (permalink / raw)
To: herbert, nicolas.ferre, jamie, linux-kernel, GPontis
Cc: Peter Korsgaard, stable
Data valid gets cleared by reading the ISR (status register) and NOT from
reading ODATA (data register). A new data word can become available between
checking ISR and reading ODATA, causing us to reuse the same data word next
time atmel_trng_read() gets called, if that happens before the following
data word is ready.
With this fixed, rngtest no longer complains of 'Continous run' errors.
Before:
rngtest -c 1000 < /dev/hwrng
rngtest 3
Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions. There is NO warr.
rngtest: starting FIPS tests...
rngtest: bits received from input: 20000032
rngtest: FIPS 140-2 successes: 923
rngtest: FIPS 140-2 failures: 77
rngtest: FIPS 140-2(2001-10-10) Monobit: 0
rngtest: FIPS 140-2(2001-10-10) Poker: 0
rngtest: FIPS 140-2(2001-10-10) Runs: 1
rngtest: FIPS 140-2(2001-10-10) Long run: 0
rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
rngtest: Program run time: 1931860 microseconds
After:
rngtest -c 1000 < /dev/hwrng
rngtest 3
Copyright (c) 2004 by Henrique de Moraes Holschuh
This is free software; see the source for copying conditions. There is NO warr.
rngtest: starting FIPS tests...
rngtest: bits received from input: 20000032
rngtest: FIPS 140-2 successes: 1000
rngtest: FIPS 140-2 failures: 0
rngtest: FIPS 140-2(2001-10-10) Monobit: 0
rngtest: FIPS 140-2(2001-10-10) Poker: 0
rngtest: FIPS 140-2(2001-10-10) Runs: 0
rngtest: FIPS 140-2(2001-10-10) Long run: 0
rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
rngtest: Program run time: 2035543 microseconds
Cc: stable@vger.kernel.org
Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
Reported-by: George Pontis <GPontis@z9.com>
---
drivers/char/hw_random/atmel-rng.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
index d7ab920..731c904 100644
--- a/drivers/char/hw_random/atmel-rng.c
+++ b/drivers/char/hw_random/atmel-rng.c
@@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
/* data ready? */
if (readl(trng->base + TRNG_ISR) & 1) {
*data = readl(trng->base + TRNG_ODATA);
+ /*
+ ensure data ready is only set again AFTER the next data
+ word is ready in case it got set between checking ISR
+ and reading ODATA, so we don't risk re-reading the
+ same word
+ */
+ readl(trng->base + TRNG_ISR);
return 4;
} else
return 0;
--
1.7.10
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
2012-05-25 9:12 [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits Peter Korsgaard
@ 2012-05-25 10:04 ` Nicolas Ferre
2012-05-25 10:10 ` Peter Korsgaard
2012-05-27 11:51 ` Nicolas Ferre
2012-05-31 10:54 ` Herbert Xu
2 siblings, 1 reply; 6+ messages in thread
From: Nicolas Ferre @ 2012-05-25 10:04 UTC (permalink / raw)
To: Peter Korsgaard, herbert; +Cc: jamie, linux-kernel, GPontis, stable
On 05/25/2012 11:12 AM, Peter Korsgaard :
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
Hi Peter, thanks for finding this...
> With this fixed, rngtest no longer complains of 'Continous run' errors.
> Before:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 923
> rngtest: FIPS 140-2 failures: 77
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 1
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
> rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
> rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
> rngtest: Program run time: 1931860 microseconds
>
> After:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 1000
> rngtest: FIPS 140-2 failures: 0
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 0
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
> rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
> rngtest: Program run time: 2035543 microseconds
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> Reported-by: George Pontis <GPontis@z9.com>
> ---
> drivers/char/hw_random/atmel-rng.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
> index d7ab920..731c904 100644
> --- a/drivers/char/hw_random/atmel-rng.c
> +++ b/drivers/char/hw_random/atmel-rng.c
> @@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
> /* data ready? */
> if (readl(trng->base + TRNG_ISR) & 1) {
> *data = readl(trng->base + TRNG_ODATA);
> + /*
> + ensure data ready is only set again AFTER the next data
> + word is ready in case it got set between checking ISR
> + and reading ODATA, so we don't risk re-reading the
> + same word
> + */
> + readl(trng->base + TRNG_ISR);
> return 4;
> } else
> return 0;
What about a single read to ISR like this:
tmp = readl(trng->base + TRNG_ODATA);
if (readl(trng->base + TRNG_ISR) & 1) {
*data = tmp;
return 4;
} else {
return 0;
}
But it is true that there is always 2 reads in case of data not
available, instead of just one: I cannot figure out which solution is
the fastest: your thoughts?
Bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
2012-05-25 10:04 ` Nicolas Ferre
@ 2012-05-25 10:10 ` Peter Korsgaard
2012-05-27 11:49 ` Nicolas Ferre
0 siblings, 1 reply; 6+ messages in thread
From: Peter Korsgaard @ 2012-05-25 10:10 UTC (permalink / raw)
To: Nicolas Ferre; +Cc: herbert, jamie, linux-kernel, GPontis, stable
>>>>> "Nicolas" == Nicolas Ferre <nicolas.ferre@atmel.com> writes:
Hi,
Nicolas> What about a single read to ISR like this:
Nicolas> tmp = readl(trng->base + TRNG_ODATA);
Nicolas> if (readl(trng->base + TRNG_ISR) & 1) {
Nicolas> *data = tmp;
Nicolas> return 4;
Nicolas> } else {
Nicolas> return 0;
Nicolas> }
No, that won't work as you then have another race. Data might not be
ready when you read ODATA, but then become ready just in time for when
you read ISR, so you end up using stale data.
It all would have been easier if the ready bit would get cleared on
reads from ODATA instead of/as well as from ISR, but that's
unfortunately not the case.
--
Bye, Peter Korsgaard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
2012-05-25 10:10 ` Peter Korsgaard
@ 2012-05-27 11:49 ` Nicolas Ferre
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Ferre @ 2012-05-27 11:49 UTC (permalink / raw)
To: Peter Korsgaard; +Cc: herbert, jamie, linux-kernel, GPontis, stable
On 05/25/2012 12:10 PM, Peter Korsgaard :
>>>>>> "Nicolas" == Nicolas Ferre <nicolas.ferre@atmel.com> writes:
>
> Hi,
>
> Nicolas> What about a single read to ISR like this:
>
> Nicolas> tmp = readl(trng->base + TRNG_ODATA);
> Nicolas> if (readl(trng->base + TRNG_ISR) & 1) {
> Nicolas> *data = tmp;
> Nicolas> return 4;
> Nicolas> } else {
> Nicolas> return 0;
> Nicolas> }
>
> No, that won't work as you then have another race. Data might not be
> ready when you read ODATA, but then become ready just in time for when
> you read ISR, so you end up using stale data.
Yes, sure.
> It all would have been easier if the ready bit would get cleared on
> reads from ODATA instead of/as well as from ISR, but that's
> unfortunately not the case.
I will talk about that idea to my friends designers and see if we can
improve things, in the future...
Thanks, bye,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
2012-05-25 9:12 [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits Peter Korsgaard
2012-05-25 10:04 ` Nicolas Ferre
@ 2012-05-27 11:51 ` Nicolas Ferre
2012-05-31 10:54 ` Herbert Xu
2 siblings, 0 replies; 6+ messages in thread
From: Nicolas Ferre @ 2012-05-27 11:51 UTC (permalink / raw)
To: Peter Korsgaard, herbert; +Cc: jamie, linux-kernel, GPontis, stable
On 05/25/2012 11:12 AM, Peter Korsgaard :
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
>
> With this fixed, rngtest no longer complains of 'Continous run' errors.
> Before:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 923
> rngtest: FIPS 140-2 failures: 77
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 1
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 76
> rngtest: input channel speed: (min=721.402; avg=46003.510; max=49321.338)Kibitss
> rngtest: FIPS tests speed: (min=11.442; avg=12.714; max=12.801)Mibits/s
> rngtest: Program run time: 1931860 microseconds
>
> After:
>
> rngtest -c 1000 < /dev/hwrng
> rngtest 3
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions. There is NO warr.
>
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 20000032
> rngtest: FIPS 140-2 successes: 1000
> rngtest: FIPS 140-2 failures: 0
> rngtest: FIPS 140-2(2001-10-10) Monobit: 0
> rngtest: FIPS 140-2(2001-10-10) Poker: 0
> rngtest: FIPS 140-2(2001-10-10) Runs: 0
> rngtest: FIPS 140-2(2001-10-10) Long run: 0
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=777.518; avg=36988.482; max=43115.342)Kibitss
> rngtest: FIPS tests speed: (min=11.951; avg=12.715; max=12.887)Mibits/s
> rngtest: Program run time: 2035543 microseconds
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> Reported-by: George Pontis <GPontis@z9.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Thanks!
> ---
> drivers/char/hw_random/atmel-rng.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/char/hw_random/atmel-rng.c b/drivers/char/hw_random/atmel-rng.c
> index d7ab920..731c904 100644
> --- a/drivers/char/hw_random/atmel-rng.c
> +++ b/drivers/char/hw_random/atmel-rng.c
> @@ -36,6 +36,13 @@ static int atmel_trng_read(struct hwrng *rng, void *buf, size_t max,
> /* data ready? */
> if (readl(trng->base + TRNG_ISR) & 1) {
> *data = readl(trng->base + TRNG_ODATA);
> + /*
> + ensure data ready is only set again AFTER the next data
> + word is ready in case it got set between checking ISR
> + and reading ODATA, so we don't risk re-reading the
> + same word
> + */
> + readl(trng->base + TRNG_ISR);
> return 4;
> } else
> return 0;
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits
2012-05-25 9:12 [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits Peter Korsgaard
2012-05-25 10:04 ` Nicolas Ferre
2012-05-27 11:51 ` Nicolas Ferre
@ 2012-05-31 10:54 ` Herbert Xu
2 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2012-05-31 10:54 UTC (permalink / raw)
To: Peter Korsgaard; +Cc: nicolas.ferre, jamie, linux-kernel, GPontis, stable
On Fri, May 25, 2012 at 11:12:38AM +0200, Peter Korsgaard wrote:
> Data valid gets cleared by reading the ISR (status register) and NOT from
> reading ODATA (data register). A new data word can become available between
> checking ISR and reading ODATA, causing us to reuse the same data word next
> time atmel_trng_read() gets called, if that happens before the following
> data word is ready.
Patch applied crypto. Thanks!
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-31 10:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-25 9:12 [PATCH] hw_random: atmel-rng: fix race condition leading to repeated bits Peter Korsgaard
2012-05-25 10:04 ` Nicolas Ferre
2012-05-25 10:10 ` Peter Korsgaard
2012-05-27 11:49 ` Nicolas Ferre
2012-05-27 11:51 ` Nicolas Ferre
2012-05-31 10:54 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).