[PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Sasha Levin]

Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
nonblocking pool becomes initialized") has added a late reseed stage
that happens as soon as the nonblocking pool is marked as initialized.

This fails in the case that the nonblocking pool gets initialized
during __prandom_reseed()'s call to get_random_bytes(). In that case
we'd double back into __prandom_reseed() in an attempt to do a late
reseed - deadlocking on 'lock' early on in the boot process.

Instead, just avoid even waiting to do a reseed if a reseed is already
occuring.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 lib/random32.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/random32.c b/lib/random32.c
index 1e5b2df..b59da12 100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -241,14 +241,27 @@ static void __prandom_reseed(bool late)
 {
 	int i;
 	unsigned long flags;
-	static bool latch = false;
+	static bool latch = false, reseeding = false;
 	static DEFINE_SPINLOCK(lock);
 
+	/*
+	 * Asking for random bytes might result in bytes getting
+	 * moved into the nonblocking pool and thus marking it
+	 * as initialized. In this case we would double back into
+	 * this function and attempt to do a late reseed.
+	 * Ignore the pointless attempt to reseed again if we're
+	 * already waiting for bytes when the nonblocking pool
+	 * got initialized
+	 */
+	if (reseeding)
+		return;
+
 	/* only allow initial seeding (late == false) once */
 	spin_lock_irqsave(&lock, flags);
 	if (latch && !late)
 		goto out;
 	latch = true;
+	reseeding = true;
 
 	for_each_possible_cpu(i) {
 		struct rnd_state *state = &per_cpu(net_rand_state,i);
@@ -263,6 +276,7 @@ static void __prandom_reseed(bool late)
 		prandom_warmup(state);
 	}
 out:
+	reseeding = false;
 	spin_unlock_irqrestore(&lock, flags);
 }
 
-- 
1.8.3.2

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Hannes Frederic Sowa]

On Wed, Mar 26, 2014 at 01:12:38PM -0400, Sasha Levin wrote:
> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
> nonblocking pool becomes initialized") has added a late reseed stage
> that happens as soon as the nonblocking pool is marked as initialized.
> 
> This fails in the case that the nonblocking pool gets initialized
> during __prandom_reseed()'s call to get_random_bytes(). In that case
> we'd double back into __prandom_reseed() in an attempt to do a late
> reseed - deadlocking on 'lock' early on in the boot process.
> 
> Instead, just avoid even waiting to do a reseed if a reseed is already
> occuring.
> 
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Ohh, I overlooked that. Thanks for fixing this, looks good.

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Daniel Borkmann]

On 03/26/2014 06:12 PM, Sasha Levin wrote:
> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
> nonblocking pool becomes initialized") has added a late reseed stage
> that happens as soon as the nonblocking pool is marked as initialized.
>
> This fails in the case that the nonblocking pool gets initialized
> during __prandom_reseed()'s call to get_random_bytes(). In that case
> we'd double back into __prandom_reseed() in an attempt to do a late
> reseed - deadlocking on 'lock' early on in the boot process.
>
> Instead, just avoid even waiting to do a reseed if a reseed is already
> occuring.
>
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>

Thanks for catching! (If you want Dave to pick it up, please also
Cc netdev.)

Why not via spin_trylock_irqsave() ? Thus, if we already hold the
lock, we do not bother any longer with doing the same work twice
and just return.

I.e. like:

static void __prandom_reseed(bool late)
{
     int i;
     unsigned long flags;
     static bool latch = false;
     static DEFINE_SPINLOCK(lock);

     /* Asking for random bytes might result in bytes getting
      * moved into the nonblocking pool and thus marking it
      * as initialized. In this case we would double back into
      * this function and attempt to do a late reseed.
      * Ignore the pointless attempt to reseed again if we're
      * already waiting for bytes when the nonblocking pool
      * got initialized.
      */

     /* only allow initial seeding (late == false) once */
     if (!spin_trylock_irqsave(&lock, flags))
         return;

     if (latch && !late)
         goto out;

     latch = true;

     for_each_possible_cpu(i) {
         struct rnd_state *state = &per_cpu(net_rand_state,i);
         u32 seeds[4];

         get_random_bytes(&seeds, sizeof(seeds));
         state->s1 = __seed(seeds[0],   2U);
         state->s2 = __seed(seeds[1],   8U);
         state->s3 = __seed(seeds[2],  16U);
         state->s4 = __seed(seeds[3], 128U);

         prandom_warmup(state);
     }
out:
     spin_unlock_irqrestore(&lock, flags);
}

> ---
>   lib/random32.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/lib/random32.c b/lib/random32.c
> index 1e5b2df..b59da12 100644
> --- a/lib/random32.c
> +++ b/lib/random32.c
> @@ -241,14 +241,27 @@ static void __prandom_reseed(bool late)
>   {
>   	int i;
>   	unsigned long flags;
> -	static bool latch = false;
> +	static bool latch = false, reseeding = false;
>   	static DEFINE_SPINLOCK(lock);
>
> +	/*
> +	 * Asking for random bytes might result in bytes getting
> +	 * moved into the nonblocking pool and thus marking it
> +	 * as initialized. In this case we would double back into
> +	 * this function and attempt to do a late reseed.
> +	 * Ignore the pointless attempt to reseed again if we're
> +	 * already waiting for bytes when the nonblocking pool
> +	 * got initialized
> +	 */
> +	if (reseeding)
> +		return;
> +
>   	/* only allow initial seeding (late == false) once */
>   	spin_lock_irqsave(&lock, flags);
>   	if (latch && !late)
>   		goto out;
>   	latch = true;
> +	reseeding = true;
>
>   	for_each_possible_cpu(i) {
>   		struct rnd_state *state = &per_cpu(net_rand_state,i);
> @@ -263,6 +276,7 @@ static void __prandom_reseed(bool late)
>   		prandom_warmup(state);
>   	}
>   out:
> +	reseeding = false;
>   	spin_unlock_irqrestore(&lock, flags);
>   }
>
>

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Sasha Levin]

On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
> On 03/26/2014 06:12 PM, Sasha Levin wrote:
>> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
>> nonblocking pool becomes initialized") has added a late reseed stage
>> that happens as soon as the nonblocking pool is marked as initialized.
>>
>> This fails in the case that the nonblocking pool gets initialized
>> during __prandom_reseed()'s call to get_random_bytes(). In that case
>> we'd double back into __prandom_reseed() in an attempt to do a late
>> reseed - deadlocking on 'lock' early on in the boot process.
>>
>> Instead, just avoid even waiting to do a reseed if a reseed is already
>> occuring.
>>
>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>
> Thanks for catching! (If you want Dave to pick it up, please also
> Cc netdev.)
>
> Why not via spin_trylock_irqsave() ? Thus, if we already hold the
> lock, we do not bother any longer with doing the same work twice
> and just return.

Your code looks much better, I'll should really stop sending patches
too early in the morning...

It's also worth adding lib/random32.c to the MAINTAINERS file, as my
list of recipients is solely based on what get_maintainer.pl tells
me to do (and I'm assuming that I'm not the last person who will be
sending patches for this).


Thanks,
Sasha

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Daniel Borkmann]

On 03/27/2014 12:35 AM, Sasha Levin wrote:
> On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
>> On 03/26/2014 06:12 PM, Sasha Levin wrote:
>>> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
>>> nonblocking pool becomes initialized") has added a late reseed stage
>>> that happens as soon as the nonblocking pool is marked as initialized.
>>>
>>> This fails in the case that the nonblocking pool gets initialized
>>> during __prandom_reseed()'s call to get_random_bytes(). In that case
>>> we'd double back into __prandom_reseed() in an attempt to do a late
>>> reseed - deadlocking on 'lock' early on in the boot process.
>>>
>>> Instead, just avoid even waiting to do a reseed if a reseed is already
>>> occuring.
>>>
>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>>
>> Thanks for catching! (If you want Dave to pick it up, please also
>> Cc netdev.)
>>
>> Why not via spin_trylock_irqsave() ? Thus, if we already hold the
>> lock, we do not bother any longer with doing the same work twice
>> and just return.
>
> Your code looks much better, I'll should really stop sending patches
> too early in the morning...
>
> It's also worth adding lib/random32.c to the MAINTAINERS file, as my
> list of recipients is solely based on what get_maintainer.pl tells
> me to do (and I'm assuming that I'm not the last person who will be
> sending patches for this).

Sounds like a good idea, afaik, historically, this got ripped out as
a lib from networking subsystem. That's why we've sent it to netdev
last time.

Cheers,

Daniel

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Hannes Frederic Sowa]

On Wed, Mar 26, 2014 at 07:35:01PM -0400, Sasha Levin wrote:
> On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
> >On 03/26/2014 06:12 PM, Sasha Levin wrote:
> >>Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
> >>nonblocking pool becomes initialized") has added a late reseed stage
> >>that happens as soon as the nonblocking pool is marked as initialized.
> >>
> >>This fails in the case that the nonblocking pool gets initialized
> >>during __prandom_reseed()'s call to get_random_bytes(). In that case
> >>we'd double back into __prandom_reseed() in an attempt to do a late
> >>reseed - deadlocking on 'lock' early on in the boot process.
> >>
> >>Instead, just avoid even waiting to do a reseed if a reseed is already
> >>occuring.
> >>
> >>Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >
> >Thanks for catching! (If you want Dave to pick it up, please also
> >Cc netdev.)
> >
> >Why not via spin_trylock_irqsave() ? Thus, if we already hold the
> >lock, we do not bother any longer with doing the same work twice
> >and just return.

I totally agree with Daniel spin_trylock_irqsave seems like the best
solution.

In case we really want to make sure that even early seeding doesn't
race with late seed and the pool is only filled by another CPU, we would
actually need per-cpu bools to get this case correct.

I really doubt this isn't worth the effort and wouldn't do that.

> Your code looks much better, I'll should really stop sending patches
> too early in the morning...
> 
> It's also worth adding lib/random32.c to the MAINTAINERS file, as my
> list of recipients is solely based on what get_maintainer.pl tells
> me to do (and I'm assuming that I'm not the last person who will be
> sending patches for this).

Would be a nice idea, especially because prandom_u32 changes are sensitive to
network security and should get reviewed there, too.

Greetings,

  Hannes

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Daniel Borkmann]

On 03/27/2014 03:21 AM, Hannes Frederic Sowa wrote:
> On Wed, Mar 26, 2014 at 07:35:01PM -0400, Sasha Levin wrote:
>> On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
>>> On 03/26/2014 06:12 PM, Sasha Levin wrote:
>>>> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
>>>> nonblocking pool becomes initialized") has added a late reseed stage
>>>> that happens as soon as the nonblocking pool is marked as initialized.
>>>>
>>>> This fails in the case that the nonblocking pool gets initialized
>>>> during __prandom_reseed()'s call to get_random_bytes(). In that case
>>>> we'd double back into __prandom_reseed() in an attempt to do a late
>>>> reseed - deadlocking on 'lock' early on in the boot process.
>>>>
>>>> Instead, just avoid even waiting to do a reseed if a reseed is already
>>>> occuring.
>>>>
>>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>>>
>>> Thanks for catching! (If you want Dave to pick it up, please also
>>> Cc netdev.)
>>>
>>> Why not via spin_trylock_irqsave() ? Thus, if we already hold the
>>> lock, we do not bother any longer with doing the same work twice
>>> and just return.
>
> I totally agree with Daniel spin_trylock_irqsave seems like the best
> solution.
>
> In case we really want to make sure that even early seeding doesn't
> race with late seed and the pool is only filled by another CPU, we would
> actually need per-cpu bools to get this case correct.

But then again, we would just exit via spin_trylock_irqsave()
now, no? Whenever something enters this section protected under
irq save spinlock we would do a reseed of the entire state (s1-s4)
for each cpu.

>> Your code looks much better, I'll should really stop sending patches
>> too early in the morning...
>>
>> It's also worth adding lib/random32.c to the MAINTAINERS file, as my
>> list of recipients is solely based on what get_maintainer.pl tells
>> me to do (and I'm assuming that I'm not the last person who will be
>> sending patches for this).
>
> Would be a nice idea, especially because prandom_u32 changes are sensitive to
> network security and should get reviewed there, too.

Indeed, sounds good to me.

> Greetings,
>
>    Hannes
>

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Hannes Frederic Sowa]

On Thu, Mar 27, 2014 at 10:04:03AM +0100, Daniel Borkmann wrote:
> On 03/27/2014 03:21 AM, Hannes Frederic Sowa wrote:
> >On Wed, Mar 26, 2014 at 07:35:01PM -0400, Sasha Levin wrote:
> >>On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
> >>>On 03/26/2014 06:12 PM, Sasha Levin wrote:
> >>>>Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
> >>>>nonblocking pool becomes initialized") has added a late reseed stage
> >>>>that happens as soon as the nonblocking pool is marked as initialized.
> >>>>
> >>>>This fails in the case that the nonblocking pool gets initialized
> >>>>during __prandom_reseed()'s call to get_random_bytes(). In that case
> >>>>we'd double back into __prandom_reseed() in an attempt to do a late
> >>>>reseed - deadlocking on 'lock' early on in the boot process.
> >>>>
> >>>>Instead, just avoid even waiting to do a reseed if a reseed is already
> >>>>occuring.
> >>>>
> >>>>Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
> >>>
> >>>Thanks for catching! (If you want Dave to pick it up, please also
> >>>Cc netdev.)
> >>>
> >>>Why not via spin_trylock_irqsave() ? Thus, if we already hold the
> >>>lock, we do not bother any longer with doing the same work twice
> >>>and just return.
> >
> >I totally agree with Daniel spin_trylock_irqsave seems like the best
> >solution.
> >
> >In case we really want to make sure that even early seeding doesn't
> >race with late seed and the pool is only filled by another CPU, we would
> >actually need per-cpu bools to get this case correct.
> 
> But then again, we would just exit via spin_trylock_irqsave()
> now, no? Whenever something enters this section protected under
> irq save spinlock we would do a reseed of the entire state (s1-s4)
> for each cpu.

If early reseed races with late one, we would actually need to spin on
maybe another cpu, so the early call can leave critical section before
late call enters. If we don't spin we could possibly miss the late call
when nonblocking pool is fully seeded (entropy may be added in batches
and first cpus of the early reseeding might miss better entropy).

If the early call blocks the late call, maybe even on another cpu, the late
call should spin until the early call left the critical section. We can only
deadlock on same cpu.

I consider this just hypothetical.

Bye,

  Hannes

Re: [PATCH] random32: avoid attempt to late reseed if in the middle of seeding

[Daniel Borkmann]

On 03/27/2014 10:18 AM, Hannes Frederic Sowa wrote:
> On Thu, Mar 27, 2014 at 10:04:03AM +0100, Daniel Borkmann wrote:
>> On 03/27/2014 03:21 AM, Hannes Frederic Sowa wrote:
>>> On Wed, Mar 26, 2014 at 07:35:01PM -0400, Sasha Levin wrote:
>>>> On 03/26/2014 07:18 PM, Daniel Borkmann wrote:
>>>>> On 03/26/2014 06:12 PM, Sasha Levin wrote:
>>>>>> Commit 4af712e8df ("random32: add prandom_reseed_late() and call when
>>>>>> nonblocking pool becomes initialized") has added a late reseed stage
>>>>>> that happens as soon as the nonblocking pool is marked as initialized.
>>>>>>
>>>>>> This fails in the case that the nonblocking pool gets initialized
>>>>>> during __prandom_reseed()'s call to get_random_bytes(). In that case
>>>>>> we'd double back into __prandom_reseed() in an attempt to do a late
>>>>>> reseed - deadlocking on 'lock' early on in the boot process.
>>>>>>
>>>>>> Instead, just avoid even waiting to do a reseed if a reseed is already
>>>>>> occuring.
>>>>>>
>>>>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>>>>>
>>>>> Thanks for catching! (If you want Dave to pick it up, please also
>>>>> Cc netdev.)
>>>>>
>>>>> Why not via spin_trylock_irqsave() ? Thus, if we already hold the
>>>>> lock, we do not bother any longer with doing the same work twice
>>>>> and just return.
>>>
>>> I totally agree with Daniel spin_trylock_irqsave seems like the best
>>> solution.
>>>
>>> In case we really want to make sure that even early seeding doesn't
>>> race with late seed and the pool is only filled by another CPU, we would
>>> actually need per-cpu bools to get this case correct.
>>
>> But then again, we would just exit via spin_trylock_irqsave()
>> now, no? Whenever something enters this section protected under
>> irq save spinlock we would do a reseed of the entire state (s1-s4)
>> for each cpu.
>
> If early reseed races with late one, we would actually need to spin on
> maybe another cpu, so the early call can leave critical section before
> late call enters. If we don't spin we could possibly miss the late call
> when nonblocking pool is fully seeded (entropy may be added in batches
> and first cpus of the early reseeding might miss better entropy).
>
> If the early call blocks the late call, maybe even on another cpu, the late
> call should spin until the early call left the critical section. We can only
> deadlock on same cpu.
>
> I consider this just hypothetical.

I think I agree with you, it seems hypothetical. We perhaps could have
tested e.g. if the nonblocking pool is initialized and in case not,
would have a fallback to get_random_bytes_arch() for that short time
to have presumably better 'entropy'. On the other hand, if the _arch()
version would have been backdoored by the NSA, as we possibly reseed
all states later on, or just s1 and xor it with all other states, it
seems that eventually there's still no real gain for doing that. So
the spin_trylock_irqsave() version seems okay ... plus prandom() API
is not cryptographically secure anyway.