linux-kernel - [PATCH 10/11] random: pull 'min' check in accounting to inside lockless update

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] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 7 Nov 2013 18:59:32 -0500
From:	Greg Price <price@....EDU>
To:	"Theodore Ts'o" <tytso@....edu>
Cc:	linux-kernel@...r.kernel.org, Jiri Kosina <jkosina@...e.cz>
Subject: [PATCH 10/11] random: pull 'min' check in accounting to inside
 lockless update

Since 902c098a3 ("random: use lockless techniques in the interrupt
path") we have protected entropy_count only with cmpxchg, not the
lock.  In 10b3a32d2 ("random: fix accounting race condition") we
put a cmpxchg-retry loop around most of the logic in account(),
but the enforcement of the 'min' parameter stayed outside.

Under concurrent accesses to /dev/urandom this can suffer a race
that defeats our "catastrophic reseed" measures so that our
reseeding isn't effective.  If accesses to /dev/urandom and
/dev/random are interleaved in the wrong pattern, we could go
indefinitely long without a successful catastrophic reseed of
/dev/urandom, even while consuming an indefinite amount of entropy
in ineffective smaller reseeds.

NB that with or without this bug, if /dev/random is simply
accessed constantly, we can go indefinitely long without any
reseed of /dev/urandom.  So the effect of the bug is not that big.

The race comes when two tasks are in account() on input_pool
and access ->entropy_count in the following order:

             task A                           task B
   if (r->entropy_count / 8
               < min + reserved)
                                   ACCESS_ONCE(r->entropy_count)
                                   cmpxchg
   ACCESS_ONCE(r->entropy_count)
   cmpxchg

where B causes r->entropy_count / 8 to fall below A's
min + reserved but above reserved.  Task A will cheerfully
take r->entropy_count / 8 - reserved bytes from the pool,
even though this is less than min.

Move the "min" check inside the ACCESS_ONCE/cmpxchg loop to
prevent the race.

Cc: Jiri Kosina <jkosina@...e.cz>
Cc: "Theodore Ts'o" <tytso@....edu>
Signed-off-by: Greg Price <price@....edu>
---
 drivers/char/random.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1bf6bf8..87d3728 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -842,18 +842,17 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min,
 {
 	unsigned long flags;
 	int wakeup_write = 0;
+	int entropy_count, orig;

 	BUG_ON(r->entropy_count > r->poolinfo->POOLBITS);
 	DEBUG_ENT("trying to extract %zu bits from %s\n",
 		  nbytes * 8, r->name);

-	/* Can we pull enough? */
-	if (r->entropy_count / 8 < min + reserved) {
+retry:
+	entropy_count = orig = ACCESS_ONCE(r->entropy_count);
+	if (entropy_count / 8 < min + reserved) {
 		nbytes = 0;
 	} else {
-		int entropy_count, orig;
-retry:
-		entropy_count = orig = ACCESS_ONCE(r->entropy_count);
 		/* If limited, never pull more than available */
 		if (r->limit)
 			nbytes = min_t(size_t, nbytes, entropy_count/8 - reserved);
-- 
1.8.3.2

--
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/