[PATCH cleanups/resubmit 0/4] Cleanups & Resubmissions for Random-dev

[PATCH cleanups/resubmit 0/4] Cleanups & Resubmissions for Random-dev

[Jason A. Donenfeld]

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

[PATCH cleanups/resubmit 1/4] security/keys: ensure RNG is seeded before use

[Jason A. Donenfeld]

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.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
---
 security/keys/encrypted-keys/encrypted.c | 8 +++++---
 security/keys/key.c                      | 8 +++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 0010955d7876..d51a28fc5cd5 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -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;
diff --git a/security/keys/key.c b/security/keys/key.c
index 455c04d80bbb..b1db72d0ab1b 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -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

[PATCH cleanups/resubmit 2/4] random: squelch sh compiler warning and ensure safe optimization

[Jason A. Donenfeld]

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.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3bdeef13afda..cef9358ecf20 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -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

[PATCH cleanups/resubmit 3/4] bluetooth/smp: ensure RNG is properly seeded before ECDH use

[Jason A. Donenfeld]

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>
---
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(-)

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

[PATCH cleanups/resubmit 4/4] crypto/rng: ensure that the RNG is ready before using

[Jason A. Donenfeld]

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>
---
This is the same as v4/v5.

 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

Re: [PATCH cleanups/resubmit 0/4] Cleanups & Resubmissions for Random-dev

[Theodore Ts'o]

On Thu, Jun 08, 2017 at 02:29:28PM +0200, Jason A. Donenfeld wrote:
> Hi Ted,
> 
> I'm not super sure what your workflow is like, but yesterday you picked
> up my v4 instead of my v5

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.

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

Thanks!

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

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

>   bluetooth/smp: ensure RNG is properly seeded before ECDH use
> 
> This one is pending your approval from the bluetooth people.

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.  :-)

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

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.

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

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

Re: [PATCH cleanups/resubmit 0/4] Cleanups & Resubmissions for Random-dev

[Jason A. Donenfeld]

Hey Ted,

On Thu, Jun 8, 2017 at 11:58 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>>   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.
>
> Or I can just grab the updated commit from v5, yes?

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.

>
>>   bluetooth/smp: ensure RNG is properly seeded before ECDH use
>>
>> This one is pending your approval from the bluetooth people.
>
> I think they said it would be enough to add wait_for_random_bytes()
> to the hci_power_on function

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.

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

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

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

Okay, that all makes sense!

Talk soon,
Jason

Re: [PATCH cleanups/resubmit 0/4] Cleanups & Resubmissions for Random-dev

[Jason A. Donenfeld]

Hi Ted,

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

Thanks,
Jason