diff mbox

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

Message ID 34197429.2CvoIfft9B@positron.chronox.de (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show

Commit Message

Stephan Mueller Aug. 5, 2016, 3:08 p.m. UTC
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(-)

Comments

Jason Cooper Aug. 6, 2016, 7:45 p.m. UTC | #1
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephan Mueller Aug. 6, 2016, 8:03 p.m. UTC | #2
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.
Jason Cooper Aug. 6, 2016, 8:16 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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