linux-kernel - Re: /dev/random changes for 3.13

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20131116003324.GC16722@thunk.org>
Date:	Fri, 15 Nov 2013 19:33:24 -0500
From:	Theodore Ts'o <tytso@....edu>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: /dev/random changes for 3.13

On Fri, Nov 15, 2013 at 01:58:05PM -0800, Linus Torvalds wrote:
> Ok, I finally got around to the random tree, but your proposed merge
> resolution makes no sense, so I didn't end up applying it.
> 
> > - -     r->entropy_total += nbits;
> >         if (!r->initialized && nbits > 0) {
> > +               r->entropy_total += nbits;
> 
> This part undoes your commit 6265e169cd31 ("random: push extra entropy
> to the output pools"), and the "entropy_total" field will now never be
> non-zero when "r->initialized" is set....

You're right, I totally screwed that up.  Part of the problem is that
I should have cleaned up the if statement in commit 6265e169cd31 so
that it looked like this:

        r->entropy_total += nbits;
        if (!r->initialized && (r->entropy_total > 128)) {
                        r->initialized = 1;
                        r->entropy_total = 0;
                }
        }

... and so at the top of the dev branch, it should have looked like
this:

        r->entropy_total += nbits;
        if (!r->initialized && (r->entropy_total > 128)) {
		if (r == &nonblocking_pool)
			pr_notice("random: %s pool is initialized\n", r->name);
		r->initialized = 1;
		r->entropy_total = 0;
        }

Instead, I had a more complicated structure which confused me when I
cleaned up the merge.

So I'm going to propose the following merge resolution:

diff --cc drivers/char/random.c
index cdf4cfb,4fe5609..0000000
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@@ -654,14 -601,12 +654,13 @@@ retry
  	if (cmpxchg(&r->entropy_count, orig, entropy_count) != orig)
  		goto retry;
  
 -	if (!r->initialized && nbits > 0) {
 -		r->entropy_total += nbits;
 -		if (r->entropy_total > 128) {
 -			r->initialized = 1;
 -			if (r == &nonblocking_pool)
 -				prandom_reseed_late();
 +	r->entropy_total += nbits;
- 	if (!r->initialized && nbits > 0) {
- 		if (r->entropy_total > 128) {
- 			if (r == &nonblocking_pool)
- 				pr_notice("random: %s pool is initialized\n",
- 					  r->name);
- 			r->initialized = 1;
- 			r->entropy_total = 0;
++	if (!r->initialized && (r->entropy_total > 128)) {
++		r->initialized = 1;
++		r->entropy_total = 0;
++		if (r == &nonblocking_pool) {
++			prandom_reseed_late();
++			pr_notice("random: %s pool is initialized\n", r->name);
  		}
  	}

It does fold a cleanup in with the merge resolution, but it's not
actually changing anything in terms of code behavior, and it makes the
resulting merge much more obvious.

Are you OK with that?

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/