[PATCH net-next 1/2] random32: improvements to prandom_bytes

[PATCH net-next 1/2] random32: improvements to prandom_bytes

[Daniel Borkmann]

This patch addresses a couple of minor items, mostly addressing
prandom_bytes(): 1) prandom_bytes{,_state}() should use size_t
for length arguments, 2) We can use put_unaligned() when filling
the array instead of open coding it [ perhaps some archs will
further benefit from their own arch specific implementation when
GCC cannot make up for it ], 3) Fix a typo, 4) Better use unsigned
int as type for getting the arch seed, 5) Make use of
prandom_u32_max() for timer slack.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/random.h |  4 ++--
 lib/random32.c         | 39 ++++++++++++++++++---------------------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/include/linux/random.h b/include/linux/random.h
index 57fbbff..b05856e 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -26,7 +26,7 @@ unsigned int get_random_int(void);
 unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
 
 u32 prandom_u32(void);
-void prandom_bytes(void *buf, int nbytes);
+void prandom_bytes(void *buf, size_t nbytes);
 void prandom_seed(u32 seed);
 void prandom_reseed_late(void);
 
@@ -35,7 +35,7 @@ struct rnd_state {
 };
 
 u32 prandom_u32_state(struct rnd_state *state);
-void prandom_bytes_state(struct rnd_state *state, void *buf, int nbytes);
+void prandom_bytes_state(struct rnd_state *state, void *buf, size_t nbytes);
 
 /**
  * prandom_u32_max - returns a pseudo-random number in interval [0, ep_ro)
diff --git a/lib/random32.c b/lib/random32.c
index c9b6bf3..ce29432 100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -37,6 +37,7 @@
 #include <linux/jiffies.h>
 #include <linux/random.h>
 #include <linux/sched.h>
+#include <asm/unaligned.h>
 
 #ifdef CONFIG_RANDOM32_SELFTEST
 static void __init prandom_state_selftest(void);
@@ -96,27 +97,23 @@ EXPORT_SYMBOL(prandom_u32);
  *	This is used for pseudo-randomness with no outside seeding.
  *	For more random results, use prandom_bytes().
  */
-void prandom_bytes_state(struct rnd_state *state, void *buf, int bytes)
+void prandom_bytes_state(struct rnd_state *state, void *buf, size_t bytes)
 {
-	unsigned char *p = buf;
-	int i;
-
-	for (i = 0; i < round_down(bytes, sizeof(u32)); i += sizeof(u32)) {
-		u32 random = prandom_u32_state(state);
-		int j;
+	u8 *ptr = buf;
 
-		for (j = 0; j < sizeof(u32); j++) {
-			p[i + j] = random;
-			random >>= BITS_PER_BYTE;
-		}
+	while (bytes > sizeof(u32)) {
+		put_unaligned(prandom_u32_state(state), (u32 *) ptr);
+		ptr += sizeof(u32);
+		bytes -= sizeof(u32);
 	}
-	if (i < bytes) {
-		u32 random = prandom_u32_state(state);
 
-		for (; i < bytes; i++) {
-			p[i] = random;
-			random >>= BITS_PER_BYTE;
-		}
+	if (bytes > 0) {
+		u32 rem = prandom_u32_state(state);
+		do {
+			*ptr++ = (u8) rem;
+			bytes--;
+			rem >>= BITS_PER_BYTE;
+		} while (bytes > 0);
 	}
 }
 EXPORT_SYMBOL(prandom_bytes_state);
@@ -126,7 +123,7 @@ EXPORT_SYMBOL(prandom_bytes_state);
  *	@buf: where to copy the pseudo-random bytes to
  *	@bytes: the requested number of bytes
  */
-void prandom_bytes(void *buf, int bytes)
+void prandom_bytes(void *buf, size_t bytes)
 {
 	struct rnd_state *state = &get_cpu_var(net_rand_state);
 
@@ -137,7 +134,7 @@ EXPORT_SYMBOL(prandom_bytes);
 
 static void prandom_warmup(struct rnd_state *state)
 {
-	/* Calling RNG ten times to satify recurrence condition */
+	/* Calling RNG ten times to satisfy recurrence condition */
 	prandom_u32_state(state);
 	prandom_u32_state(state);
 	prandom_u32_state(state);
@@ -152,7 +149,7 @@ static void prandom_warmup(struct rnd_state *state)
 
 static u32 __extract_hwseed(void)
 {
-	u32 val = 0;
+	unsigned int val = 0;
 
 	(void)(arch_get_random_seed_int(&val) ||
 	       arch_get_random_int(&val));
@@ -228,7 +225,7 @@ static void __prandom_timer(unsigned long dontcare)
 	prandom_seed(entropy);
 
 	/* reseed every ~60 seconds, in [40 .. 80) interval with slack */
-	expires = 40 + (prandom_u32() % 40);
+	expires = 40 + prandom_u32_max(40);
 	seed_timer.expires = jiffies + msecs_to_jiffies(expires * MSEC_PER_SEC);
 
 	add_timer(&seed_timer);
-- 
1.7.11.7

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 31 Jul 2014 22:11:16 +0200

> -		for (j = 0; j < sizeof(u32); j++) {
> -			p[i + j] = random;
> -			random >>= BITS_PER_BYTE;
> -		}
> +	while (bytes > sizeof(u32)) {
> +		put_unaligned(prandom_u32_state(state), (u32 *) ptr);
> +		ptr += sizeof(u32);
> +		bytes -= sizeof(u32);
>  	}
> -	if (i < bytes) {
> -		u32 random = prandom_u32_state(state);
>  
> -		for (; i < bytes; i++) {
> -			p[i] = random;
> -			random >>= BITS_PER_BYTE;
> -		}
> +	if (bytes > 0) {
> +		u32 rem = prandom_u32_state(state);
> +		do {
> +			*ptr++ = (u8) rem;
> +			bytes--;
> +			rem >>= BITS_PER_BYTE;
> +		} while (bytes > 0);
>  	}

This conversion to put_unaligned() is not an equivalent depending
upon the endianness of the cpu.

And this means the random value gets distributed differently into
full words than it will into trailing bytes.

Let's just not mess around with this, ok?

Thanks.

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[Daniel Borkmann]

[ Cc'ing Akinobu, 6582c665d6b882 ("prandom: introduce prandom_bytes()
   and prandom_bytes_state()") ]

On 08/01/2014 07:28 AM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu, 31 Jul 2014 22:11:16 +0200
>
>> -		for (j = 0; j < sizeof(u32); j++) {
>> -			p[i + j] = random;
>> -			random >>= BITS_PER_BYTE;
>> -		}
>> +	while (bytes > sizeof(u32)) {

[ Should have been >= here, missed that earlier. ]

>> +		put_unaligned(prandom_u32_state(state), (u32 *) ptr);
>> +		ptr += sizeof(u32);
>> +		bytes -= sizeof(u32);
>>   	}
>> -	if (i < bytes) {
>> -		u32 random = prandom_u32_state(state);
>>
>> -		for (; i < bytes; i++) {
>> -			p[i] = random;
>> -			random >>= BITS_PER_BYTE;
>> -		}
>> +	if (bytes > 0) {
>> +		u32 rem = prandom_u32_state(state);
>> +		do {
>> +			*ptr++ = (u8) rem;
>> +			bytes--;
>> +			rem >>= BITS_PER_BYTE;
>> +		} while (bytes > 0);
>>   	}
>
> This conversion to put_unaligned() is not an equivalent depending
> upon the endianness of the cpu.
>
> And this means the random value gets distributed differently into
> full words than it will into trailing bytes.
>
> Let's just not mess around with this, ok?

Sorry for the late answer, Dave.

So for the case of callers of prandom_bytes() which internally
invoke prandom_bytes_state(), it doesn't matter at all since, well,
they expect the array to be filled randomly and don't have any
control of the internal state anyway (that's also why we have
periodic reseeding there, etc), so they really don't care.

Now for the direct callers of prandom_bytes_state(), which are
solely located in drivers/mtd/tests/{oobtest.c, pagetest.c,
subpagetest.c}:

These tests basically fill a test write-vector through prandom_bytes_state()
with an a-priori defined seed each time and write that to a MTD
device.

Later on, they set up a read-vector and read back that blocks from
the device. So in the verification phase, the write-vector is being
re-setup [ so same seed and prandom_bytes_state() called ], and then
memcmp()'ed against the read-vector to check if the data is the same.

Akinobu and I also tested this patch with the above fix included,
and it runs through the 3 relevant test cases _without_ any errors
on the nandsim device (simulator for MTD devs):

# modprobe nandsim first_id_byte=0x20 second_id_byte=0xac \
                    third_id_byte=0x00 fourth_id_byte=0x15
# modprobe mtd_oobtest dev=0
# modprobe mtd_pagetest dev=0
# modprobe mtd_subpagetest dev=0

[ ... 'finished with 0 errors' each in klog ]

We also don't have any users depending directly on a particular
result of the PRNG (except the PRNG self-test itself), and that's
just fine as it e.g. allowed us easily to do things like upgrading
from taus88 to taus113.

Thanks,

Daniel

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Mon, 04 Aug 2014 11:48:33 +0200

> Akinobu and I also tested this patch with the above fix included,
> and it runs through the 3 relevant test cases _without_ any errors
> on the nandsim device (simulator for MTD devs):

On one endianness only perhaps?

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[Daniel Borkmann]

On 08/04/2014 08:41 PM, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Mon, 04 Aug 2014 11:48:33 +0200
>
>> Akinobu and I also tested this patch with the above fix included,
>> and it runs through the 3 relevant test cases _without_ any errors
>> on the nandsim device (simulator for MTD devs):
>
> On one endianness only perhaps?

On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6).

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[Daniel Borkmann]

On 08/05/2014 05:16 PM, Daniel Borkmann wrote:
> On 08/04/2014 08:41 PM, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Mon, 04 Aug 2014 11:48:33 +0200
>>
>>> Akinobu and I also tested this patch with the above fix included,
>>> and it runs through the 3 relevant test cases _without_ any errors
>>> on the nandsim device (simulator for MTD devs):
>>
>> On one endianness only perhaps?
>
> On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6).

Not really urgent, so if there are no further objections,
I'd respin it with the code fix included, proper Tested-by
tags from MTD people, and updated commit message when
net-next reopens again.

Thanks,

Daniel

Re: [PATCH net-next 1/2] random32: improvements to prandom_bytes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 06 Aug 2014 12:12:33 +0200

> On 08/05/2014 05:16 PM, Daniel Borkmann wrote:
>> On 08/04/2014 08:41 PM, David Miller wrote:
>>> From: Daniel Borkmann <dborkman@redhat.com>
>>> Date: Mon, 04 Aug 2014 11:48:33 +0200
>>>
>>>> Akinobu and I also tested this patch with the above fix included,
>>>> and it runs through the 3 relevant test cases _without_ any errors
>>>> on the nandsim device (simulator for MTD devs):
>>>
>>> On one endianness only perhaps?
>>
>> On x86_64, ppc64, ARM (i.MX28, i.MX53 and i.MX6).
> 
> Not really urgent, so if there are no further objections,
> I'd respin it with the code fix included, proper Tested-by
> tags from MTD people, and updated commit message when
> net-next reopens again.

Ok, please do.

[PATCH net-next 1/2] random32: improvements to prandom_bytes

[Daniel Borkmann]

This patch addresses a couple of minor items, mostly addressing
prandom_bytes(): 1) prandom_bytes{,_state}() should use size_t
for length arguments, 2) We can use put_unaligned() when filling
the array instead of open coding it [ perhaps some archs will
further benefit from their own arch specific implementation when
GCC cannot make up for it ], 3) Fix a typo, 4) Better use unsigned
int as type for getting the arch seed, 5) Make use of
prandom_u32_max() for timer slack.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/random.h |  4 ++--
 lib/random32.c         | 39 ++++++++++++++++++---------------------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/include/linux/random.h b/include/linux/random.h
index 57fbbff..b05856e 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -26,7 +26,7 @@ unsigned int get_random_int(void);
 unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
 
 u32 prandom_u32(void);
-void prandom_bytes(void *buf, int nbytes);
+void prandom_bytes(void *buf, size_t nbytes);
 void prandom_seed(u32 seed);
 void prandom_reseed_late(void);
 
@@ -35,7 +35,7 @@ struct rnd_state {
 };
 
 u32 prandom_u32_state(struct rnd_state *state);
-void prandom_bytes_state(struct rnd_state *state, void *buf, int nbytes);
+void prandom_bytes_state(struct rnd_state *state, void *buf, size_t nbytes);
 
 /**
  * prandom_u32_max - returns a pseudo-random number in interval [0, ep_ro)
diff --git a/lib/random32.c b/lib/random32.c
index c9b6bf3..ce29432 100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -37,6 +37,7 @@
 #include <linux/jiffies.h>
 #include <linux/random.h>
 #include <linux/sched.h>
+#include <asm/unaligned.h>
 
 #ifdef CONFIG_RANDOM32_SELFTEST
 static void __init prandom_state_selftest(void);
@@ -96,27 +97,23 @@ EXPORT_SYMBOL(prandom_u32);
  *	This is used for pseudo-randomness with no outside seeding.
  *	For more random results, use prandom_bytes().
  */
-void prandom_bytes_state(struct rnd_state *state, void *buf, int bytes)
+void prandom_bytes_state(struct rnd_state *state, void *buf, size_t bytes)
 {
-	unsigned char *p = buf;
-	int i;
-
-	for (i = 0; i < round_down(bytes, sizeof(u32)); i += sizeof(u32)) {
-		u32 random = prandom_u32_state(state);
-		int j;
+	u8 *ptr = buf;
 
-		for (j = 0; j < sizeof(u32); j++) {
-			p[i + j] = random;
-			random >>= BITS_PER_BYTE;
-		}
+	while (bytes > sizeof(u32)) {
+		put_unaligned(prandom_u32_state(state), (u32 *) ptr);
+		ptr += sizeof(u32);
+		bytes -= sizeof(u32);
 	}
-	if (i < bytes) {
-		u32 random = prandom_u32_state(state);
 
-		for (; i < bytes; i++) {
-			p[i] = random;
-			random >>= BITS_PER_BYTE;
-		}
+	if (bytes > 0) {
+		u32 rem = prandom_u32_state(state);
+		do {
+			*ptr++ = (u8) rem;
+			bytes--;
+			rem >>= BITS_PER_BYTE;
+		} while (bytes > 0);
 	}
 }
 EXPORT_SYMBOL(prandom_bytes_state);
@@ -126,7 +123,7 @@ EXPORT_SYMBOL(prandom_bytes_state);
  *	@buf: where to copy the pseudo-random bytes to
  *	@bytes: the requested number of bytes
  */
-void prandom_bytes(void *buf, int bytes)
+void prandom_bytes(void *buf, size_t bytes)
 {
 	struct rnd_state *state = &get_cpu_var(net_rand_state);
 
@@ -137,7 +134,7 @@ EXPORT_SYMBOL(prandom_bytes);
 
 static void prandom_warmup(struct rnd_state *state)
 {
-	/* Calling RNG ten times to satify recurrence condition */
+	/* Calling RNG ten times to satisfy recurrence condition */
 	prandom_u32_state(state);
 	prandom_u32_state(state);
 	prandom_u32_state(state);
@@ -152,7 +149,7 @@ static void prandom_warmup(struct rnd_state *state)
 
 static u32 __extract_hwseed(void)
 {
-	u32 val = 0;
+	unsigned int val = 0;
 
 	(void)(arch_get_random_seed_int(&val) ||
 	       arch_get_random_int(&val));
@@ -228,7 +225,7 @@ static void __prandom_timer(unsigned long dontcare)
 	prandom_seed(entropy);
 
 	/* reseed every ~60 seconds, in [40 .. 80) interval with slack */
-	expires = 40 + (prandom_u32() % 40);
+	expires = 40 + prandom_u32_max(40);
 	seed_timer.expires = jiffies + msecs_to_jiffies(expires * MSEC_PER_SEC);
 
 	add_timer(&seed_timer);
-- 
1.7.11.7