Cleanups & Resubmissions for Random-dev

Hi Ted,

I'm not super sure what your workflow is like, but yesterday you picked up my v4 instead of my v5, and as you applied the wrong commits, I sent you several emails notifying you, along with quick fixes to various issues, which you also did not see. I assume you have complicated email filters, or maybe something like offlineimap where you grab snapshots of email, and thus the only way to make an impact on an in-motion workflow is for me to just formally resubmit things. So, here's a summary of what's in here:

security/keys: ensure RNG is seeded before use

You had noticed an issue with this in v4/v5, so now this is cleaned up the way you suggested so that you can merge it.

random: squelch sh compiler warning and ensure safe optimization

This was the biggie -- I had incorporated this fix into v5, but you picked up v4, so this commit is an additional one to bring your tree up to date, without having to rebase or rewrite history.

bluetooth/smp: ensure RNG is properly seeded before ECDH use

This one is pending your approval from the bluetooth people.

crypto/rng: ensure that the RNG is ready before using

This one I assume you didn't pick up because it depends on my big_keys rewrite going through.

Sorry for the procedural back-and-forth. If there's a way to do this flow better next time, do tell me what it is. Alternatively, if you're ever on IRC, realtime chat could be useful.

Regards,
Jason

 crypto/rng.c                             |  6 ++++--
 drivers/char/random.c                    |  8 ++++----
 net/bluetooth/hci_request.c              |  6 ++++++
 net/bluetooth/smp.c                      | 18 ++++++++++++++----
 security/keys/encrypted-keys/encrypted.c |  8 +++++---
 security/keys/key.c                      |  8 +++-----
 6 files changed, 36 insertions(+), 18 deletions(-)

2.13.0

Otherwise, we might use bad random numbers which, particularly in the case of IV generation, could be quite bad. It makes sense to use the synchronous API here, because we're always in process context (as the code is littered with GFP_KERNEL and the like). However, we can't change to using a blocking function in key serial allocation, because this will block booting in some configurations, so here we use the more appropriate get_random_u32, which will use RDRAND if available.

---
 security/keys/encrypted-keys/encrypted.c | 8 +++++---
 security/keys/key.c                      | 8 +++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

index 0010955d7876..d51a28fc5cd5 100644
@@ -777,10 +777,12 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
 
 	__ekey_init(epayload, format, master_desc, datalen);
 	if (!hex_encoded_iv) {
-		get_random_bytes(epayload->iv, ivsize);
+		ret = get_random_bytes_wait(epayload->iv, ivsize);
+		if (unlikely(ret))
+			return ret;
 
-		get_random_bytes(epayload->decrypted_data,
-				 epayload->decrypted_datalen);
+		ret = get_random_bytes_wait(epayload->decrypted_data,
+					    epayload->decrypted_datalen);
 	} else
 		ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
 	return ret;

index 455c04d80bbb..b1db72d0ab1b 100644
@@ -139,12 +139,10 @@ static inline void key_alloc_serial(struct key *key)
 	struct rb_node *parent, **p;
 	struct key *xkey;
 
-	/* propose a random serial number and look for a hole for it in the
-	 * serial number tree */
+	/* propose a non-negative random serial number and look for a hole for
+	 * it in the serial number tree */
 	do {
-		get_random_bytes(&key->serial, sizeof(key->serial));
-
-		key->serial >>= 1; /* negative numbers are not permitted */
+		key->serial = get_random_u32() >> 1;
 	} while (key->serial < 3);
 
 	spin_lock(&key_serial_lock);

2.13.0

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.

This patch was previously submitted as part of v5, but v4 was picked up, so this represents a delta between v4 and v5.

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

index 3bdeef13afda..cef9358ecf20 100644
@@ -2066,8 +2066,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;
+	const bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 #ifdef CONFIG_WARN_UNSEEDED_RANDOM
 	static void *previous = NULL;

@@ -2110,8 +2110,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;
+	const bool use_lock = READ_ONCE(crng_init) < 2;
+	unsigned long flags = 0;
 	struct batched_entropy *batch;
 #ifdef CONFIG_WARN_UNSEEDED_RANDOM
 	static void *previous = NULL;

2.13.0

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.

---
This is the same as v4/v5.

 net/bluetooth/hci_request.c |  6 ++++++
 net/bluetooth/smp.c         | 18 ++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

index b5faff458d8b..4078057c4fd7 100644
@@ -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.

index 14585edc9439..5fef1bc96f42 100644
@@ -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

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.

---
This is the same as v4/v5.

 crypto/rng.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index f46dac5288b9..e042437e64b4 100644
@@ -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

I use offlineimap to sync my Inbox to my laptop, and so while I was working on reviewing and applying your v4 patches, your v5 patches went out. These things happen. No big deal.

The random.git tree is one where no one else is depending on my tree, so I don't mind making rewinding the branch head to update patches. I don't do this with fcrypt.git, although I do this for the ext4.git tree. Since there are other file system trees that build on top of, or pull in fscrypt.git, I won't rewinding the branch since that causes them pain. But that's not real issue for ext4.git and random.git since it gets incorporated into linux-next (which is also rewound every day), but not any other subsystem tree. In the rare case where someone needs to do that, I'll use the "master" branch to denote that this is a branch that I won't rewind.

Thanks!

Or I can just grab the updated commit from v5, yes?

I think they said it would be enough to add wait_for_random_bytes() to the hci_power_on function

We probably should ask them to send us a patch that exposes the hardware RNG in the Bluetooth LE devices. Anytime we can get more entropy, I want to mix it into the entropy pools, even if we don't give any entropy credit.

Anything to make the government cryptographers work for a living. :-)

Yes; depending on when it goes through (e.g., if it's the next merge window) we can just also run it through the crypto tree. The main reason I'm running some of these changes through the random.git tree is because they have a dependency on the new interfaces that you've introduced. As we play whack-a-mole with all of the vairous get_random_byte users, we can either run them through their tree, or if the code in doesn't doesn't an active maintainer, or the maintainer prefers that we run them through random.git, we can run them through random.git tree.

These days I'm generally too busy to be on IRC. If there's something really urgent you can try hitting me up on Google Hangouts, or schedule a Google Video Chat if it really needs face-to-face communications.

I'm often travelling or time sharing between multiple projects, so do understand that sometimes my delay in responding is because I'm busy or because I'm deliberately waiting for other people to do review the patches before I do a detailed review.

- Ted

Hey Ted,

Yes, no problem, you can take the one from v5 and shuffle around the commits. This is probably cleanest, as this is the one that should be backported to 4.11, so it'll be more clear if it's in one commit rather than two.

Yea, they sort of did, but then they mentioned that not all devices would actually make use of it. I was thinking that:
a) that might result in waiting when it's not necessary;
b) hci_power_on() is a bit "far" from the actual use of get_random_bytes(), so it becomes very non-obvious and very particular that these are related. On the otherhand, these kind of potential non-obvious things are exactly why we've made that dmesg warning about uninitialized randomness more clear, should it ever come up.

So, I'll send you a patch for this, and then let you decide which approach you'd rather merge.

Yea, that's a great idea. I might have a poke at it soon.

Okay, that all makes sense!

Talk soon,
Jason

Hi Ted,

One other question -- you plan on pushing this to Linus for 4.12, correct?

Thanks,
Jason