Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660404 > unrolled thread

[PATCH v5 00/13] Unseeded In-Kernel Randomness Fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/13] Unseeded In-Kernel Randomness Fixes "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 13/13] random: warn when kernel uses unseeded randomness "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 06/13] iscsi: ensure RNG is seeded before use "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 05/13] crypto/rng: ensure that the RNG is ready before using "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 11/13] net/route: use get_random_int for random counter "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 10/13] net/neighbor: use get_random_u32 for 32-bit hash random "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 08/13] cifs: use get_random_u32 for 32-bit lock random "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 02/13] random: add synchronous API for the urandom pool "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 01/13] random: invalidate batched entropy after crng init "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
      Re: [PATCH v5 01/13] random: invalidate batched entropy after crng  init Sebastian Andrzej Siewior <sebastian@breakpoint.cc> - 2017-06-14 21:40 +0200
        Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-15 00:40 +0200
          Re: [PATCH v5 01/13] random: invalidate batched entropy after crng  init Sebastian Andrzej Siewior <sebastian@breakpoint.cc> - 2017-06-16 10:40 +0200
            Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-16 14:20 +0200
              Re: [PATCH v5 01/13] random: invalidate batched entropy after crng  init Sebastian Andrzej Siewior <sebastian@breakpoint.cc> - 2017-06-16 16:40 +0200
        [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-15 00:50 +0200
          Re: [PATCH] random: silence compiler warnings and fix race Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-16 16:40 +0200
            Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-17 02:40 +0200
              Re: [PATCH] random: silence compiler warnings and fix race Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-06-19 09:50 +0200
                Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-19 23:00 +0200
          Re: [PATCH] random: silence compiler warnings and fix race "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-19 23:00 +0200
    [PATCH v5 03/13] random: add get_random_{bytes,u32,u64,int,long,once}_wait family "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200
    [PATCH v5 04/13] security/keys: ensure RNG is seeded before use "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 01:30 +0200

Page 1 of 2  [1] 2  Next page →

#1660404 — [PATCH v5 00/13] Unseeded In-Kernel Randomness Fixes

It looks like critique of this has come to an end. Could somebody take
this into their tree for 4.12?

As discussed in [1], there is a problem with get_random_bytes being
used before the RNG has actually been seeded. The solution for fixing
this appears to be multi-pronged. One of those prongs involves adding
a simple blocking API so that modules that use the RNG in process
context can just sleep (in an interruptable manner) until the RNG is
ready to be used. This winds up being a very useful API that covers
a few use cases, several of which are included in this patch set.

[1] http://www.openwall.com/lists/kernel-hardening/2017/06/02/2

Changes v4->v5:
  - Old versions of gcc warned on an uninitialized variable, so set
    this to silence warning.

Jason A. Donenfeld (13):
  random: invalidate batched entropy after crng init
  random: add synchronous API for the urandom pool
  random: add get_random_{bytes,u32,u64,int,long,once}_wait family
  security/keys: ensure RNG is seeded before use
  crypto/rng: ensure that the RNG is ready before using
  iscsi: ensure RNG is seeded before use
  ceph: ensure RNG is seeded before using
  cifs: use get_random_u32 for 32-bit lock random
  rhashtable: use get_random_u32 for hash_rnd
  net/neighbor: use get_random_u32 for 32-bit hash random
  net/route: use get_random_int for random counter
  bluetooth/smp: ensure RNG is properly seeded before ECDH use
  random: warn when kernel uses unseeded randomness

 crypto/rng.c                              |  6 +-
 drivers/char/random.c                     | 93 +++++++++++++++++++++++++++----
 drivers/target/iscsi/iscsi_target_auth.c  | 14 ++++-
 drivers/target/iscsi/iscsi_target_login.c | 22 +++++---
 fs/cifs/cifsfs.c                          |  2 +-
 include/linux/net.h                       |  2 +
 include/linux/once.h                      |  2 +
 include/linux/random.h                    | 26 +++++++++
 lib/Kconfig.debug                         | 16 ++++++
 lib/rhashtable.c                          |  2 +-
 net/bluetooth/hci_request.c               |  6 ++
 net/bluetooth/smp.c                       | 18 ++++--
 net/ceph/ceph_common.c                    |  6 +-
 net/core/neighbour.c                      |  3 +-
 net/ipv4/route.c                          |  3 +-
 security/keys/encrypted-keys/encrypted.c  |  8 ++-
 security/keys/key.c                       | 16 +++---
 17 files changed, 198 insertions(+), 47 deletions(-)

-- 
2.13.0

[toc] | [next] | [standalone]

#1660409 — [PATCH v5 13/13] random: warn when kernel uses unseeded randomness

This enables an important dmesg notification about when drivers have
used the crng without it being seeded first. Prior, these errors would
occur silently, and so there hasn't been a great way of diagnosing these
types of bugs for obscure setups. By adding this as a config option, we
can leave it on by default, so that we learn where these issues happen,
in the field, will still allowing some people to turn it off, if they
really know what they're doing and do not want the log entries.

However, we don't leave it _completely_ by default. An earlier version
of this patch simply had `default y`. I'd really love that, but it turns
out, this problem with unseeded randomness being used is really quite
present and is going to take a long time to fix. Thus, as a compromise
between log-messages-for-all and nobody-knows, this is `default y`,
except it is also `depends on DEBUG_KERNEL`. This will ensure that the
curious see the messages while others don't have to.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 15 +++++++++++++--
 lib/Kconfig.debug     | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 77587ac1ecb2..5252690610ec 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -288,7 +288,6 @@
 #define SEC_XFER_SIZE		512
 #define EXTRACT_SIZE		10
 
-#define DEBUG_RANDOM_BOOT 0
 
 #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
 
@@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
 {
 	__u8 tmp[CHACHA20_BLOCK_SIZE];
 
-#if DEBUG_RANDOM_BOOT > 0
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
 	if (!crng_ready())
 		printk(KERN_NOTICE "random: %pF get_random_bytes called "
 		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
@@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
 	    return ret;
 #endif
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u64 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u64);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
@@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
 	if (arch_get_random_int(&ret))
 		return ret;
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+	if (!crng_ready())
+		printk(KERN_NOTICE "random: %pF get_random_u32 called "
+		       "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
 	batch = &get_cpu_var(batched_entropy_u32);
 	if (use_lock)
 		read_lock_irqsave(&batched_entropy_reset_lock, flags);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e4587ebe52c7..c4159605bfbf 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,6 +1209,22 @@ config STACKTRACE
 	  It is also used by various kernel debugging features that require
 	  stack trace generation.
 
+config WARN_UNSEEDED_RANDOM
+	bool "Warn when kernel uses unseeded randomness"
+	default y
+	depends on DEBUG_KERNEL
+	help
+	  Some parts of the kernel contain bugs relating to their use of
+	  cryptographically secure random numbers before it's actually possible
+	  to generate those numbers securely. This setting ensures that these
+	  flaws don't go unnoticed, by enabling a message, should this ever
+	  occur. This will allow people with obscure setups to know when things
+	  are going wrong, so that they might contact developers about fixing
+	  it.
+
+	  Say Y here, unless you simply do not care about using unseeded
+	  randomness and do not want a potential warning message in your logs.
+
 config DEBUG_KOBJECT
 	bool "kobject debugging"
 	depends on DEBUG_KERNEL
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660411 — [PATCH v5 06/13] iscsi: ensure RNG is seeded before use

It's not safe to use weak random data here, especially for the challenge
response randomness. Since we're always in process context, it's safe to
simply wait until we have enough randomness to carry out the
authentication correctly.

While we're at it, we clean up a small memleak during an error
condition.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org>
Cc: Lee Duncan <lduncan@suse.com>
Cc: Chris Leech <cleech@redhat.com>
---
 drivers/target/iscsi/iscsi_target_auth.c  | 14 +++++++++++---
 drivers/target/iscsi/iscsi_target_login.c | 22 ++++++++++++++--------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
index 903b667f8e01..f9bc8ec6fb6b 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -47,18 +47,21 @@ static void chap_binaryhex_to_asciihex(char *dst, char *src, int src_len)
 	}
 }
 
-static void chap_gen_challenge(
+static int chap_gen_challenge(
 	struct iscsi_conn *conn,
 	int caller,
 	char *c_str,
 	unsigned int *c_len)
 {
+	int ret;
 	unsigned char challenge_asciihex[CHAP_CHALLENGE_LENGTH * 2 + 1];
 	struct iscsi_chap *chap = conn->auth_protocol;
 
 	memset(challenge_asciihex, 0, CHAP_CHALLENGE_LENGTH * 2 + 1);
 
-	get_random_bytes(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	ret = get_random_bytes_wait(chap->challenge, CHAP_CHALLENGE_LENGTH);
+	if (unlikely(ret))
+		return ret;
 	chap_binaryhex_to_asciihex(challenge_asciihex, chap->challenge,
 				CHAP_CHALLENGE_LENGTH);
 	/*
@@ -69,6 +72,7 @@ static void chap_gen_challenge(
 
 	pr_debug("[%s] Sending CHAP_C=0x%s\n\n", (caller) ? "server" : "client",
 			challenge_asciihex);
+	return 0;
 }
 
 static int chap_check_algorithm(const char *a_str)
@@ -143,6 +147,7 @@ static struct iscsi_chap *chap_server_open(
 	case CHAP_DIGEST_UNKNOWN:
 	default:
 		pr_err("Unsupported CHAP_A value\n");
+		kfree(conn->auth_protocol);
 		return NULL;
 	}
 
@@ -156,7 +161,10 @@ static struct iscsi_chap *chap_server_open(
 	/*
 	 * Generate Challenge.
 	 */
-	chap_gen_challenge(conn, 1, aic_str, aic_len);
+	if (chap_gen_challenge(conn, 1, aic_str, aic_len) < 0) {
+		kfree(conn->auth_protocol);
+		return NULL;
+	}
 
 	return chap;
 }
diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c
index 92b96b51d506..e9bdc8b86e7d 100644
--- a/drivers/target/iscsi/iscsi_target_login.c
+++ b/drivers/target/iscsi/iscsi_target_login.c
@@ -245,22 +245,26 @@ int iscsi_check_for_session_reinstatement(struct iscsi_conn *conn)
 	return 0;
 }
 
-static void iscsi_login_set_conn_values(
+static int iscsi_login_set_conn_values(
 	struct iscsi_session *sess,
 	struct iscsi_conn *conn,
 	__be16 cid)
 {
+	int ret;
 	conn->sess		= sess;
 	conn->cid		= be16_to_cpu(cid);
 	/*
 	 * Generate a random Status sequence number (statsn) for the new
 	 * iSCSI connection.
 	 */
-	get_random_bytes(&conn->stat_sn, sizeof(u32));
+	ret = get_random_bytes_wait(&conn->stat_sn, sizeof(u32));
+	if (unlikely(ret))
+		return ret;
 
 	mutex_lock(&auth_id_lock);
 	conn->auth_id		= iscsit_global->auth_id++;
 	mutex_unlock(&auth_id_lock);
+	return 0;
 }
 
 __printf(2, 3) int iscsi_change_param_sprintf(
@@ -306,7 +310,11 @@ static int iscsi_login_zero_tsih_s1(
 		return -ENOMEM;
 	}
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	ret = iscsi_login_set_conn_values(sess, conn, pdu->cid);
+	if (unlikely(ret)) {
+		kfree(sess);
+		return ret;
+	}
 	sess->init_task_tag	= pdu->itt;
 	memcpy(&sess->isid, pdu->isid, 6);
 	sess->exp_cmd_sn	= be32_to_cpu(pdu->cmdsn);
@@ -497,8 +505,7 @@ static int iscsi_login_non_zero_tsih_s1(
 {
 	struct iscsi_login_req *pdu = (struct iscsi_login_req *)buf;
 
-	iscsi_login_set_conn_values(NULL, conn, pdu->cid);
-	return 0;
+	return iscsi_login_set_conn_values(NULL, conn, pdu->cid);
 }
 
 /*
@@ -554,9 +561,8 @@ static int iscsi_login_non_zero_tsih_s2(
 		atomic_set(&sess->session_continuation, 1);
 	spin_unlock_bh(&sess->conn_lock);
 
-	iscsi_login_set_conn_values(sess, conn, pdu->cid);
-
-	if (iscsi_copy_param_list(&conn->param_list,
+	if (iscsi_login_set_conn_values(sess, conn, pdu->cid) < 0 ||
+	    iscsi_copy_param_list(&conn->param_list,
 			conn->tpg->param_list, 0) < 0) {
 		iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR,
 				ISCSI_LOGIN_STATUS_NO_RESOURCES);
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660412 — [PATCH v5 05/13] crypto/rng: ensure that the RNG is ready before using

Otherwise, we might be seeding the RNG using bad randomness, which is
dangerous. The one use of this function from within the kernel -- not
from userspace -- is being removed (keys/big_key), so that call site
isn't relevant in assessing this.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 crypto/rng.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/crypto/rng.c b/crypto/rng.c
index f46dac5288b9..e042437e64b4 100644
--- a/crypto/rng.c
+++ b/crypto/rng.c
@@ -48,12 +48,14 @@ int crypto_rng_reset(struct crypto_rng *tfm, const u8 *seed, unsigned int slen)
 		if (!buf)
 			return -ENOMEM;
 
-		get_random_bytes(buf, slen);
+		err = get_random_bytes_wait(buf, slen);
+		if (err)
+			goto out;
 		seed = buf;
 	}
 
 	err = crypto_rng_alg(tfm)->seed(tfm, seed, slen);
-
+out:
 	kzfree(buf);
 	return err;
 }
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660415 — [PATCH v5 11/13] net/route: use get_random_int for random counter

Using get_random_int here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is when many of these structures
are assigned.

Also, semantically, it's not really proper to have been assigning an
atomic_t in this way before, even if in practice it works fine.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Miller <davem@davemloft.net>
---
 net/ipv4/route.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6883b3d4ba8f..32a3332ec9cf 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2944,8 +2944,7 @@ static __net_init int rt_genid_init(struct net *net)
 {
 	atomic_set(&net->ipv4.rt_genid, 0);
 	atomic_set(&net->fnhe_genid, 0);
-	get_random_bytes(&net->ipv4.dev_addr_genid,
-			 sizeof(net->ipv4.dev_addr_genid));
+	atomic_set(&net->ipv4.dev_addr_genid, get_random_int());
 	return 0;
 }
 
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660418 — [PATCH v5 10/13] net/neighbor: use get_random_u32 for 32-bit hash random

Using get_random_u32 here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is when many of these structures
are assigned.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Miller <davem@davemloft.net>
---
 net/core/neighbour.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index d274f81fcc2c..9784133b0cdb 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -312,8 +312,7 @@ static struct neighbour *neigh_alloc(struct neigh_table *tbl, struct net_device
 
 static void neigh_get_hash_rnd(u32 *x)
 {
-	get_random_bytes(x, sizeof(*x));
-	*x |= 1;
+	*x = get_random_u32() | 1;
 }
 
 static struct neigh_hash_table *neigh_hash_alloc(unsigned int shift)
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660424 — [PATCH v5 08/13] cifs: use get_random_u32 for 32-bit lock random

Using get_random_u32 here is faster, more fitting of the use case, and
just as cryptographically secure. It also has the benefit of providing
better randomness at early boot, which is sometimes when this is used.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Steve French <sfrench@samba.org>
---
 fs/cifs/cifsfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 9a1667e0e8d6..fe0c8dcc7dc7 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1359,7 +1359,7 @@ init_cifs(void)
 	spin_lock_init(&cifs_tcp_ses_lock);
 	spin_lock_init(&GlobalMid_Lock);
 
-	get_random_bytes(&cifs_lock_secret, sizeof(cifs_lock_secret));
+	cifs_lock_secret = get_random_u32();
 
 	if (cifs_max_pending < 2) {
 		cifs_max_pending = 2;
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660426 — [PATCH v5 12/13] bluetooth/smp: ensure RNG is properly seeded before ECDH use

This protocol uses lots of complex cryptography that relies on securely
generated random numbers. Thus, it's important that the RNG is actually
seeded before use. Fortuantely, it appears we're always operating in
process context (there are many GFP_KERNEL allocations and other
sleeping operations), and so we can simply demand that the RNG is seeded
before we use it.

We take two strategies in this commit. The first is for the library code
that's called from other modules like hci or mgmt: here we just change
the call to get_random_bytes_wait, and return the result of the wait to
the caller, along with the other error codes of those functions like
usual. Then there's the SMP protocol handler itself, which makes many
many many calls to get_random_bytes during different phases. For this,
rather than have to change all the calls to get_random_bytes_wait and
propagate the error result, it's actually enough to just put a single
call to wait_for_random_bytes() at the beginning of the handler, to
ensure that all the subsequent invocations are safe, without having to
actually change them. Likewise, for the random address changing
function, we'd rather know early on in the function whether the RNG
initialization has been interrupted, rather than later, so we call
wait_for_random_bytes() at the top, so that later on the call to
get_random_bytes() is acceptable.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
---
 net/bluetooth/hci_request.c |  6 ++++++
 net/bluetooth/smp.c         | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index b5faff458d8b..4078057c4fd7 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1406,6 +1406,12 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
 	struct hci_dev *hdev = req->hdev;
 	int err;
 
+	if (require_privacy) {
+		err = wait_for_random_bytes();
+		if (unlikely(err))
+			return err;
+	}
+
 	/* If privacy is enabled use a resolvable private address. If
 	 * current RPA has expired or there is something else than
 	 * the current RPA in use, then generate a new one.
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 14585edc9439..5fef1bc96f42 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -537,7 +537,9 @@ int smp_generate_rpa(struct hci_dev *hdev, const u8 irk[16], bdaddr_t *rpa)
 
 	smp = chan->data;
 
-	get_random_bytes(&rpa->b[3], 3);
+	err = get_random_bytes_wait(&rpa->b[3], 3);
+	if (unlikely(err))
+		return err;
 
 	rpa->b[5] &= 0x3f;	/* Clear two most significant bits */
 	rpa->b[5] |= 0x40;	/* Set second most significant bit */
@@ -570,7 +572,9 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
 	} else {
 		while (true) {
 			/* Seed private key with random number */
-			get_random_bytes(smp->local_sk, 32);
+			err = get_random_bytes_wait(smp->local_sk, 32);
+			if (unlikely(err))
+				return err;
 
 			/* Generate local key pair for Secure Connections */
 			if (!generate_ecdh_keys(smp->local_pk, smp->local_sk))
@@ -589,7 +593,9 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
 	SMP_DBG("OOB Public Key Y: %32phN", smp->local_pk + 32);
 	SMP_DBG("OOB Private Key:  %32phN", smp->local_sk);
 
-	get_random_bytes(smp->local_rand, 16);
+	err = get_random_bytes_wait(smp->local_rand, 16);
+	if (unlikely(err))
+		return err;
 
 	err = smp_f4(smp->tfm_cmac, smp->local_pk, smp->local_pk,
 		     smp->local_rand, 0, hash);
@@ -2831,7 +2837,11 @@ static int smp_sig_channel(struct l2cap_chan *chan, struct sk_buff *skb)
 	struct hci_conn *hcon = conn->hcon;
 	struct smp_chan *smp;
 	__u8 code, reason;
-	int err = 0;
+	int err;
+
+	err = wait_for_random_bytes();
+	if (unlikely(err))
+		return err;
 
 	if (skb->len < 1)
 		return -EILSEQ;
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660431 — [PATCH v5 02/13] random: add synchronous API for the urandom pool

This enables users of get_random_{bytes,u32,u64,int,long} to wait until
the pool is ready before using this function, in case they actually want
to have reliable randomness.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 41 +++++++++++++++++++++++++++++++----------
 include/linux/random.h |  1 +
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index d35da1603e12..77587ac1ecb2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -851,11 +851,6 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	spin_unlock_irqrestore(&primary_crng.lock, flags);
 }
 
-static inline void crng_wait_ready(void)
-{
-	wait_event_interruptible(crng_init_wait, crng_ready());
-}
-
 static void _extract_crng(struct crng_state *crng,
 			  __u8 out[CHACHA20_BLOCK_SIZE])
 {
@@ -1477,7 +1472,10 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
  * number of good random numbers, suitable for key generation, seeding
  * TCP sequence numbers, etc.  It does not rely on the hardware random
  * number generator.  For random bytes direct from the hardware RNG
- * (when available), use get_random_bytes_arch().
+ * (when available), use get_random_bytes_arch(). In order to ensure
+ * that the randomness provided by this function is okay, the function
+ * wait_for_random_bytes() should be called and return 0 at least once
+ * at any point prior.
  */
 void get_random_bytes(void *buf, int nbytes)
 {
@@ -1507,6 +1505,24 @@ void get_random_bytes(void *buf, int nbytes)
 EXPORT_SYMBOL(get_random_bytes);
 
 /*
+ * Wait for the urandom pool to be seeded and thus guaranteed to supply
+ * cryptographically secure random numbers. This applies to: the /dev/urandom
+ * device, the get_random_bytes function, and the get_random_{u32,u64,int,long}
+ * family of functions. Using any of these functions without first calling
+ * this function forfeits the guarantee of security.
+ *
+ * Returns: 0 if the urandom pool has been seeded.
+ *          -ERESTARTSYS if the function was interrupted by a signal.
+ */
+int wait_for_random_bytes(void)
+{
+	if (likely(crng_ready()))
+		return 0;
+	return wait_event_interruptible(crng_init_wait, crng_ready());
+}
+EXPORT_SYMBOL(wait_for_random_bytes);
+
+/*
  * Add a callback function that will be invoked when the nonblocking
  * pool is initialised.
  *
@@ -1860,6 +1876,8 @@ const struct file_operations urandom_fops = {
 SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
 		unsigned int, flags)
 {
+	int ret;
+
 	if (flags & ~(GRND_NONBLOCK|GRND_RANDOM))
 		return -EINVAL;
 
@@ -1872,9 +1890,9 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
 	if (!crng_ready()) {
 		if (flags & GRND_NONBLOCK)
 			return -EAGAIN;
-		crng_wait_ready();
-		if (signal_pending(current))
-			return -ERESTARTSYS;
+		ret = wait_for_random_bytes();
+		if (unlikely(ret))
+			return ret;
 	}
 	return urandom_read(NULL, buf, count, NULL);
 }
@@ -2035,7 +2053,10 @@ static rwlock_t batched_entropy_reset_lock = __RW_LOCK_UNLOCKED(batched_entropy_
 /*
  * Get a random word for internal kernel use only. The quality of the random
  * number is either as good as RDRAND or as good as /dev/urandom, with the
- * goal of being quite fast and not depleting entropy.
+ * goal of being quite fast and not depleting entropy. In order to ensure
+ * that the randomness provided by this function is okay, the function
+ * wait_for_random_bytes() should be called and return 0 at least once
+ * at any point prior.
  */
 static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
 u64 get_random_u64(void)
diff --git a/include/linux/random.h b/include/linux/random.h
index ed5c3838780d..e29929347c95 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,6 +34,7 @@ extern void add_input_randomness(unsigned int type, unsigned int code,
 extern void add_interrupt_randomness(int irq, int irq_flags) __latent_entropy;
 
 extern void get_random_bytes(void *buf, int nbytes);
+extern int wait_for_random_bytes(void);
 extern int add_random_ready_callback(struct random_ready_callback *rdy);
 extern void del_random_ready_callback(struct random_ready_callback *rdy);
 extern void get_random_bytes_arch(void *buf, int nbytes);
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1660433 — [PATCH v5 01/13] random: invalidate batched entropy after crng init

It's possible that get_random_{u32,u64} is used before the crng has
initialized, in which case, its output might not be cryptographically
secure. For this problem, directly, this patch set is introducing the
*_wait variety of functions, but even with that, there's a subtle issue:
what happens to our batched entropy that was generated before
initialization. Prior to this commit, it'd stick around, supplying bad
numbers. After this commit, we force the entropy to be re-extracted
after each phase of the crng has initialized.

In order to avoid a race condition with the position counter, we
introduce a simple rwlock for this invalidation. Since it's only during
this awkward transition period, after things are all set up, we stop
using it, so that it doesn't have an impact on performance.

This should probably be backported to 4.11.

(Also: adding my copyright to the top. With the patch series from
January, this patch, and then the ones that come after, I think there's
a relevant amount of code in here to add my name to the top.)

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/char/random.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index a561f0c2f428..d35da1603e12 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1,6 +1,9 @@
 /*
  * random.c -- A strong random number generator
  *
+ * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All
+ * Rights Reserved.
+ *
  * Copyright Matt Mackall <mpm@selenic.com>, 2003, 2004, 2005
  *
  * Copyright Theodore Ts'o, 1994, 1995, 1996, 1997, 1998, 1999.  All
@@ -762,6 +765,8 @@ static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct crng_state **crng_node_pool __read_mostly;
 #endif
 
+static void invalidate_batched_entropy(void);
+
 static void crng_initialize(struct crng_state *crng)
 {
 	int		i;
@@ -799,6 +804,7 @@ static int crng_fast_load(const char *cp, size_t len)
 		cp++; crng_init_cnt++; len--;
 	}
 	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
+		invalidate_batched_entropy();
 		crng_init = 1;
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: fast init done\n");
@@ -836,6 +842,7 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	memzero_explicit(&buf, sizeof(buf));
 	crng->init_time = jiffies;
 	if (crng == &primary_crng && crng_init < 2) {
+		invalidate_batched_entropy();
 		crng_init = 2;
 		process_random_ready_list();
 		wake_up_interruptible(&crng_init_wait);
@@ -2023,6 +2030,7 @@ struct batched_entropy {
 	};
 	unsigned int position;
 };
+static rwlock_t batched_entropy_reset_lock = __RW_LOCK_UNLOCKED(batched_entropy_reset_lock);
 
 /*
  * Get a random word for internal kernel use only. The quality of the random
@@ -2033,6 +2041,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
 u64 get_random_u64(void)
 {
 	u64 ret;
+	const bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 
 #if BITS_PER_LONG == 64
@@ -2045,11 +2055,15 @@ u64 get_random_u64(void)
 #endif
 
 	batch = &get_cpu_var(batched_entropy_u64);
+	if (use_lock)
+		read_lock_irqsave(&batched_entropy_reset_lock, flags);
 	if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
 		extract_crng((u8 *)batch->entropy_u64);
 		batch->position = 0;
 	}
 	ret = batch->entropy_u64[batch->position++];
+	if (use_lock)
+		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
 	put_cpu_var(batched_entropy_u64);
 	return ret;
 }
@@ -2059,22 +2073,45 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
 u32 get_random_u32(void)
 {
 	u32 ret;
+	const bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 
 	if (arch_get_random_int(&ret))
 		return ret;
 
 	batch = &get_cpu_var(batched_entropy_u32);
+	if (use_lock)
+		read_lock_irqsave(&batched_entropy_reset_lock, flags);
 	if (batch->position % ARRAY_SIZE(batch->entropy_u32) == 0) {
 		extract_crng((u8 *)batch->entropy_u32);
 		batch->position = 0;
 	}
 	ret = batch->entropy_u32[batch->position++];
+	if (use_lock)
+		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
 	put_cpu_var(batched_entropy_u32);
 	return ret;
 }
 EXPORT_SYMBOL(get_random_u32);
 
+/* It's important to invalidate all potential batched entropy that might
+ * be stored before the crng is initialized, which we can do lazily by
+ * simply resetting the counter to zero so that it's re-extracted on the
+ * next usage. */
+static void invalidate_batched_entropy(void)
+{
+	int cpu;
+	unsigned long flags;
+
+	write_lock_irqsave(&batched_entropy_reset_lock, flags);
+	for_each_possible_cpu (cpu) {
+		per_cpu_ptr(&batched_entropy_u32, cpu)->position = 0;
+		per_cpu_ptr(&batched_entropy_u64, cpu)->position = 0;
+	}
+	write_unlock_irqrestore(&batched_entropy_reset_lock, flags);
+}
+
 /**
  * randomize_page - Generate a random, page aligned address
  * @start:	The smallest acceptable address the caller will take.
-- 
2.13.0

[toc] | [prev] | [next] | [standalone]

#1666168 — Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init

On 2017-06-08 01:25:55 [+0200], Jason A. Donenfeld wrote:
> It's possible that get_random_{u32,u64} is used before the crng has
> initialized, in which case, its output might not be cryptographically
> secure. For this problem, directly, this patch set is introducing the
> *_wait variety of functions, but even with that, there's a subtle issue:
> what happens to our batched entropy that was generated before
> initialization. Prior to this commit, it'd stick around, supplying bad
> numbers. After this commit, we force the entropy to be re-extracted
> after each phase of the crng has initialized.
> 
> In order to avoid a race condition with the position counter, we
> introduce a simple rwlock for this invalidation. Since it's only during
> this awkward transition period, after things are all set up, we stop
> using it, so that it doesn't have an impact on performance.
> 
> This should probably be backported to 4.11.
> 
> (Also: adding my copyright to the top. With the patch series from
> January, this patch, and then the ones that come after, I think there's
> a relevant amount of code in here to add my name to the top.)
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

I don't understand why lockdep notices this possible deadlock only in
RT:

| the existing dependency chain (in reverse order) is:
|
| -> #1 (primary_crng.lock){+.+...}:
|        lock_acquire+0xb5/0x2b0
|        rt_spin_lock+0x46/0x50
|        _extract_crng+0x39/0xa0
|        extract_crng+0x3a/0x40
|        get_random_u64+0x17a/0x200
|        cache_random_seq_create+0x51/0x100
|        init_cache_random_seq+0x35/0x90
|        __kmem_cache_create+0xd3/0x560
|        create_boot_cache+0x8c/0xb2
|        create_kmalloc_cache+0x54/0x9f
|        create_kmalloc_caches+0xe3/0xfd
|        kmem_cache_init+0x14f/0x1f0
|        start_kernel+0x1e7/0x3b3
|        x86_64_start_reservations+0x2a/0x2c
|        x86_64_start_kernel+0x13d/0x14c
|        verify_cpu+0x0/0xfc
|
| -> #0 (batched_entropy_reset_lock){+.+...}:
|        __lock_acquire+0x11b4/0x1320
|        lock_acquire+0xb5/0x2b0
|        rt_write_lock+0x26/0x40
|        rt_write_lock_irqsave+0x9/0x10
|        invalidate_batched_entropy+0x28/0xb0
|        crng_fast_load+0xb5/0xe0
|        add_interrupt_randomness+0x16c/0x1a0
|        irq_thread+0x15c/0x1e0
|        kthread+0x112/0x150
|        ret_from_fork+0x31/0x40

We have	the path add_interrupt_randomness() -> crng_fast_load() which
take
  primary_crng.lock -> batched_entropy_reset_lock
and we have get_random_uXX() -> extract_crng() which take the locks in
opposite order:
  batched_entropy_reset_lock -> crng->lock

however batched_entropy_reset_lock() is only taken if "crng_init < 2" so
it is possible RT hits this constraint more reliably.

> ---
>  drivers/char/random.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index a561f0c2f428..d35da1603e12 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1,6 +1,9 @@
>  /*
>   * random.c -- A strong random number generator
>   *
> + * Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All
> + * Rights Reserved.
> + *
>   * Copyright Matt Mackall <mpm@selenic.com>, 2003, 2004, 2005
>   *
>   * Copyright Theodore Ts'o, 1994, 1995, 1996, 1997, 1998, 1999.  All
> @@ -762,6 +765,8 @@ static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
>  static struct crng_state **crng_node_pool __read_mostly;
>  #endif
>  
> +static void invalidate_batched_entropy(void);
> +
>  static void crng_initialize(struct crng_state *crng)
>  {
>  	int		i;
> @@ -799,6 +804,7 @@ static int crng_fast_load(const char *cp, size_t len)
>  		cp++; crng_init_cnt++; len--;
>  	}
>  	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
> +		invalidate_batched_entropy();
>  		crng_init = 1;
>  		wake_up_interruptible(&crng_init_wait);
>  		pr_notice("random: fast init done\n");
> @@ -836,6 +842,7 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
>  	memzero_explicit(&buf, sizeof(buf));
>  	crng->init_time = jiffies;
>  	if (crng == &primary_crng && crng_init < 2) {
> +		invalidate_batched_entropy();
>  		crng_init = 2;
>  		process_random_ready_list();
>  		wake_up_interruptible(&crng_init_wait);
> @@ -2023,6 +2030,7 @@ struct batched_entropy {
>  	};
>  	unsigned int position;
>  };
> +static rwlock_t batched_entropy_reset_lock = __RW_LOCK_UNLOCKED(batched_entropy_reset_lock);
>  
>  /*
>   * Get a random word for internal kernel use only. The quality of the random
> @@ -2033,6 +2041,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
>  u64 get_random_u64(void)
>  {
>  	u64 ret;
> +	const bool use_lock = READ_ONCE(crng_init) < 2;
> +	unsigned long flags = 0;
>  	struct batched_entropy *batch;
>  
>  #if BITS_PER_LONG == 64
> @@ -2045,11 +2055,15 @@ u64 get_random_u64(void)
>  #endif
>  
>  	batch = &get_cpu_var(batched_entropy_u64);
> +	if (use_lock)
> +		read_lock_irqsave(&batched_entropy_reset_lock, flags);
>  	if (batch->position % ARRAY_SIZE(batch->entropy_u64) == 0) {
>  		extract_crng((u8 *)batch->entropy_u64);
>  		batch->position = 0;
>  	}
>  	ret = batch->entropy_u64[batch->position++];
> +	if (use_lock)
> +		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
>  	put_cpu_var(batched_entropy_u64);
>  	return ret;
>  }
> @@ -2059,22 +2073,45 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
>  u32 get_random_u32(void)
>  {
>  	u32 ret;
> +	const bool use_lock = READ_ONCE(crng_init) < 2;
> +	unsigned long flags = 0;
>  	struct batched_entropy *batch;
>  
>  	if (arch_get_random_int(&ret))
>  		return ret;
>  
>  	batch = &get_cpu_var(batched_entropy_u32);
> +	if (use_lock)
> +		read_lock_irqsave(&batched_entropy_reset_lock, flags);
>  	if (batch->position % ARRAY_SIZE(batch->entropy_u32) == 0) {
>  		extract_crng((u8 *)batch->entropy_u32);
>  		batch->position = 0;
>  	}
>  	ret = batch->entropy_u32[batch->position++];
> +	if (use_lock)
> +		read_unlock_irqrestore(&batched_entropy_reset_lock, flags);
>  	put_cpu_var(batched_entropy_u32);
>  	return ret;
>  }
>  EXPORT_SYMBOL(get_random_u32);
>  
> +/* It's important to invalidate all potential batched entropy that might
> + * be stored before the crng is initialized, which we can do lazily by
> + * simply resetting the counter to zero so that it's re-extracted on the
> + * next usage. */
> +static void invalidate_batched_entropy(void)
> +{
> +	int cpu;
> +	unsigned long flags;
> +
> +	write_lock_irqsave(&batched_entropy_reset_lock, flags);
> +	for_each_possible_cpu (cpu) {
> +		per_cpu_ptr(&batched_entropy_u32, cpu)->position = 0;
> +		per_cpu_ptr(&batched_entropy_u64, cpu)->position = 0;
> +	}
> +	write_unlock_irqrestore(&batched_entropy_reset_lock, flags);
> +}
> +
>  /**
>   * randomize_page - Generate a random, page aligned address
>   * @start:	The smallest acceptable address the caller will take.
> -- 
> 2.13.0

Sebastian

[toc] | [prev] | [next] | [standalone]

#1666295 — Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init

There's a potential race that I fixed in my v5 of that patch set, but
Ted only took v4, and for whatever reason has been to busy to submit
the additional patch I already posted showing the diff between v4&v5.
Hopefully he actually gets around to it and sends this for the next
rc. Here it is:

https://patchwork.kernel.org/patch/9774563/

[toc] | [prev] | [next] | [standalone]

#1667527 — Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init

On 2017-06-15 00:33:12 [+0200], Jason A. Donenfeld wrote:
> There's a potential race that I fixed in my v5 of that patch set, but
> Ted only took v4, and for whatever reason has been to busy to submit
> the additional patch I already posted showing the diff between v4&v5.
> Hopefully he actually gets around to it and sends this for the next
> rc. Here it is:
> 
> https://patchwork.kernel.org/patch/9774563/

So you replace "crng_init < 2" with use_lock instead. That is not what I
am talking about. Again:
	add_interrupt_randomness()
->       crng_fast_load()	        spin_trylock_irqsave(&primary_crng.lock, )
->        invalidate_batched_entropy()  write_lock_irqsave(&batched_entropy_reset_lock, );

in that order while the code path
	get_random_uXX()                read_lock_irqsave(&batched_entropy_reset_lock, );
->       extract_crng()
->        _extract_crng()                spin_lock_irqsave(&crng->lock, );

which allocates the same lock in opposite order.
That means
  T1			T2
  crng_fast_load()	get_random_u64()
  			extract_crng()
              *dead lock*
invalidate_batched_entropy()
			_extract_crng()

So T1 waits for batched_entropy_reset_lock holding primary_crng.lock and
T2 waits for primary_crng.lock holding batched_entropy_reset_lock.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1667693 — Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init

On Fri, Jun 16, 2017 at 10:31 AM, Sebastian Andrzej Siewior
<sebastian@breakpoint.cc> wrote:
> am talking about. Again:

I actually figured that out myself after sending the initial email, so
then I wrote a follow-up patch which I attached to this thread. You
should have received it. Can you take a look?

https://lkml.org/lkml/2017/6/14/942

[toc] | [prev] | [next] | [standalone]

#1667806 — Re: [PATCH v5 01/13] random: invalidate batched entropy after crng init

On 2017-06-16 14:12:42 [+0200], Jason A. Donenfeld wrote:
> I actually figured that out myself after sending the initial email, so
> then I wrote a follow-up patch which I attached to this thread. You
> should have received it. Can you take a look?

replied to the patch.

Sebastian

[toc] | [prev] | [next] | [standalone]

#1666305 — [PATCH] random: silence compiler warnings and fix race

Odd versions of gcc for the sh4 architecture will actually warn about
flags being used while uninitialized, so we set them to zero. Non crazy
gccs will optimize that out again, so it doesn't make a difference.

Next, over aggressive gccs could inline the expression that defines
use_lock, which could then introduce a race resulting in a lock
imbalance. By using READ_ONCE, we prevent that fate. Finally, we make
that assignment const, so that gcc can still optimize a nice amount.

Finally, we fix a potential deadlock between primary_crng.lock and
batched_entropy_reset_lock, where they could be called in opposite
order. Moving the call to invalidate_batched_entropy to outside the lock
rectifies this issue.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Ted -- the first part of this is the fixup patch we discussed earlier.
Then I added on top a fix for a potentially related race.

I'm not totally convinced that moving this block to outside the spinlock
is 100% okay, so please give this a close look before merging.


 drivers/char/random.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index e870f329db88..01a260f67437 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -803,13 +803,13 @@ static int crng_fast_load(const char *cp, size_t len)
 		p[crng_init_cnt % CHACHA20_KEY_SIZE] ^= *cp;
 		cp++; crng_init_cnt++; len--;
 	}
+	spin_unlock_irqrestore(&primary_crng.lock, flags);
 	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
 		invalidate_batched_entropy();
 		crng_init = 1;
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: fast init done\n");
 	}
-	spin_unlock_irqrestore(&primary_crng.lock, flags);
 	return 1;
 }
 
@@ -841,6 +841,7 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	}
 	memzero_explicit(&buf, sizeof(buf));
 	crng->init_time = jiffies;
+	spin_unlock_irqrestore(&primary_crng.lock, flags);
 	if (crng == &primary_crng && crng_init < 2) {
 		invalidate_batched_entropy();
 		crng_init = 2;
@@ -848,7 +849,6 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: crng init done\n");
 	}
-	spin_unlock_irqrestore(&primary_crng.lock, flags);
 }
 
 static inline void crng_wait_ready(void)
@@ -2041,8 +2041,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
 u64 get_random_u64(void)
 {
 	u64 ret;
-	bool use_lock = crng_init < 2;
-	unsigned long flags;
+	bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 
 #if BITS_PER_LONG == 64
@@ -2073,8 +2073,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u32);
 u32 get_random_u32(void)
 {
 	u32 ret;
-	bool use_lock = crng_init < 2;
-	unsigned long flags;
+	bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 
 	if (arch_get_random_int(&ret))
-- 
2.13.1

[toc] | [prev] | [next] | [standalone]

#1667807 — Re: [PATCH] random: silence compiler warnings and fix race

On 2017-06-15 00:45:26 [+0200], Jason A. Donenfeld wrote:
> Odd versions of gcc for the sh4 architecture will actually warn about
> flags being used while uninitialized, so we set them to zero. Non crazy
> gccs will optimize that out again, so it doesn't make a difference.

that is minor

> Next, over aggressive gccs could inline the expression that defines
> use_lock, which could then introduce a race resulting in a lock
> imbalance. By using READ_ONCE, we prevent that fate. Finally, we make
> that assignment const, so that gcc can still optimize a nice amount.

Not sure about that, more below.

> Finally, we fix a potential deadlock between primary_crng.lock and
> batched_entropy_reset_lock, where they could be called in opposite
> order. Moving the call to invalidate_batched_entropy to outside the lock
> rectifies this issue.

and *that* is separate issue which has to pulled in for stable once it
has been addressed in Linus' tree.

> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Ted -- the first part of this is the fixup patch we discussed earlier.
> Then I added on top a fix for a potentially related race.
> 
> I'm not totally convinced that moving this block to outside the spinlock
> is 100% okay, so please give this a close look before merging.
> 
> 
>  drivers/char/random.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index e870f329db88..01a260f67437 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -803,13 +803,13 @@ static int crng_fast_load(const char *cp, size_t len)
>  		p[crng_init_cnt % CHACHA20_KEY_SIZE] ^= *cp;
>  		cp++; crng_init_cnt++; len--;
>  	}
> +	spin_unlock_irqrestore(&primary_crng.lock, flags);
>  	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
>  		invalidate_batched_entropy();
>  		crng_init = 1;
>  		wake_up_interruptible(&crng_init_wait);
>  		pr_notice("random: fast init done\n");
>  	}
> -	spin_unlock_irqrestore(&primary_crng.lock, flags);
>  	return 1;

I wouldn't just push the lock one up as is but move that write part to
crng_init to remain within the locked section. Like that:

diff --git a/drivers/char/random.c b/drivers/char/random.c
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -804,12 +804,14 @@ static int crng_fast_load(const char *cp, size_t len)
 		cp++; crng_init_cnt++; len--;
 	}
 	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
-		invalidate_batched_entropy();
 		crng_init = 1;
+		spin_unlock_irqrestore(&primary_crng.lock, flags);
+		invalidate_batched_entropy();
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: fast init done\n");
+	} else {
+		spin_unlock_irqrestore(&primary_crng.lock, flags);
 	}
-	spin_unlock_irqrestore(&primary_crng.lock, flags);
 	return 1;
 }
 
@@ -842,13 +844,16 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
 	memzero_explicit(&buf, sizeof(buf));
 	crng->init_time = jiffies;
 	if (crng == &primary_crng && crng_init < 2) {
-		invalidate_batched_entropy();
 		crng_init = 2;
+		spin_unlock_irqrestore(&primary_crng.lock, flags);
+
+		invalidate_batched_entropy();
 		process_random_ready_list();
 		wake_up_interruptible(&crng_init_wait);
 		pr_notice("random: crng init done\n");
+	} else {
+		spin_unlock_irqrestore(&primary_crng.lock, flags);
 	}
-	spin_unlock_irqrestore(&primary_crng.lock, flags);
 }
 
 static inline void crng_wait_ready(void)

>  }
>  
> @@ -2041,8 +2041,8 @@ static DEFINE_PER_CPU(struct batched_entropy, batched_entropy_u64);
>  u64 get_random_u64(void)
>  {
>  	u64 ret;
> -	bool use_lock = crng_init < 2;
> -	unsigned long flags;
> +	bool use_lock = READ_ONCE(crng_init) < 2;

Are use about that? I am not sure that the gcc will inline "crng_init"
read twice. It is not a local variable. READ_ONCE() is usually used
where gcc could cache a memory access but you do not want this. But hey!
If someone knows better I am here to learn.
One thing that this change does for sure is that crng_init is read very
early in the function while without READ_ONCE it is delayed _after_
arch_get_random_XXX(). So if arch_get_random_XXX() is around and works
you have one read for nothing.

> +	unsigned long flags = 0;
>  	struct batched_entropy *batch;
>  
>  #if BITS_PER_LONG == 64

Sebastian

[toc] | [prev] | [next] | [standalone]

#1668105 — Re: [PATCH] random: silence compiler warnings and fix race

On Fri, Jun 16, 2017 at 4:35 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> I wouldn't just push the lock one up as is but move that write part to
> crng_init to remain within the locked section. Like that:

We can't quite do that, because invalidate_batched_entropy() needs to
be called _before_ crng_init. Otherwise a concurrent call to
get_random_u32/u64() will have crng_init being the wrong value when
the batched entropy is still old.

> Are use about that? I am not sure that the gcc will inline "crng_init"
> read twice. It is not a local variable. READ_ONCE() is usually used
> where gcc could cache a memory access but you do not want this. But hey!
> If someone knows better I am here to learn.

The whole purpose is that I _want_ it to cache the memory access so
that it is _not_ inlined. So, based on your understanding, it does
exactly what I intended it to do. The reason is that I'd like to avoid
a lock imbalance, which could happen if the read is inlined.

Jason

[toc] | [prev] | [next] | [standalone]

#1668869 — Re: [PATCH] random: silence compiler warnings and fix race

On 2017-06-17 02:39:40 [+0200], Jason A. Donenfeld wrote:
> On Fri, Jun 16, 2017 at 4:35 PM, Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> > I wouldn't just push the lock one up as is but move that write part to
> > crng_init to remain within the locked section. Like that:
> 
> We can't quite do that, because invalidate_batched_entropy() needs to
> be called _before_ crng_init. Otherwise a concurrent call to
> get_random_u32/u64() will have crng_init being the wrong value when
> the batched entropy is still old.

ehm. You sure? I simply delayed the lock-dropping _after_ the state
variable was been modified. So it was basically what your patch did
except it was unlocked later…

> 
> > Are use about that? I am not sure that the gcc will inline "crng_init"
> > read twice. It is not a local variable. READ_ONCE() is usually used
> > where gcc could cache a memory access but you do not want this. But hey!
> > If someone knows better I am here to learn.
> 
> The whole purpose is that I _want_ it to cache the memory access so
> that it is _not_ inlined. So, based on your understanding, it does
> exactly what I intended it to do. The reason is that I'd like to avoid
> a lock imbalance, which could happen if the read is inlined.

So it was good as it was which means you can drop that READ_ONCE().

> Jason

Sebastian

[toc] | [prev] | [next] | [standalone]

#1669945 — Re: [PATCH] random: silence compiler warnings and fix race

On Mon, Jun 19, 2017 at 9:45 AM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> ehm. You sure? I simply delayed the lock-dropping _after_ the state
> variable was been modified. So it was basically what your patch did
> except it was unlocked later…

Yes, I'm sure. You moved the call to invalidate_batched_entropy() to
be after the assignment of crng_init. However, the call to
invalidate_batched_entropy() must be made _before_ the assignment of
crng_init.

>> > Are use about that? I am not sure that the gcc will inline "crng_init"
>> > read twice. It is not a local variable. READ_ONCE() is usually used
>> > where gcc could cache a memory access but you do not want this. But hey!
>> > If someone knows better I am here to learn.
>>
>> The whole purpose is that I _want_ it to cache the memory access so
>> that it is _not_ inlined. So, based on your understanding, it does
>> exactly what I intended it to do. The reason is that I'd like to avoid
>> a lock imbalance, which could happen if the read is inlined.
>
> So it was good as it was which means you can drop that READ_ONCE().

Except READ_ONCE ensures that the compiler will never inline it, so it
actually needs to stay.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web