[2/2] ath9k: disable RNG by default

Commit Message

Miaoqing Pan Aug. 9, 2016, 7:02 a.m. UTC

From: Miaoqing Pan <[e-mail]>

ath9k RNG will dominates all the noise sources from the real HW
RNG, disable it by default. But we strongly recommand to enable
it if the system without HW RNG, especially on embedded systems.

Signed-off-by: Miaoqing Pan <[e-mail]>
---
 drivers/net/wireless/ath/ath9k/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephan Mueller Aug. 9, 2016, 7:14 a.m. UTC | #1

Am Dienstag, 9. August 2016, 15:02:27 CEST schrieb [e-mail]:

Hi Miaoqing,

> From: Miaoqing Pan <[e-mail]>
> 
> ath9k RNG will dominates all the noise sources from the real HW
> RNG, disable it by default. But we strongly recommand to enable
> it if the system without HW RNG, especially on embedded systems.
> 
> Signed-off-by: Miaoqing Pan <[e-mail]>

As a short term solution:

Acked-by: Stephan Mueller <[e-mail]>

But as Jason outlined, there should be nothing that prevents using this code 
with the HW Random framework. This framework also has logic to limit the rate 
of injection and allows the setting of the entropy threshold at runtime.

> ---
>  drivers/net/wireless/ath/ath9k/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/Kconfig
> b/drivers/net/wireless/ath/ath9k/Kconfig index f68cb00..8f231c6 100644
> --- a/drivers/net/wireless/ath/ath9k/Kconfig
> +++ b/drivers/net/wireless/ath/ath9k/Kconfig
> @@ -180,7 +180,7 @@ config ATH9K_HTC_DEBUGFS
>  config ATH9K_HWRNG
>  	bool "Random number generator support"
>  	depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K)
> -	default y
> +	default n
>  	---help---
>  	  This option incorporates the ADC register output as a source of
>  	  randomness into Linux entropy pool (/dev/urandom and /dev/random)



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

miaoqing pan Aug. 9, 2016, 7:35 a.m. UTC | #2

Hi Stephan,

So your suggestion is to use HW Random framework ?   Actually, which was done by the commit 6301566e0b2d ("ath9k: export HW random number generator"), but it was reverted, you can refer to https://www.mail-archive.com/linux-crypto%40vger.kernel.org/msg15483.html for more information.

--
Miaoqing


-----Original Message-----
From: Stephan Mueller [mailto:[e-mail]] 
Sent: Tuesday, August 09, 2016 3:15 PM
To: [e-mail]
Cc: Valo, Kalle <[e-mail]>; [e-mail]; ath9k-devel <[e-mail]>; [e-mail]; [e-mail]; Sepehrdad, Pouyan <[e-mail]>
Subject: Re: [PATCH 2/2] ath9k: disable RNG by default

Am Dienstag, 9. August 2016, 15:02:27 CEST schrieb [e-mail]:

Hi Miaoqing,

> From: Miaoqing Pan <[e-mail]>
> 
> ath9k RNG will dominates all the noise sources from the real HW RNG, 
> disable it by default. But we strongly recommand to enable it if the 
> system without HW RNG, especially on embedded systems.
> 
> Signed-off-by: Miaoqing Pan <[e-mail]>

As a short term solution:

Acked-by: Stephan Mueller <[e-mail]>

But as Jason outlined, there should be nothing that prevents using this code with the HW Random framework. This framework also has logic to limit the rate of injection and allows the setting of the entropy threshold at runtime.

> ---
>  drivers/net/wireless/ath/ath9k/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/Kconfig
> b/drivers/net/wireless/ath/ath9k/Kconfig index f68cb00..8f231c6 100644
> --- a/drivers/net/wireless/ath/ath9k/Kconfig
> +++ b/drivers/net/wireless/ath/ath9k/Kconfig
> @@ -180,7 +180,7 @@ config ATH9K_HTC_DEBUGFS  config ATH9K_HWRNG
>  	bool "Random number generator support"
>  	depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K)
> -	default y
> +	default n
>  	---help---
>  	  This option incorporates the ADC register output as a source of
>  	  randomness into Linux entropy pool (/dev/urandom and /dev/random)



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephan Mueller Aug. 9, 2016, 8:07 a.m. UTC | #3

Am Dienstag, 9. August 2016, 07:35:33 CEST schrieb Pan, Miaoqing:

Hi Miaoqing, Herbert, Matt,

> Hi Stephan,
> 
> So your suggestion is to use HW Random framework ?   Actually, which was
> done by the commit 6301566e0b2d ("ath9k: export HW random number
> generator"), but it was reverted, you can refer to
> https://www.mail-archive.com/linux-crypto%40vger.kernel.org/msg15483.html
> for more information.

I see, it is the same RNG we talked about earlier. The issue is that the 
suggested rngd per default assumes one bit of entropy with every data bit. 
This is not given with this noise source. This is the basis of my reply last 
time.

Herbert, Matt, should such noise sources be added to the HW random framework? 
The thing is that the in-kernel HW random to input_pool link per default uses 
a more conservative entropy estimate than the user space rngd. I would think 
that the in-kernel link would appropriate for that rng. But the user space 
rngd tool with its default behavior is not really suited here.

Thanks
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu Aug. 9, 2016, 8:58 a.m. UTC | #4

On Tue, Aug 09, 2016 at 10:07:29AM +0200, Stephan Mueller wrote:
> 
> Herbert, Matt, should such noise sources be added to the HW random framework? 
> The thing is that the in-kernel HW random to input_pool link per default uses 
> a more conservative entropy estimate than the user space rngd. I would think 
> that the in-kernel link would appropriate for that rng. But the user space 
> rngd tool with its default behavior is not really suited here.

Yes hwrng would be the best fit, with a quality of zero to be safe.

Contrary to the quoted thread, there is no need to whiten the output
/dev/hw_random.  It was always meant to go through some intermediate
processing such as rngd before it is used.

Cheers,

Stephan Mueller Aug. 9, 2016, 9:02 a.m. UTC | #5

Am Dienstag, 9. August 2016, 16:58:58 CEST schrieb Herbert Xu:

Hi Herbert,

> On Tue, Aug 09, 2016 at 10:07:29AM +0200, Stephan Mueller wrote:
> > Herbert, Matt, should such noise sources be added to the HW random
> > framework? The thing is that the in-kernel HW random to input_pool link
> > per default uses a more conservative entropy estimate than the user space
> > rngd. I would think that the in-kernel link would appropriate for that
> > rng. But the user space rngd tool with its default behavior is not really
> > suited here.
> 
> Yes hwrng would be the best fit, with a quality of zero to be safe.
> 
> Contrary to the quoted thread, there is no need to whiten the output
> /dev/hw_random.  It was always meant to go through some intermediate
> processing such as rngd before it is used.

But shouldn't the default of the rngd then be adjusted a bit?
> 
> Cheers,



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu Aug. 9, 2016, 9:17 a.m. UTC | #6

On Tue, Aug 09, 2016 at 11:02:58AM +0200, Stephan Mueller wrote:
> 
> But shouldn't the default of the rngd then be adjusted a bit?

Please elaborate.

Thanks,

Stephan Mueller Aug. 9, 2016, 9:37 a.m. UTC | #7

Am Dienstag, 9. August 2016, 17:17:55 CEST schrieb Herbert Xu:

Hi Herbert,

> On Tue, Aug 09, 2016 at 11:02:58AM +0200, Stephan Mueller wrote:
> > But shouldn't the default of the rngd then be adjusted a bit?
> 
> Please elaborate.

in rngd_linux.c:random_add_entropy(void *buf, size_t size):

        entropy.ent_count = size * 8;
        entropy.size = size;
        memcpy(entropy.data, buf, size);

        if (ioctl(random_fd, RNDADDENTROPY, &entropy) != 0) {

...


in rngd.c:do_loop():

                        retval = iter->xread(buf, sizeof buf, iter);
...
                        rc = update_kernel_random(random_step,
                                             buf, iter->fipsctx);

where update_kernel_random simply invokes random_add_entropy in chunks.

Hence, the rngd reads some bytes from /dev/hwrand and injects it into /dev/
random with an entropy estimate that is equal to the read bytes.

With less than perfect noise sources, entropy.ent_count should be much 
smaller.
> 
> Thanks,



Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu Aug. 9, 2016, 9:46 a.m. UTC | #8

On Tue, Aug 09, 2016 at 11:37:39AM +0200, Stephan Mueller wrote:
> Am Dienstag, 9. August 2016, 17:17:55 CEST schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Tue, Aug 09, 2016 at 11:02:58AM +0200, Stephan Mueller wrote:
> > > But shouldn't the default of the rngd then be adjusted a bit?
> > 
> > Please elaborate.
> 
> in rngd_linux.c:random_add_entropy(void *buf, size_t size):
> 
>         entropy.ent_count = size * 8;
>         entropy.size = size;
>         memcpy(entropy.data, buf, size);
> 
>         if (ioctl(random_fd, RNDADDENTROPY, &entropy) != 0) {
> 
> ...
> 
> 
> in rngd.c:do_loop():
> 
>                         retval = iter->xread(buf, sizeof buf, iter);
> ...
>                         rc = update_kernel_random(random_step,
>                                              buf, iter->fipsctx);
> 
> where update_kernel_random simply invokes random_add_entropy in chunks.
> 
> Hence, the rngd reads some bytes from /dev/hwrand and injects it into /dev/
> random with an entropy estimate that is equal to the read bytes.
> 
> With less than perfect noise sources, entropy.ent_count should be much 
> smaller.

You're supposed to tweak the quality of the input.  In any case,
this is not affected by whether we whiten the result.

Cheers,

Stephan Mueller Aug. 9, 2016, 9:56 a.m. UTC | #9

Am Dienstag, 9. August 2016, 17:46:56 CEST schrieb Herbert Xu:

Hi Herbert,
> 
> You're supposed to tweak the quality of the input.  In any case,

How is that tweak supposed to happen? The rngd does not allow changing the 
amount of read data relative to the assumed entropy.

> this is not affected by whether we whiten the result.

I understand that.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Herbert Xu Aug. 9, 2016, 9:56 a.m. UTC | #10

On Tue, Aug 09, 2016 at 11:56:08AM +0200, Stephan Mueller wrote:
> Am Dienstag, 9. August 2016, 17:46:56 CEST schrieb Herbert Xu:
> 
> Hi Herbert,
> > 
> > You're supposed to tweak the quality of the input.  In any case,
> 
> How is that tweak supposed to happen? The rngd does not allow changing the 
> amount of read data relative to the assumed entropy.

Hmm, I guess it depends on your distro.  Some do.

Cheers,

Stephan Mueller Aug. 9, 2016, 10:06 a.m. UTC | #11

Am Dienstag, 9. August 2016, 17:56:57 CEST schrieb Herbert Xu:

Hi Herbert,

> On Tue, Aug 09, 2016 at 11:56:08AM +0200, Stephan Mueller wrote:
> > Am Dienstag, 9. August 2016, 17:46:56 CEST schrieb Herbert Xu:
> > 
> > Hi Herbert,
> > 
> > > You're supposed to tweak the quality of the input.  In any case,
> > 
> > How is that tweak supposed to happen? The rngd does not allow changing the
> > amount of read data relative to the assumed entropy.
> 
> Hmm, I guess it depends on your distro.  Some do.
> 
> Cheers,

RHEL 7 and Fedora do not adjust it. So, shall we consider those rng-tools then 
broken (at least in those large distros)?

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Henrique de Moraes Holschuh Aug. 9, 2016, 10:24 a.m. UTC | #12

On Tue, 09 Aug 2016, Stephan Mueller wrote:
> RHEL 7 and Fedora do not adjust it. So, shall we consider those rng-tools then 
> broken (at least in those large distros)?

Might I humbly suggest that the kernel start providing some metatada
about the quality of the random source that userspace can consume?
Preferably by a new ioctl, so that it will fit naturally if we ever
extend /dev/hwrng to support more than one source?

That would allow for auto tunning to be implemented in userspace...

Jason Cooper Aug. 9, 2016, 5:48 p.m. UTC | #13

On Tue, Aug 09, 2016 at 03:02:27PM +0800, [e-mail] wrote:
> From: Miaoqing Pan <[e-mail]>
> 
> ath9k RNG will dominates all the noise sources from the real HW
> RNG, disable it by default. But we strongly recommand to enable
> it if the system without HW RNG, especially on embedded systems.
> 
> Signed-off-by: Miaoqing Pan <[e-mail]>

Until we get the hw_random/get_device_randomness question sorted...

Reviewed-by: Jason Cooper <[e-mail]>

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Cooper Aug. 9, 2016, 5:51 p.m. UTC | #14

Hi Henrique,

On Tue, Aug 09, 2016 at 07:24:58AM -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 09 Aug 2016, Stephan Mueller wrote:
> > RHEL 7 and Fedora do not adjust it. So, shall we consider those rng-tools then 
> > broken (at least in those large distros)?
> 
> Might I humbly suggest that the kernel start providing some metatada
> about the quality of the random source that userspace can consume?
> Preferably by a new ioctl, so that it will fit naturally if we ever
> extend /dev/hwrng to support more than one source?
> 
> That would allow for auto tunning to be implemented in userspace...

See my reply to Keith Packard's proposed multi-device hw_random patch:

  https://lkml.kernel.org/r/[e-mail]

and top of thread:

  https://lkml.kernel.org/r/[e-mail]

thx,

Jason.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

miaoqing pan Aug. 10, 2016, 2:35 a.m. UTC | #15

Hi Stephan,

For those less perfect noise source, can't pass the FIPS test.

static int update_kernel_random(int random_step,
        unsigned char *buf, fips_ctx_t *fipsctx_in)
{
        unsigned char *p; 
        int fips;

        fips = fips_run_rng_test(fipsctx_in, buf);
        if (fips)
                return 1;

        for (p = buf; p + random_step <= &buf[FIPS_RNG_BUFFER_SIZE];
                 p += random_step) {
                random_add_entropy(p, random_step);
                random_sleep();
        }
        return 0;
}

--
Miaoqing

Stephan Mueller Aug. 10, 2016, 5:29 a.m. UTC | #16

Am Mittwoch, 10. August 2016, 02:35:04 CEST schrieb Pan, Miaoqing:

Hi Miaoqing,

> Hi Stephan,
> 
> For those less perfect noise source, can't pass the FIPS test.
> 
> static int update_kernel_random(int random_step,
>         unsigned char *buf, fips_ctx_t *fipsctx_in)
> {
>         unsigned char *p;
>         int fips;
> 
>         fips = fips_run_rng_test(fipsctx_in, buf);
>         if (fips)
>                 return 1;
> 
>         for (p = buf; p + random_step <= &buf[FIPS_RNG_BUFFER_SIZE];
>                  p += random_step) {
>                 random_add_entropy(p, random_step);
>                 random_sleep();
>         }
>         return 0;
> }

Not even the poor cheap AIS20 statistical tests from rngd pass?

I guess the only sensible solution is what Ted suggested to use 
add_device_randomness.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

miaoqing pan Aug. 10, 2016, 6:04 a.m. UTC | #17

Hi Stephan,

FIPS RNG test is supposed to be run on the output of an RNG, and not on the RNG entropy source. It is not surprising that the RNG input fails the entropy tests from NIST. Check the following example.

Imagine you have a perfectly random sequence, a_1, a_2, .., a_n, where each a_i is a byte. And imagine, this sequence passes all randomness tests.

Now, let's say I create a new sequence a_1, 0, a_2, 0, a_3, 0, ..., 0, a_n, where each zero is a byte

If you give this sequence (as an entropy source) to a randomness test, it will fail most of the tests, if not all. This does not mean this sequence is not appropriate as an entropy source, it just means we need twice more bytes to gain the same amount of entropy.

I can give this 2n byte sequence to an RNG as an entropy source and it provides the same amount of security as if I give the n byte stream.

Thanks,
Miaoqing

-----Original Message-----
From: Stephan Mueller [mailto:[e-mail]] 
Sent: Wednesday, August 10, 2016 1:29 PM
To: Pan, Miaoqing <[e-mail]>
Cc: Herbert Xu <[e-mail]>; Matt Mackall <[e-mail]>; [e-mail]; Valo, Kalle <[e-mail]>; [e-mail]; ath9k-devel <[e-mail]>; [e-mail]; [e-mail]; Sepehrdad, Pouyan <[e-mail]>
Subject: Re: [PATCH 2/2] ath9k: disable RNG by default

Am Mittwoch, 10. August 2016, 02:35:04 CEST schrieb Pan, Miaoqing:

Hi Miaoqing,

> Hi Stephan,
> 
> For those less perfect noise source, can't pass the FIPS test.
> 
> static int update_kernel_random(int random_step,
>         unsigned char *buf, fips_ctx_t *fipsctx_in) {
>         unsigned char *p;
>         int fips;
> 
>         fips = fips_run_rng_test(fipsctx_in, buf);
>         if (fips)
>                 return 1;
> 
>         for (p = buf; p + random_step <= &buf[FIPS_RNG_BUFFER_SIZE];
>                  p += random_step) {
>                 random_add_entropy(p, random_step);
>                 random_sleep();
>         }
>         return 0;
> }

Not even the poor cheap AIS20 statistical tests from rngd pass?

I guess the only sensible solution is what Ted suggested to use add_device_randomness.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kalle Valo Sept. 28, 2016, 10 a.m. UTC | #18

miaoqing pan <[e-mail]> wrote:
> From: Miaoqing Pan <[e-mail]>
> 
> ath9k RNG will dominates all the noise sources from the real HW
> RNG, disable it by default. But we strongly recommand to enable
> it if the system without HW RNG, especially on embedded systems.
> 
> Signed-off-by: Miaoqing Pan <[e-mail]>
> Acked-by: Stephan Mueller <[e-mail]>
> Acked-by: Stephan Mueller <[e-mail]>
> Reviewed-by: Jason Cooper <[e-mail]>

Patch applied to ath-next branch of ath.git, thanks.

739ccd76b40e ath9k: disable RNG by default

Patch

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index f68cb00..8f231c6 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -180,7 +180,7 @@  config ATH9K_HTC_DEBUGFS
 config ATH9K_HWRNG
 	bool "Random number generator support"
 	depends on ATH9K && (HW_RANDOM = y || HW_RANDOM = ATH9K)
-	default y
+	default n
 	---help---
 	  This option incorporates the ADC register output as a source of
 	  randomness into Linux entropy pool (/dev/urandom and /dev/random)