Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1457157 > unrolled thread

[RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy Stephan Mueller <smueller@chronox.de> - 2016-08-05 17:10 +0200
    Re: [RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy Stephan Mueller <smueller@chronox.de> - 2016-08-06 22:10 +0200
      Re: [RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy Jason Cooper <jason@lakedaemon.net> - 2016-08-06 22:20 +0200
    Re: [RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy Jason Cooper <jason@lakedaemon.net> - 2016-08-06 22:10 +0200
    [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Stephan Mueller <smueller@chronox.de> - 2016-08-07 11:40 +0200
      RE: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy "Pan, Miaoqing" <miaoqing@qti.qualcomm.com> - 2016-08-08 04:10 +0200
        Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Stephan Mueller <smueller@chronox.de> - 2016-08-08 08:50 +0200
          Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Jason Cooper <jason@lakedaemon.net> - 2016-08-08 19:40 +0200
            Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Jason Cooper <jason@lakedaemon.net> - 2016-08-09 00:10 +0200
            RE: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy "Pan, Miaoqing" <miaoqing@qti.qualcomm.com> - 2016-08-09 08:40 +0200
              Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Theodore Ts'o <tytso@mit.edu> - 2016-08-09 14:00 +0200
                Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy Jason Cooper <jason@lakedaemon.net> - 2016-08-09 16:10 +0200

#1457157 — [RFC][PATCH] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Ted, Herbert,

I sent a question to the ATH9K RNG some time ago to the developers.
See https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19115.html

I have not yet received a word and I think this issue should be resolved.

Thanks
Stephan

---8<---

The ATH9K driver implements an RNG which is completely bypassing the
standard Linux HW generator logic.

The RNG may or may not deliver entropy. Considering the conservative
approach in treating entropy with respect to non-auditable sources, this
patch changes the delivered entropy value to zero. The RNG still feeds
data into the input_pool but it is assumed to have no entropy.

When the ATH9K RNG changes to use the HW RNG framework, it may re-enable
the entropy estimation considering that a user can change that value at
boot and runtime.

Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/net/wireless/ath/ath9k/rng.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index d38e50f..d63dc48 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -92,8 +92,7 @@ static int ath9k_rng_kthread(void *data)
 		fail_stats = 0;
 
 		/* sleep until entropy bits under write_wakeup_threshold */
-		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
-					   ATH9K_RNG_ENTROPY(bytes_read));
+		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
 	}
 
 	kfree(rng_buf);
-- 
2.7.4

[toc] | [next] | [standalone]

#1457293

Am Samstag, 6. August 2016, 19:45:51 CEST schrieb Jason Cooper:

Hi Jason,

> Hi Stephan,
> 
> On Fri, Aug 05, 2016 at 05:08:14PM +0200, Stephan Mueller wrote:
> > Hi Ted, Herbert,
> > 
> > I sent a question to the ATH9K RNG some time ago to the developers.
> > See
> > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19115.html
> > 
> > I have not yet received a word and I think this issue should be resolved.
> > 
> > Thanks
> > Stephan
> > 
> > ---8<---
> 
> If the above text is placed below the three dashes, "---", below ...
> 
> > The ATH9K driver implements an RNG which is completely bypassing the
> > standard Linux HW generator logic.
> > 
> > The RNG may or may not deliver entropy. Considering the conservative
> > approach in treating entropy with respect to non-auditable sources, this
> > patch changes the delivered entropy value to zero. The RNG still feeds
> > data into the input_pool but it is assumed to have no entropy.
> > 
> > When the ATH9K RNG changes to use the HW RNG framework, it may re-enable
> > the entropy estimation considering that a user can change that value at
> > boot and runtime.
> > 
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> 
> here, then the mail can be applied directly without editing.

Thank you for the hint. I will resend the patch that can be applied.
> 
> >  drivers/net/wireless/ath/ath9k/rng.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath9k/rng.c
> > b/drivers/net/wireless/ath/ath9k/rng.c index d38e50f..d63dc48 100644
> > --- a/drivers/net/wireless/ath/ath9k/rng.c
> > +++ b/drivers/net/wireless/ath/ath9k/rng.c
> > @@ -92,8 +92,7 @@ static int ath9k_rng_kthread(void *data)
> > 
> >  		fail_stats = 0;
> >  		
> >  		/* sleep until entropy bits under write_wakeup_threshold */
> > 
> > -		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
> > -					   ATH9K_RNG_ENTROPY(bytes_read));
> 
> This is the only use of this macro.  I'd remove the #define on line 25
> as well.

My idea for leaving it was that folks who would bring the RNG into the 
hwrandom framework could reuse the ideas from the original authors.

What about commenting it out with #if 0 ?
> 
> > +		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
> > 
> >  	}
> >  	
> >  	kfree(rng_buf);
> 
> Other than that,
> 
> Reviewed-by: Jason Cooper <jason@lakedaemon.net>

Thank you.
> 
> thx,
> 
> Jason.



-- 
Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1457314

Hi Stephan,

On Sat, Aug 06, 2016 at 10:03:58PM +0200, Stephan Mueller wrote:
> Am Samstag, 6. August 2016, 19:45:51 CEST schrieb Jason Cooper:
> > On Fri, Aug 05, 2016 at 05:08:14PM +0200, Stephan Mueller wrote:
...
> > > diff --git a/drivers/net/wireless/ath/ath9k/rng.c
> > > b/drivers/net/wireless/ath/ath9k/rng.c index d38e50f..d63dc48 100644
> > > --- a/drivers/net/wireless/ath/ath9k/rng.c
> > > +++ b/drivers/net/wireless/ath/ath9k/rng.c
> > > @@ -92,8 +92,7 @@ static int ath9k_rng_kthread(void *data)
> > > 
> > >  		fail_stats = 0;
> > >  		
> > >  		/* sleep until entropy bits under write_wakeup_threshold */
> > > 
> > > -		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
> > > -					   ATH9K_RNG_ENTROPY(bytes_read));
> > 
> > This is the only use of this macro.  I'd remove the #define on line 25
> > as well.
> 
> My idea for leaving it was that folks who would bring the RNG into the 
> hwrandom framework could reuse the ideas from the original authors.
> 
> What about commenting it out with #if 0 ?

#if 0 is frowned upon.  If that calculation is documented somewhere,
then it can be redone from the spec.  If it isn't, then I'd be curious
to know where it came from.

Perhaps one of the ath9k devs can point to a document containing the
formula?  We could put the reference in a comment.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1457297

Hi Stephan,

On Fri, Aug 05, 2016 at 05:08:14PM +0200, Stephan Mueller wrote:
> Hi Ted, Herbert,
> 
> I sent a question to the ATH9K RNG some time ago to the developers.
> See https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg19115.html
> 
> I have not yet received a word and I think this issue should be resolved.
> 
> Thanks
> Stephan
> 
> ---8<---

If the above text is placed below the three dashes, "---", below ...

> The ATH9K driver implements an RNG which is completely bypassing the
> standard Linux HW generator logic.
> 
> The RNG may or may not deliver entropy. Considering the conservative
> approach in treating entropy with respect to non-auditable sources, this
> patch changes the delivered entropy value to zero. The RNG still feeds
> data into the input_pool but it is assumed to have no entropy.
> 
> When the ATH9K RNG changes to use the HW RNG framework, it may re-enable
> the entropy estimation considering that a user can change that value at
> boot and runtime.
> 
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---

here, then the mail can be applied directly without editing.

>  drivers/net/wireless/ath/ath9k/rng.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
> index d38e50f..d63dc48 100644
> --- a/drivers/net/wireless/ath/ath9k/rng.c
> +++ b/drivers/net/wireless/ath/ath9k/rng.c
> @@ -92,8 +92,7 @@ static int ath9k_rng_kthread(void *data)
>  		fail_stats = 0;
>  
>  		/* sleep until entropy bits under write_wakeup_threshold */
> -		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
> -					   ATH9K_RNG_ENTROPY(bytes_read));

This is the only use of this macro.  I'd remove the #define on line 25
as well.

> +		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
>  	}
>  
>  	kfree(rng_buf);

Other than that,

Reviewed-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1457446 — [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

The ATH9K driver implements an RNG which is completely bypassing the
standard Linux HW generator logic.

The RNG may or may not deliver entropy. Considering the conservative
approach in treating entropy with respect to non-auditable sources, this
patch changes the delivered entropy value to zero. The RNG still feeds
data into the input_pool but it is assumed to have no entropy.

When the ATH9K RNG changes to use the HW RNG framework, it may re-enable
the entropy estimation considering that a user can change that value at
boot and runtime.

Reviewed-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/net/wireless/ath/ath9k/rng.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index d38e50f..1ed8338 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -22,7 +22,6 @@
 #include "ar9003_phy.h"
 
 #define ATH9K_RNG_BUF_SIZE	320
-#define ATH9K_RNG_ENTROPY(x)	(((x) * 8 * 320) >> 10) /* quality: 320/1024 */
 
 static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size)
 {
@@ -92,8 +91,7 @@ static int ath9k_rng_kthread(void *data)
 		fail_stats = 0;
 
 		/* sleep until entropy bits under write_wakeup_threshold */
-		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
-					   ATH9K_RNG_ENTROPY(bytes_read));
+		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
 	}
 
 	kfree(rng_buf);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1457514 — RE: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

The entropy was evaluated by crypto expert,  the analysis report show the ADC with at least 10bits and up to 22 bits of min-entropy for a 32 bits value, we conservatively assume the min-entropy is 10 bits out of 32 bits, so that's why set entropy quality  to  320/1024 = 10/32.  Also we have explained in the commit message why can't use the HW RNG framework.

Otherwise, your patch will cause high CPU load,  as continuously read ADC data if entropy bits under write_wakeup_threshold.

--
Miaoqing

-----Original Message-----
From: Stephan Mueller [mailto:smueller@chronox.de] 
Sent: Sunday, August 07, 2016 5:36 PM
To: Ted Tso <tytso@mit.edu>
Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org; linux-crypto@vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Kalle Valo <kvalo@codeaurora.org>; Jason Cooper <jason@lakedaemon.net>
Subject: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

The ATH9K driver implements an RNG which is completely bypassing the standard Linux HW generator logic.

The RNG may or may not deliver entropy. Considering the conservative approach in treating entropy with respect to non-auditable sources, this patch changes the delivered entropy value to zero. The RNG still feeds data into the input_pool but it is assumed to have no entropy.

When the ATH9K RNG changes to use the HW RNG framework, it may re-enable the entropy estimation considering that a user can change that value at boot and runtime.

Reviewed-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 drivers/net/wireless/ath/ath9k/rng.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/rng.c b/drivers/net/wireless/ath/ath9k/rng.c
index d38e50f..1ed8338 100644
--- a/drivers/net/wireless/ath/ath9k/rng.c
+++ b/drivers/net/wireless/ath/ath9k/rng.c
@@ -22,7 +22,6 @@
 #include "ar9003_phy.h"
 
 #define ATH9K_RNG_BUF_SIZE	320
-#define ATH9K_RNG_ENTROPY(x)	(((x) * 8 * 320) >> 10) /* quality: 320/1024 */
 
 static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32 buf_size)  { @@ -92,8 +91,7 @@ static int ath9k_rng_kthread(void *data)
 		fail_stats = 0;
 
 		/* sleep until entropy bits under write_wakeup_threshold */
-		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
-					   ATH9K_RNG_ENTROPY(bytes_read));
+		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
 	}
 
 	kfree(rng_buf);
--
2.7.4

[toc] | [prev] | [next] | [standalone]

#1457576 — Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing:

Hi Miaoqing,

> The entropy was evaluated by crypto expert,  the analysis report show the
> ADC with at least 10bits and up to 22 bits of min-entropy for a 32 bits
> value, we conservatively assume the min-entropy is 10 bits out of 32 bits,
> so that's why set entropy quality  to  320/1024 = 10/32.  Also we have
> explained in the commit message why can't use the HW RNG framework.

Where is the description of the RNG, where is the test implementation? 
> 
> Otherwise, your patch will cause high CPU load,  as continuously read ADC
> data if entropy bits under write_wakeup_threshold.

The issue is that although you may have analyzed it, others are unable to 
measure the quality of the RNG and assess the design as well as the 
implementation of the RNG. This RNG is the only implementation of a hardware 
RNG that per default and without being able to change it at runtime injects 
data into the input_pool where the noise source cannot be audited. Note, even 
other respected RNG noise sources like the Intel RDRAND will not feed into /
dev/random per default in a way that dominates all other noise sources.

I would like to be able to deactivate that noise source to the extent that it 
does not cause /dev/random to unblock. The reason is that your noise source 
starts to dominate all other noise sources.

If you think that this patch is a challenge because your driver starts to 
spin, please help and offer another solution.
> 
> --
> Miaoqing
> 
> -----Original Message-----
> From: Stephan Mueller [mailto:smueller@chronox.de]
> Sent: Sunday, August 07, 2016 5:36 PM
> To: Ted Tso <tytso@mit.edu>
> Cc: herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org;
> linux-crypto@vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>;
> linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Kalle Valo
> <kvalo@codeaurora.org>; Jason Cooper <jason@lakedaemon.net> Subject: [PATCH
> v2] RANDOM: ATH9K RNG delivers zero bits of entropy
> 
> The ATH9K driver implements an RNG which is completely bypassing the
> standard Linux HW generator logic.
> 
> The RNG may or may not deliver entropy. Considering the conservative
> approach in treating entropy with respect to non-auditable sources, this
> patch changes the delivered entropy value to zero. The RNG still feeds data
> into the input_pool but it is assumed to have no entropy.
> 
> When the ATH9K RNG changes to use the HW RNG framework, it may re-enable 
the
> entropy estimation considering that a user can change that value at boot
> and runtime.
> 
> Reviewed-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  drivers/net/wireless/ath/ath9k/rng.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/rng.c
> b/drivers/net/wireless/ath/ath9k/rng.c index d38e50f..1ed8338 100644
> --- a/drivers/net/wireless/ath/ath9k/rng.c
> +++ b/drivers/net/wireless/ath/ath9k/rng.c
> @@ -22,7 +22,6 @@
>  #include "ar9003_phy.h"
> 
>  #define ATH9K_RNG_BUF_SIZE	320
> -#define ATH9K_RNG_ENTROPY(x)	(((x) * 8 * 320) >> 10) /* quality: 320/1024
> */
> 
>  static int ath9k_rng_data_read(struct ath_softc *sc, u32 *buf, u32
> buf_size)  { @@ -92,8 +91,7 @@ static int ath9k_rng_kthread(void *data)
> fail_stats = 0;
> 
>  		/* sleep until entropy bits under write_wakeup_threshold */
> -		add_hwgenerator_randomness((void *)rng_buf, bytes_read,
> -					   ATH9K_RNG_ENTROPY(bytes_read));
> +		add_hwgenerator_randomness((void *)rng_buf, bytes_read, 0);
>  	}
> 
>  	kfree(rng_buf);
> --
> 2.7.4



Ciao
Stephan

[toc] | [prev] | [next] | [standalone]

#1457946 — Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Stephan, Miaoqing Pan,

On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote:
> Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing:
> > The entropy was evaluated by crypto expert,  the analysis report show the
> > ADC with at least 10bits and up to 22 bits of min-entropy for a 32 bits
> > value, we conservatively assume the min-entropy is 10 bits out of 32 bits,
> > so that's why set entropy quality  to  320/1024 = 10/32.

Ok, so the relevant commit is:

  ed14dc0af7cce ath9k: feeding entropy in kernel from ADC capture

Which refers to a previous commit:

  6301566e0b2d ath9k: export HW random number generator

> > Also we have explained in the commit message why can't use the HW
> > RNG framework.

From ed14dc0af7cce:

"""
Since ADC was not designed to be a dedicated HW RNG, we do not want to
bind it to /dev/hwrng framework directly.
"""

> Where is the description of the RNG, where is the test implementation? 
> > 
> > Otherwise, your patch will cause high CPU load,  as continuously read ADC
> > data if entropy bits under write_wakeup_threshold.
> 
> The issue is that although you may have analyzed it, others are unable to 
> measure the quality of the RNG and assess the design as well as the 
> implementation of the RNG. This RNG is the only implementation of a hardware 
> RNG that per default and without being able to change it at runtime injects 
> data into the input_pool where the noise source cannot be audited. Note, even 
> other respected RNG noise sources like the Intel RDRAND will not feed into /
> dev/random per default in a way that dominates all other noise sources.
> 
> I would like to be able to deactivate that noise source to the extent that it 
> does not cause /dev/random to unblock. The reason is that your noise source 
> starts to dominate all other noise sources.

I think the short-term problem here is the config logic:

config ATH9K_HWRNG
       bool "Random number generator support"
       depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K)
       default y

If you have *any* hwrngs you want to use and you have an ath9k card
(HW_RANDOM = y and ATH9K != n), you get the behavior Stephan is pointing
out.

Short term, we should just default no here.

> If you think that this patch is a challenge because your driver starts to 
> spin, please help and offer another solution.

Well, I don't buy the reasoning listed above for not using the hwrng
framework.  Interrupt timings were never designed to be a source of entropy
either.  We need to grab it where ever we can find it, especially on
embedded systems.  Documentation/hw_random.txt even says:

"""
This data is NOT CHECKED by any fitness tests, and could potentially be
bogus (if the hardware is faulty or has been tampered with).
"""

I really don't think there's a problem with adding these sorts of
sources under char/hw_random/.  I think the only thing we would be
concerned about, other than the already addressed entropy estimation,
would be constraining the data rate.

Is ath9k the only wireless card that exposes ADC registers?  What about
sound cards?

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1458277 — Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Stephan,

On Mon, Aug 08, 2016 at 05:29:30PM +0000, Jason Cooper wrote:
> On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote:
...
> > If you think that this patch is a challenge because your driver starts to 
> > spin, please help and offer another solution.
> 
> Well, I don't buy the reasoning listed above for not using the hwrng
> framework.  Interrupt timings were never designed to be a source of entropy
> either.  We need to grab it where ever we can find it, especially on
> embedded systems.  Documentation/hw_random.txt even says:
> 
> """
> This data is NOT CHECKED by any fitness tests, and could potentially be
> bogus (if the hardware is faulty or has been tampered with).
> """
> 
> I really don't think there's a problem with adding these sorts of
> sources under char/hw_random/.  I think the only thing we would be
> concerned about, other than the already addressed entropy estimation,
> would be constraining the data rate.

Further research yields char/hw_random/timeriomem-rng.c

It could use an update to ->read() vice data_{present,read}(), but it's
functionally exactly what the ath9k rng is doing. :)

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1458441 — RE: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Jason, Stephan,

Agree with Jason's point, also  understand Stephan's concern.  The date rate can be roughly estimated by 'cat /dev/random |rngtest -c 1000',  the average speed is 1111.294Kibits/s. I will sent the patch to disable ath9k RNG by default. 

Thanks,
Miaoqing

-----Original Message-----
From: Jason Cooper [mailto:jason@lakedaemon.net] 
Sent: Tuesday, August 09, 2016 1:30 AM
To: Stephan Mueller <smueller@chronox.de>
Cc: Pan, Miaoqing <miaoqing@qti.qualcomm.com>; Ted Tso <tytso@mit.edu>; Sepehrdad, Pouyan <pouyans@qti.qualcomm.com>; herbert@gondor.apana.org.au; linux-kernel@vger.kernel.org; linux-crypto@vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; linux-wireless@vger.kernel.org; ath9k-devel@lists.ath9k.org; Kalle Valo <kvalo@codeaurora.org>
Subject: Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Stephan, Miaoqing Pan,

On Mon, Aug 08, 2016 at 08:41:36AM +0200, Stephan Mueller wrote:
> Am Montag, 8. August 2016, 02:03:36 CEST schrieb Pan, Miaoqing:
> > The entropy was evaluated by crypto expert,  the analysis report 
> > show the ADC with at least 10bits and up to 22 bits of min-entropy 
> > for a 32 bits value, we conservatively assume the min-entropy is 10 
> > bits out of 32 bits, so that's why set entropy quality  to  320/1024 = 10/32.

Ok, so the relevant commit is:

  ed14dc0af7cce ath9k: feeding entropy in kernel from ADC capture

Which refers to a previous commit:

  6301566e0b2d ath9k: export HW random number generator

> > Also we have explained in the commit message why can't use the HW 
> > RNG framework.

From ed14dc0af7cce:

"""
Since ADC was not designed to be a dedicated HW RNG, we do not want to bind it to /dev/hwrng framework directly.
"""

> Where is the description of the RNG, where is the test implementation? 
> > 
> > Otherwise, your patch will cause high CPU load,  as continuously 
> > read ADC data if entropy bits under write_wakeup_threshold.
> 
> The issue is that although you may have analyzed it, others are unable 
> to measure the quality of the RNG and assess the design as well as the 
> implementation of the RNG. This RNG is the only implementation of a 
> hardware RNG that per default and without being able to change it at 
> runtime injects data into the input_pool where the noise source cannot 
> be audited. Note, even other respected RNG noise sources like the 
> Intel RDRAND will not feed into / dev/random per default in a way that dominates all other noise sources.
> 
> I would like to be able to deactivate that noise source to the extent 
> that it does not cause /dev/random to unblock. The reason is that your 
> noise source starts to dominate all other noise sources.

I think the short-term problem here is the config logic:

config ATH9K_HWRNG
       bool "Random number generator support"
       depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K)
       default y

If you have *any* hwrngs you want to use and you have an ath9k card (HW_RANDOM = y and ATH9K != n), you get the behavior Stephan is pointing out.

Short term, we should just default no here.

> If you think that this patch is a challenge because your driver starts 
> to spin, please help and offer another solution.

Well, I don't buy the reasoning listed above for not using the hwrng framework.  Interrupt timings were never designed to be a source of entropy either.  We need to grab it where ever we can find it, especially on embedded systems.  Documentation/hw_random.txt even says:

"""
This data is NOT CHECKED by any fitness tests, and could potentially be bogus (if the hardware is faulty or has been tampered with).
"""

I really don't think there's a problem with adding these sorts of sources under char/hw_random/.  I think the only thing we would be concerned about, other than the already addressed entropy estimation, would be constraining the data rate.

Is ath9k the only wireless card that exposes ADC registers?  What about sound cards?

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1458647 — Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

On Tue, Aug 09, 2016 at 06:30:03AM +0000, Pan, Miaoqing wrote:
> Agree with Jason's point, also understand Stephan's concern.  The
> date rate can be roughly estimated by 'cat /dev/random |rngtest -c
> 1000', the average speed is 1111.294Kibits/s. I will sent the patch
> to disable ath9k RNG by default.

If the ATH9K is generating some random amount of data, but you don't
know how random, and you're gathering it opportunistically --- for
example, suppose a wireless driver gets the radio's signal strength
through the normal course of its operation, and while there might not
be that much randomness for someone who can observe the exact details
of how the phone is positioned in the room --- but for which the
analyst sitting in Fort Meade won't know whether or not the phone is
on the desk, or in a knapsack under the table, the right interface to
use is:

   void add_device_randomness(const void *buf, unsigned int size);
	
This won't increase the entropy count, but if you have the bit of
potential randomness "for free", you might as well contribute it to
the entropy pool.  If it turns out that the chip was manufactured in
China, and the MSS has backdoored it out the wazoo, it won't do any
harm --- where as using the hwrng framework would be disastrous.

Cheerfs,

						- Ted

[toc] | [prev] | [next] | [standalone]

#1458783 — Re: [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

Hi Ted,

On Tue, Aug 09, 2016 at 07:56:22AM -0400, Theodore Ts'o wrote:
> On Tue, Aug 09, 2016 at 06:30:03AM +0000, Pan, Miaoqing wrote:
> > Agree with Jason's point, also understand Stephan's concern.  The
> > date rate can be roughly estimated by 'cat /dev/random |rngtest -c
> > 1000', the average speed is 1111.294Kibits/s. I will sent the patch
> > to disable ath9k RNG by default.
> 
> If the ATH9K is generating some random amount of data, but you don't
> know how random, and you're gathering it opportunistically --- for
> example, suppose a wireless driver gets the radio's signal strength
> through the normal course of its operation, and while there might not
> be that much randomness for someone who can observe the exact details
> of how the phone is positioned in the room --- but for which the
> analyst sitting in Fort Meade won't know whether or not the phone is
> on the desk, or in a knapsack under the table, the right interface to
> use is:
> 
>    void add_device_randomness(const void *buf, unsigned int size);
> 	
> This won't increase the entropy count, but if you have the bit of
> potential randomness "for free", you might as well contribute it to
> the entropy pool.  If it turns out that the chip was manufactured in
> China, and the MSS has backdoored it out the wazoo, it won't do any
> harm --- where as using the hwrng framework would be disastrous.

Ok, I get that.  However, we have Documentation/hw_random.txt saying:

  This data is NOT CHECKED by any fitness tests, and could potentially be
  bogus (if the hardware is faulty or has been tampered with).  Data is
  only output if the hardware "has-data" flag is set, but nevertheless a
  security-conscious person would run fitness tests on the data before
  assuming it is truly random.

Which I would read as "Don't assume 1 bit read from /dev/hwrng equals 1
bit of entropy."  Which runs counter to Stephan's reading of the rngd
code.

And then we have drivers like timeriomem-rng.c that appear to be
spitting out the raw 32bit register value of $SOC's timer.

Thankfully, most hw_random drivers don't set the quality.  So unless the
user sets the default_quality param, it's zero.

iiuc, Ted, you're saying using the hw_random framework would be
disasterous because despite most drivers having a default quality of 0,
rngd assumes 1 bit of entropy for every bit read?

thx,

Jason.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web