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

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

[Stephan Mueller]

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

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

[Jason Cooper]

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

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

[Stephan Mueller]

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

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

[Jason Cooper]

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.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Stephan Mueller]

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

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[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.  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 at chronox.de] 
Sent: Sunday, August 07, 2016 5:36 PM
To: Ted Tso <tytso@mit.edu>
Cc: herbert at gondor.apana.org.au; linux-kernel at vger.kernel.org; linux-crypto at vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; linux-wireless at vger.kernel.org; ath9k-devel at 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

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Stephan Mueller]

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 at chronox.de]
> Sent: Sunday, August 07, 2016 5:36 PM
> To: Ted Tso <tytso@mit.edu>
> Cc: herbert at gondor.apana.org.au; linux-kernel at vger.kernel.org;
> linux-crypto at vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>;
> linux-wireless at vger.kernel.org; ath9k-devel at 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

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Jason Cooper]

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.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Jason Cooper]

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.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Pan, Miaoqing]

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 at 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 at gondor.apana.org.au; linux-kernel at vger.kernel.org; linux-crypto at vger.kernel.org; ath9k-devel <ath9k-devel@qca.qualcomm.com>; linux-wireless at vger.kernel.org; ath9k-devel at 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.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Theodore Ts'o]

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

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Jason Cooper]

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.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Theodore Ts'o]

On Tue, Aug 09, 2016 at 02:04:44PM +0000, Jason Cooper wrote:
> 
> 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?

Sorry, what I was trying to say (but failed) was that bypassing the
hwrng framework and injecting entropy directly the entropy pool was
disatrous.

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

The fact that this is "most" and not "all" does scare me a little.

As far as I'm concerned *all* hw_random drivers should set quality to
zero, since it should be up to the system administrator.  Perhaps the
one exception is virtio_rng, since if you don't trust the hypvervisor,
the security of the VM is hopeless.  That being said, I have seen
configurations of KVM which use:

	-object rng-random,filename=/dev/urandom,id=rng0 \
	-device virtio-rng-pci,rng=rng0

Which is somewhat non-ideal.  (Try running od -x /dev/random on such a
guest system....)

					- Ted

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Jason Cooper]

Hey Ted,

On Wed, Aug 10, 2016 at 07:44:25PM -0400, Theodore Ts'o wrote:
> On Tue, Aug 09, 2016 at 02:04:44PM +0000, Jason Cooper wrote:
> > 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?
> 
> Sorry, what I was trying to say (but failed) was that bypassing the
> hwrng framework and injecting entropy directly the entropy pool was
> disatrous.

Ok, whew. :)

> > Thankfully, most hw_random drivers don't set the quality.  So unless the
> > user sets the default_quality param, it's zero.
> 
> The fact that this is "most" and not "all" does scare me a little.

My recent grep showed that only virtio-rng set it to a non-zero value.

> As far as I'm concerned *all* hw_random drivers should set quality to
> zero, since it should be up to the system administrator.

Agreed.

Gathering conversation about this from a few related threads, I have one
concern.  Apparently there is some confusion in userspace consumers of
/dev/hwrng data as to the quality of it.  Specifically, rngd (spotted by
Stephan Mueller) appears to assume 1bit of entropy per 1 bit read. :-/

So, while moving ath9k-rng to the hwrng framework makes complete sense
internally, it's not so good for existing userspace assumptions.  I'd
think that timeriomem-rng falls in this same category.

In light of this, do you think it's worth the effort (I'm volunteering)
to create a subcategory of hwrng drivers that are 'environemntal' rngs?
They can contribute to the kernel entropy pools, but not to /dev/hwrng.

thx,

Jason.

[ath9k-devel] [PATCH v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Kalle Valo]

"Pan, Miaoqing" <miaoqing@qti.qualcomm.com> writes:

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

Please don't top post, it breaks patchwork which is extremely annoying
for me:

https://patchwork.kernel.org/patch/9266265/

https://patchwork.kernel.org/patch/9266617/

-- 
Kalle Valo

[ath9k-devel] [v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Kalle Valo]

Stephan Mueller <smueller@chronox.de> wrote:
> 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>

Based on the discussion I'm dropping this patch. But the discussion was
hard to follow so please let me know if I misunderstood.

Patch set to Rejected.

-- 
https://patchwork.kernel.org/patch/9266265/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[ath9k-devel] [v2] RANDOM: ATH9K RNG delivers zero bits of entropy

[Stephan Mueller]

Am Dienstag, 27. September 2016, 16:44:16 CEST schrieb Kalle Valo:

Hi Kalle,

> Stephan Mueller <smueller@chronox.de> wrote:
> > 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>
> 
> Based on the discussion I'm dropping this patch. But the discussion was
> hard to follow so please let me know if I misunderstood.

I guess the rejection is appropriate, but something needs to be done: 
add_hwgenerator_randomness should not be used in this scenario.
> 
> Patch set to Rejected.



Ciao
Stephan