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
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 <[email protected]>
---
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
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 <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
---
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 <[email protected]>. All
+ * Rights Reserved.
+ *
* Copyright Matt Mackall <[email protected]>, 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
This is much faster and just as secure. It also has the added benefit of
probably returning better randomness at early-boot on systems with
architectural RNGs.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Thomas Graf <[email protected]>
Cc: Herbert Xu <[email protected]>
---
lib/rhashtable.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index d9e7274a04cd..a1eb7c947f46 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -235,7 +235,7 @@ static struct bucket_table *bucket_table_alloc(struct rhashtable *ht,
INIT_LIST_HEAD(&tbl->walkers);
- get_random_bytes(&tbl->hash_rnd, sizeof(tbl->hash_rnd));
+ tbl->hash_rnd = get_random_u32();
for (i = 0; i < nbuckets; i++)
INIT_RHT_NULLS_HEAD(tbl->buckets[i], ht, i);
--
2.13.0
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 <[email protected]>
Cc: David Miller <[email protected]>
---
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
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 <[email protected]>
Cc: David Howells <[email protected]>
Cc: Mimi Zohar <[email protected]>
Cc: David Safford <[email protected]>
---
security/keys/encrypted-keys/encrypted.c | 8 +++++---
security/keys/key.c | 16 ++++++++--------
2 files changed, 13 insertions(+), 11 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..b72078e532f2 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -134,17 +134,15 @@ void key_user_put(struct key_user *user)
* Allocate a serial number for a key. These are assigned randomly to avoid
* security issues through covert channel problems.
*/
-static inline void key_alloc_serial(struct key *key)
+static inline int 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);
@@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
rb_insert_color(&key->serial_node, &key_serial_tree);
spin_unlock(&key_serial_lock);
- return;
+ return 0;
/* we found a key with the proposed serial number - walk the tree from
* that point looking for the next unused serial number */
@@ -314,7 +312,9 @@ struct key *key_alloc(struct key_type *type, const char *desc,
/* publish the key by giving it a serial number */
atomic_inc(&user->nkeys);
- key_alloc_serial(key);
+ ret = key_alloc_serial(key);
+ if (ret < 0)
+ goto security_error;
error:
return key;
--
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.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Marcel Holtmann <[email protected]>
Cc: Gustavo Padovan <[email protected]>
Cc: Johan Hedberg <[email protected]>
---
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
These functions are simple convenience wrappers that call
wait_for_random_bytes before calling the respective get_random_*
function.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
include/linux/net.h | 2 ++
include/linux/once.h | 2 ++
include/linux/random.h | 25 +++++++++++++++++++++++++
3 files changed, 29 insertions(+)
diff --git a/include/linux/net.h b/include/linux/net.h
index abcfa46a2bd9..dda2cc939a53 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -274,6 +274,8 @@ do { \
#define net_get_random_once(buf, nbytes) \
get_random_once((buf), (nbytes))
+#define net_get_random_once_wait(buf, nbytes) \
+ get_random_once_wait((buf), (nbytes))
int kernel_sendmsg(struct socket *sock, struct msghdr *msg, struct kvec *vec,
size_t num, size_t len);
diff --git a/include/linux/once.h b/include/linux/once.h
index 285f12cb40e6..9c98aaa87cbc 100644
--- a/include/linux/once.h
+++ b/include/linux/once.h
@@ -53,5 +53,7 @@ void __do_once_done(bool *done, struct static_key *once_key,
#define get_random_once(buf, nbytes) \
DO_ONCE(get_random_bytes, (buf), (nbytes))
+#define get_random_once_wait(buf, nbytes) \
+ DO_ONCE(get_random_bytes_wait, (buf), (nbytes)) \
#endif /* _LINUX_ONCE_H */
diff --git a/include/linux/random.h b/include/linux/random.h
index e29929347c95..4aecc339558d 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -58,6 +58,31 @@ static inline unsigned long get_random_long(void)
#endif
}
+/* Calls wait_for_random_bytes() and then calls get_random_bytes(buf, nbytes).
+ * Returns the result of the call to wait_for_random_bytes. */
+static inline int get_random_bytes_wait(void *buf, int nbytes)
+{
+ int ret = wait_for_random_bytes();
+ if (unlikely(ret))
+ return ret;
+ get_random_bytes(buf, nbytes);
+ return 0;
+}
+
+#define declare_get_random_var_wait(var) \
+ static inline int get_random_ ## var ## _wait(var *out) { \
+ int ret = wait_for_random_bytes(); \
+ if (unlikely(ret)) \
+ return ret; \
+ *out = get_random_ ## var(); \
+ return 0; \
+ }
+declare_get_random_var_wait(u32)
+declare_get_random_var_wait(u64)
+declare_get_random_var_wait(int)
+declare_get_random_var_wait(long)
+#undef declare_get_random_var
+
unsigned long randomize_page(unsigned long start, unsigned long range);
u32 prandom_u32(void);
--
2.13.0
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 <[email protected]>
---
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
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 <[email protected]>
Cc: "Nicholas A. Bellinger" <[email protected]>
Cc: Lee Duncan <[email protected]>
Cc: Chris Leech <[email protected]>
---
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
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 <[email protected]>
Cc: David Miller <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
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
Ceph uses the RNG for various nonce generations, and it shouldn't accept
using bad randomness. So, we wait for the RNG to be properly seeded. We
do this by calling wait_for_random_bytes() in a function that is
certainly called in process context, early on, so that all subsequent
calls to get_random_bytes are necessarily acceptable.
Signed-off-by: Jason A. Donenfeld <[email protected]>
Cc: Ilya Dryomov <[email protected]>
Cc: "Yan, Zheng" <[email protected]>
Cc: Sage Weil <[email protected]>
---
net/ceph/ceph_common.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/ceph/ceph_common.c b/net/ceph/ceph_common.c
index 47e94b560ba0..0368a04995b3 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -598,7 +598,11 @@ struct ceph_client *ceph_create_client(struct ceph_options *opt, void *private)
{
struct ceph_client *client;
struct ceph_entity_addr *myaddr = NULL;
- int err = -ENOMEM;
+ int err;
+
+ err = wait_for_random_bytes();
+ if (err < 0)
+ return ERR_PTR(err);
client = kzalloc(sizeof(*client), GFP_KERNEL);
if (client == NULL)
--
2.13.0
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 <[email protected]>
Cc: Steve French <[email protected]>
---
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
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 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
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 <[email protected]>. All
> + * Rights Reserved.
> + *
> * Copyright Matt Mackall <[email protected]>, 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
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/
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 <[email protected]>
---
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
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
On Fri, Jun 16, 2017 at 10:31 AM, Sebastian Andrzej Siewior
<[email protected]> 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
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 <[email protected]>
> ---
> 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
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
On Fri, Jun 16, 2017 at 4:35 PM, Sebastian Andrzej Siewior
<[email protected]> 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
On 2017-06-17 02:39:40 [+0200], Jason A. Donenfeld wrote:
> On Fri, Jun 16, 2017 at 4:35 PM, Sebastian Andrzej Siewior
> <[email protected]> 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
On Mon, Jun 19, 2017 at 9:45 AM, Sebastian Andrzej Siewior
<[email protected]> 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.
Hello Ted,
With rc6 already released and rc7 coming up, I'd really appreciate you
stepping in here and either ACKing the above commit, or giving your
two cents about it in case I need to roll something different.
Thanks,
Jason
On Thu, Jun 15, 2017 at 12:45 AM, Jason A. Donenfeld <[email protected]> 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.
>
> 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 <[email protected]>
> ---
> 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
>
On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
>
> With rc6 already released and rc7 coming up, I'd really appreciate you
> stepping in here and either ACKing the above commit, or giving your
> two cents about it in case I need to roll something different.
I actually had set up an earlier version of your patch for on Saturday
while I was in Beijing. (Like Linus, I'm attending the LinuxCon China
conference Monday and Tuesday.) I had even created the signed tag,
but I didn't send the pull request to Linus because I was waiting to
see about how discussions over the locking strategy and the spammy log
messages on PowerPC was going to get resolved.
I've since respun the commit to reflect your newer patch (see the
random_for_linus_stable tag on random.git) and rebased the dev branch
on top of that. Please take a look and comment.
The other open issue I want to resolve before sending a pull request
this week is whether we want to change the default for
CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'. It *is* spammy
for PowerPC, because they aren't getting their CRNG initialized
quickly enough, so several userspace processes are getting
fork/exec'ed with an uninitialized CRNG. That being said, it is a
valid warning because it means that the initial stack canary for the
first couple of PowerPC processes are being created without a fully
initialized CRNG, which may mean that an attacker might be able to
circumvent the stack canary on the first couple of processes. So that
could potentially be a real security issue on Power. OTOH, most Power
users aren't going to be able to do anything about the fact the stack
canaries of the system daemons started during early boot don't have
strong randomness, so perhaps we should disable the warning by
default.
Opinions?
- Ted
On Tue, Jun 20, 2017 at 3:33 PM, Theodore Ts'o <[email protected]> wrote:
> On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
>>
>> With rc6 already released and rc7 coming up, I'd really appreciate you
>> stepping in here and either ACKing the above commit, or giving your
>> two cents about it in case I need to roll something different.
>
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing. (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.) I had even created the signed tag,
> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
>
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that. Please take a look and comment.
>
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'. It *is* spammy
> for PowerPC, because they aren't getting their CRNG initialized
> quickly enough, so several userspace processes are getting
> fork/exec'ed with an uninitialized CRNG.
It's very spammy for ARM as well. I booted next-20170619 on an Aspeed
(32-bit ARM) board and by the time I made it to a shell the log buffer
contained only warnings:
[ 10.452921] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[ 10.461255] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[ 10.471464] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[ 10.480429] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0
[ 10.494802] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[ 10.503141] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[ 10.511571] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[ 10.520527] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0
[ 10.537847] random: arch_pick_mmap_layout+0xa8/0xe8 get_random_u32
called with crng_init=0
[ 10.546182] random: load_elf_binary+0x3c8/0x104c get_random_u32
called with crng_init=0
[ 10.554611] random: arch_setup_additional_pages+0x6c/0x110
get_random_u32 called with crng_init=0
[ 10.563563] random: randomize_page+0x44/0x58 get_random_u32 called
with crng_init=0
So +1 for defaulting CONFIG_WARN_UNSEEDED_RANDOM=n.
Cheers,
Joel
> That being said, it is a
> valid warning because it means that the initial stack canary for the
> first couple of PowerPC processes are being created without a fully
> initialized CRNG, which may mean that an attacker might be able to
> circumvent the stack canary on the first couple of processes. So that
> could potentially be a real security issue on Power. OTOH, most Power
> users aren't going to be able to do anything about the fact the stack
> canaries of the system daemons started during early boot don't have
> strong randomness, so perhaps we should disable the warning by
> default.
>
> Opinions?
>
> - Ted
On 2017-06-19 22:55:37 [+0200], Jason A. Donenfeld wrote:
> On Mon, Jun 19, 2017 at 9:45 AM, Sebastian Andrzej Siewior
> <[email protected]> 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.
so you need to find a another way then. Doing the assignment after
dropping the lock opens another race.
> >> > 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.
I don't think the compiler is allowed to inline it the way you describe
it. This would render any assignment to local variable useless. Also
the READ_ONCE creates worse code in this case (because the read can not
be delayed).
Sebastian
Theodore Ts'o <[email protected]> writes:
> On Mon, Jun 19, 2017 at 10:57:18PM +0200, Jason A. Donenfeld wrote:
>>
>> With rc6 already released and rc7 coming up, I'd really appreciate you
>> stepping in here and either ACKing the above commit, or giving your
>> two cents about it in case I need to roll something different.
>
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing. (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.) I had even created the signed tag,
> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
>
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that. Please take a look and comment.
>
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.
Yes please.
> It *is* spammy for PowerPC, because they aren't getting their CRNG
*some* powerpc machines ...
> initialized quickly enough, so several userspace processes are getting
> fork/exec'ed with an uninitialized CRNG. That being said, it is a
> valid warning because it means that the initial stack canary for the
> first couple of PowerPC processes are being created without a fully
> initialized CRNG, which may mean that an attacker might be able to
> circumvent the stack canary on the first couple of processes. So that
> could potentially be a real security issue on Power. OTOH, most Power
> users aren't going to be able to do anything about the fact the stack
> canaries of the system daemons started during early boot don't have
> strong randomness, so perhaps we should disable the warning by
> default.
powerpc supports a wide range of hardware platforms, some of which are
10-15 years old, and don't have a hardware RNG.
Is there anything we can do on those machines? Seems like our only
option would be to block the boot while some more "entropy" builds up,
but that's unlikely to be popular with users.
On our newer machines (>= Power8) we have a hardware RNG which we wire
up to arch_get_random_seed_long(), so on those machines the warnings
would be valid, because they'd indicate a bug.
So I think it should be up to arches to decide whether this is turned on
via their defconfigs, and the default should be 'n' because a lot of old
hardware won't be able to do anything useful with the warnings.
cheers
Hey Ted,
On Tue, Jun 20, 2017 at 02:03:44AM -0400, Theodore Ts'o wrote:
> I actually had set up an earlier version of your patch for on Saturday
> while I was in Beijing. (Like Linus, I'm attending the LinuxCon China
> conference Monday and Tuesday.) I had even created the signed tag,
> I've since respun the commit to reflect your newer patch (see the
> random_for_linus_stable tag on random.git) and rebased the dev branch
> on top of that. Please take a look and comment.
So it looks like you've gone with 4a072c71f49. If that looks good
(moving the lock, etc) to you, then great, we're done. If there are
still locking objections (are there?), then we'll need to revisit.
> but I didn't send the pull request to Linus because I was waiting to
> see about how discussions over the locking strategy and the spammy log
> messages on PowerPC was going to get resolved.
> The other open issue I want to resolve before sending a pull request
> this week is whether we want to change the default for
> CONFIG_WARN_UNSEEDED_RANDOM so that the answer is 'n'.
In the v1 of this patch many moons ago, it was just vanilla, default y,
but due to the spamminess, I thought folks would revolt. So I made a
change:
Specifically, I added `depends on DEBUG_KERNEL`. This means that these
useful warnings will only poke other kernel developers. This is probably
exactly what we want. If the various associated developers see a warning
coming from their particular subsystem, they'll be more motivated to
fix it. Ordinary users on distribution kernels shouldn't see the
warnings or the spam at all, since typically users aren't using
DEBUG_KERNEL.
Then, to make things _even less_ annoying to kernel developers, you
added a nice patch on top to squelch repeated messages.
So, I still think this current strategy is a good one, of default y, but
depends on DEBUG_KERNEL.
Regards,
Jason
On Tue, Jun 20, 2017 at 4:14 AM, Jason A. Donenfeld <[email protected]> wrote:
>...
> Specifically, I added `depends on DEBUG_KERNEL`. This means that these
> useful warnings will only poke other kernel developers. This is probably
> exactly what we want. If the various associated developers see a warning
> coming from their particular subsystem, they'll be more motivated to
> fix it. Ordinary users on distribution kernels shouldn't see the
> warnings or the spam at all, since typically users aren't using
> DEBUG_KERNEL.
I think it is a bad idea to suppress all messages from a security
engineering point of view.
Many folks don't run debug kernels. Most of the users who want or need
to know of the issues won't realize its happening. Consider, the
reason we learned of systemd's problems was due to dmesg's.
Suppressing all messages for all configurations cast a wider net than
necessary. Configurations that could potentially be detected and fixed
likely will go unnoticed. If the problem is not brought to light, then
it won't be fixed.
I feel like the kernel is making policy decisions for some
organizations. For those who have hardware that is effectively
unfixable, then organization has to decide what to do based on their
risk adversity. They may decide to live with the risk, or they may
decide to refresh the hardware. However, without information on the
issue, they may not even realize they have an actionable item.
Jeff
On Tue, Jun 20, 2017 at 10:33 AM, Jeffrey Walton <[email protected]> wrote:
> I think it is a bad idea to suppress all messages from a security
> engineering point of view.
>
> Many folks don't run debug kernels. Most of the users who want or need
> to know of the issues won't realize its happening. Consider, the
> reason we learned of systemd's problems was due to dmesg's.
>
> Suppressing all messages for all configurations cast a wider net than
> necessary. Configurations that could potentially be detected and fixed
> likely will go unnoticed. If the problem is not brought to light, then
> it won't be fixed.
I more or less agree with you that we should just turn this on for all
users and they'll just have to live with the spam and report odd
entries, and overtime we'll fix all the violations.
But I think there's another camp that would mutiny in the face of this
kind of hubris.
That's why I moved pretty readily toward the compromise position of
default y, but depends on DEBUG_KERNEL. My hope was that it'd to an
extent satisfy both camps, and also disappoint both camps in an equal
way.
On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
> > Suppressing all messages for all configurations cast a wider net than
> > necessary. Configurations that could potentially be detected and fixed
> > likely will go unnoticed. If the problem is not brought to light, then
> > it won't be fixed.
>
> I more or less agree with you that we should just turn this on for all
> users and they'll just have to live with the spam and report odd
> entries, and overtime we'll fix all the violations.
Fix all the problems *how*? If you are on an old system which doesn't
a hardware random number generator, and which doesn't have a high
resolution cycle counter, and may not have a lot of entropy easily
harvestable from the environment, there may not be a lot you can do.
Sure, you can pretend that the cache (which by the way is usually
determinstic) is ***so*** complicated that no one can figure it out,
and essentially pretend that you have entropy when you probably don't;
that just simply becomes a different way of handwaving and suppressing
the warning messages.
> But I think there's another camp that would mutiny in the face of this
> kind of hubris.
Blocking the boot for hours and hours until we have enough entropy to
initialize the CRNG is ***not*** an acceptable way of making the
warning messages go away. Do that and the users **will** mutiny.
It's this sort of attitude which is why Linus has in the past said
that security people are sometimes insane....
- Ted
On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <[email protected]> wrote:
> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>> > Suppressing all messages for all configurations cast a wider net than
>> > necessary. Configurations that could potentially be detected and fixed
>> > likely will go unnoticed. If the problem is not brought to light, then
>> > it won't be fixed.
>>
>> I more or less agree with you that we should just turn this on for all
>> users and they'll just have to live with the spam and report odd
>> entries, and overtime we'll fix all the violations.
>
> Fix all the problems *how*? If you are on an old system which doesn't
> a hardware random number generator, and which doesn't have a high
> resolution cycle counter, and may not have a lot of entropy easily
> harvestable from the environment, there may not be a lot you can do.
> Sure, you can pretend that the cache (which by the way is usually
> determinstic) is ***so*** complicated that no one can figure it out,
> and essentially pretend that you have entropy when you probably don't;
> that just simply becomes a different way of handwaving and suppressing
> the warning messages.
>
>> But I think there's another camp that would mutiny in the face of this
>> kind of hubris.
>
> Blocking the boot for hours and hours until we have enough entropy to
> initialize the CRNG is ***not*** an acceptable way of making the
> warning messages go away. Do that and the users **will** mutiny.
>
> It's this sort of attitude which is why Linus has in the past said
> that security people are sometimes insane....
I don't believe it has anything to do with insanity. Its sound
security engineering.
Are there compelling reasons a single dmesg warning cannot be provided?
A single message avoids spamming the logs. It also informs the system
owner of the problem. An individual or organization can then take
action based on their risk posture. Finally, it avoids the kernel
making policy decisions for a user or organization.
Jeff
On Tue, Jun 20, 2017 at 11:36 AM, Theodore Ts'o <[email protected]> wrote:
>> But I think there's another camp that would mutiny in the face of this
>> kind of hubris.
>
> Blocking the boot for hours and hours until we have enough entropy to
> initialize the CRNG is ***not*** an acceptable way of making the
> warning messages go away. Do that and the users **will** mutiny.
>
> It's this sort of attitude which is why Linus has in the past said
> that security people are sometimes insane....
Uh, talk about a totally unnecessary punch... In case my last email
wasn't clear, I fully recognize that `default y` is a tad too extreme,
which is why from one of the earliest revisions in this series, I
moved directly to the compromise solution (`depends DEBUG_KERNEL`)
without even waiting for people to complain first.
On Tue, Jun 20, 2017 at 5:49 AM, Jeffrey Walton <[email protected]> wrote:
> On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <[email protected]> wrote:
>> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>>> > Suppressing all messages for all configurations cast a wider net than
>>> > necessary. Configurations that could potentially be detected and fixed
>>> > likely will go unnoticed. If the problem is not brought to light, then
>>> > it won't be fixed.
> Are there compelling reasons a single dmesg warning cannot be provided?
>
> A single message avoids spamming the logs. It also informs the system
> owner of the problem. An individual or organization can then take
> action based on their risk posture. Finally, it avoids the kernel
> making policy decisions for a user or organization.
I'd say the best solution is to have no configuration option
specifically for these messages. Always give some, but let
DEBUG_KERNEL control how many.
If DEBUG_KERNEL is not set, emit exactly one message & ignore any
other errors of this type. On some systems, that message may have to
be ignored, on some it might start an incremental process where one
problem gets fixed only to have another crop up & on some it might
prompt the admin to explore further by compiling with DEBUG_KERNEL.
If DEBUG_KERNEL is set, emit a message for every error of this type.
On Tue, Jun 20, 2017 at 10:50 AM, Sandy Harris <[email protected]> wrote:
> On Tue, Jun 20, 2017 at 5:49 AM, Jeffrey Walton <[email protected]> wrote:
>> On Tue, Jun 20, 2017 at 5:36 AM, Theodore Ts'o <[email protected]> wrote:
>>> On Tue, Jun 20, 2017 at 10:53:35AM +0200, Jason A. Donenfeld wrote:
>
>>>> > Suppressing all messages for all configurations cast a wider net than
>>>> > necessary. Configurations that could potentially be detected and fixed
>>>> > likely will go unnoticed. If the problem is not brought to light, then
>>>> > it won't be fixed.
>
>> Are there compelling reasons a single dmesg warning cannot be provided?
>>
>> A single message avoids spamming the logs. It also informs the system
>> owner of the problem. An individual or organization can then take
>> action based on their risk posture. Finally, it avoids the kernel
>> making policy decisions for a user or organization.
>
> I'd say the best solution is to have no configuration option
> specifically for these messages. Always give some, but let
> DEBUG_KERNEL control how many.
>
> If DEBUG_KERNEL is not set, emit exactly one message & ignore any
> other errors of this type. On some systems, that message may have to
> be ignored, on some it might start an incremental process where one
> problem gets fixed only to have another crop up & on some it might
> prompt the admin to explore further by compiling with DEBUG_KERNEL.
>
> If DEBUG_KERNEL is set, emit a message for every error of this type.
How about doing this:
default DEBUG_KERNEL
Most distro kernel select DEBUG_KERNEL because it unhides a bunch of
other useful configs. Since it doesn't strictly _depend_ on
DEBUG_KERNEL, I think it's probably a mistake to enforce a false
dependency. Using it as a hint for the default seems maybe like a good
middle ground. (And if people can't agree on that, then I guess
"default n"...)
-Kees
--
Kees Cook
Pixel Security
On Tue, Jun 20, 2017 at 8:14 PM, Kees Cook <[email protected]> wrote:
> How about doing this:
>
> default DEBUG_KERNEL
>
> Most distro kernel select DEBUG_KERNEL because it unhides a bunch of
> other useful configs. Since it doesn't strictly _depend_ on
> DEBUG_KERNEL, I think it's probably a mistake to enforce a false
> dependency. Using it as a hint for the default seems maybe like a good
> middle ground. (And if people can't agree on that, then I guess
> "default n"...)
I didn't know you could do that with Kconfig. Great idea. I'll make
this change and submit a new patch to Ted.
Jason
On Tue, Jun 20, 2017 at 11:49:07AM +0200, Jason A. Donenfeld wrote:
> Uh, talk about a totally unnecessary punch... In case my last email
> wasn't clear, I fully recognize that `default y` is a tad too extreme,
> which is why from one of the earliest revisions in this series, I
> moved directly to the compromise solution (`depends DEBUG_KERNEL`)
> without even waiting for people to complain first.
The punch was in response to this statement, which I personally found
fairly infuriating:
>> I more or less agree with you that we should just turn this on for all
>> users and they'll just have to live with the spam and report odd
>> entries, and overtime we'll fix all the violations.
There seems to be a fundamental misapprehension that it will be easy
to "fix all the violations". For certain hardware types, this is
not easy, and the "eh, let them get spammed until we get around to
fixing it" attitude is precisely what I was pushing back against.
There's a certain amount of privilege for those of us who are using
x86 systems with built-in hardware random number generators, and cycle
counters, where the problem is much easier to solve. But for other
platforms, it really, REALLY isn't that easy to fix.
One solution that might be acceptable is to simply print a *single*
warning, the first time some piece of kernel code tries to get
randomness before the CRNG is initialized. And that's it. If it's
only a single dmesg line, then we probably don't need to hide it
behind a #ifdef. That might satisfy the security-obsessed who want to
rub users' noses in the face that their kernel is doing something
potentially insecure and there is nothing they can do about it. But
since it's also a single line in the syslog, it's not actively
annoying.
The #ifdef will allow the current behaviour where we suppress
duplicates, but we warn for every attempt to get randomness. That we
can default to no, since it will only be people who are trying to
audit calls to see if the real fix is to switch the call to
prandom_u32, because the use case really was't security/crypto
sensitive.
As I have said before, ultimately I think the only real solution to
this whole mess is to allow the bootloader to read entropy from the
boot device (or maybe some NVRAM or battery-backed memory), which is
then overwritten as early as possible in the boot process with new
random data. This is what OpenBSD does, but OpenBSD has a much easier
job because they only have to support a small set of architectures.
We will need to do this for each bootloader that is used by Linux,
which is a pretty large set. But ultimately, it solves *all* the
problems, including getting entropy for KASLR, which needs the
randomness super-early in the boot process, long before we have any
hope of initializing the entropy pool from environmental noise.
So personally, I think this focus on trying to warn/spam users is not
particularly useful. If we can mute the warnings to those people who
want to play whack-a-mole, it's not harmful, but for those who think
that futzing with get_random_* calls is the right approach, personally
I'm really not convinced. Of course, the kernel is a volunteer
project, so ultimately all a maintainer can do is to say no to
patches, and not command people to work on things that he or she
wishes. I *can* try to pursude people about what the right way to go
is, because doing something that involves boot loaders is going to
require a huge amount of effort from many people. It's certainly not
anything I or anyone else can do by him or herself.
- Ted
On Wed, Jun 21, 2017 at 1:38 AM, Theodore Ts'o <[email protected]> wrote:
> The punch was in response to this statement, which I personally found
> fairly infuriating:
>
>>> I more or less agree with you that we should just turn this on for all
>>> users and they'll just have to live with the spam and report odd
>>> entries, and overtime we'll fix all the violations.
Holy cow, please cool it. I think the "or less" part was relevant, as
was the subsequent sentence which characterized that sentiment as
"hubris". Also, the subsequent email when I made explicit the fact
that I was more in agreement with you than you interpreted. So, in
case you're really really really not getting the message I'm trying to
make so explicitly clear to you: I AGREE WITH YOU. So, no more
punching, pretty please?
>
> There seems to be a fundamental misapprehension that it will be easy
> to "fix all the violations".
I don't think it will be easy. I agree with you.
> But for other
> platforms, it really, REALLY isn't that easy to fix.
Other platforms will be hard. I agree with you.
> So personally, I think
I'm going to roll Kees' suggestion into a PATCH and send it to you.
You can decide if you want to apply it. I'll be satisfied with
whatever you choose and will follow your lead.
Jason
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 <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
Hi Ted,
This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
which is currently in your dev tree. It switches from using `default y`
and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
This kind of change I think should satisfy most potential objections, by
being present for those who might find it useful, but invisble for those
who don't want the spam.
If you'd like to replace the earlier commit with this one, feel free. If
not, that's fine too.
Jason
drivers/char/random.c | 15 +++++++++++++--
lib/Kconfig.debug | 15 +++++++++++++++
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3853dd4f92e7..fa5bbd5a7ca0 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..41cf12288369 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,6 +1209,21 @@ 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 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.1
On Tue, Jun 20, 2017 at 5:03 PM, Jason A. Donenfeld <[email protected]> wrote:
> 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.
This commit log needs updating (default DEBUG_KERNEL, not depends).
But otherwise:
Reviewed-by: Kees Cook <[email protected]>
-Kees
>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> Hi Ted,
>
> This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
> which is currently in your dev tree. It switches from using `default y`
> and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
> This kind of change I think should satisfy most potential objections, by
> being present for those who might find it useful, but invisble for those
> who don't want the spam.
>
> If you'd like to replace the earlier commit with this one, feel free. If
> not, that's fine too.
>
> Jason
>
> drivers/char/random.c | 15 +++++++++++++--
> lib/Kconfig.debug | 15 +++++++++++++++
> 2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3853dd4f92e7..fa5bbd5a7ca0 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..41cf12288369 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1209,6 +1209,21 @@ 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 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.1
>
--
Kees Cook
Pixel Security
"Jason A. Donenfeld" <[email protected]> writes:
> 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.
All the distro kernels I'm aware of have DEBUG_KERNEL=y.
Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.
So it's still essentially default y.
Emitting *one* warning by default would be reasonable. That gives users
who are interested something to chase, they can then turn on the option
to get the full story.
Filling the dmesg buffer with repeated warnings is really not helpful.
cheers
On Wed, Jun 21, 2017 at 04:06:49PM +1000, Michael Ellerman wrote:
> All the distro kernels I'm aware of have DEBUG_KERNEL=y.
>
> Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.
>
> So it's still essentially default y.
>
> Emitting *one* warning by default would be reasonable. That gives users
> who are interested something to chase, they can then turn on the option
> to get the full story.
>
> Filling the dmesg buffer with repeated warnings is really not helpful.
I agree completely with all of this. The following patch replaces the
current topmost patch on the random.git tree:
>From 25b683ee9bd5536807f813efbd19809333461f89 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Thu, 8 Jun 2017 04:16:59 -0400
Subject: [PATCH] random: suppress spammy warnings about unseeded randomness
Unfortunately, on some models of some architectures getting a fully
seeded CRNG is extremely difficult, and so this can result in dmesg
getting spammed for a surprisingly long time. This is really bad from
a security perspective, and so architecture maintainers needed to do
what they can to get the CRNG seeded sooner after the system is
booted. However, users can't do anything actionble to address this,
and spamming the kernel messages log will only just annoy people.
For developers who want to work on improving this situation,
CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
CONFIG_WARN_ALL_UNSEEDED_RANDOM. By default the kernel will always
print the first use of unseeded randomness. This way, hopefully the
security obsessed will be happy that there is _some_ indication when
the kernel boots there may be a potential issue with that architecture
or subarchitecture. To see all uses of unseeded randomness,
developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.
Signed-off-by: Theodore Ts'o <[email protected]>
---
drivers/char/random.c | 45 ++++++++++++++++++++++++++++++---------------
lib/Kconfig.debug | 24 ++++++++++++++++++------
2 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index fa5bbd5a7ca0..7405c914bbcf 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1466,6 +1466,30 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
return ret;
}
+#define warn_unseeded_randomness(previous) \
+ _warn_unseeded_randomness(__func__, (void *) _RET_IP_, (previous))
+
+static void _warn_unseeded_randomness(const char *func_name, void *caller,
+ void **previous)
+{
+#ifdef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+ const bool print_once = false;
+#else
+ static bool print_once __read_mostly;
+#endif
+
+ if (print_once ||
+ crng_ready() ||
+ (previous && (caller == READ_ONCE(*previous))))
+ return;
+ WRITE_ONCE(*previous, caller);
+#ifndef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+ print_once = true;
+#endif
+ pr_notice("random: %s called from %pF with crng_init=%d\n",
+ func_name, caller, crng_init);
+}
+
/*
* This function is the exported kernel interface. It returns some
* number of good random numbers, suitable for key generation, seeding
@@ -1479,12 +1503,9 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
void get_random_bytes(void *buf, int nbytes)
{
__u8 tmp[CHACHA20_BLOCK_SIZE];
+ static void *previous;
-#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);
-#endif
+ warn_unseeded_randomness(&previous);
trace_get_random_bytes(nbytes, _RET_IP_);
while (nbytes >= CHACHA20_BLOCK_SIZE) {
@@ -2064,6 +2085,7 @@ u64 get_random_u64(void)
bool use_lock = READ_ONCE(crng_init) < 2;
unsigned long flags = 0;
struct batched_entropy *batch;
+ static void *previous;
#if BITS_PER_LONG == 64
if (arch_get_random_long((unsigned long *)&ret))
@@ -2074,11 +2096,7 @@ 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
+ warn_unseeded_randomness(&previous);
batch = &get_cpu_var(batched_entropy_u64);
if (use_lock)
@@ -2102,15 +2120,12 @@ u32 get_random_u32(void)
bool use_lock = READ_ONCE(crng_init) < 2;
unsigned long flags = 0;
struct batched_entropy *batch;
+ static void *previous;
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
+ warn_unseeded_randomness(&previous);
batch = &get_cpu_var(batched_entropy_u32);
if (use_lock)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c4159605bfbf..4be6b7c66b69 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,10 +1209,9 @@ 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
+config WARN_ALL_UNSEEDED_RANDOM
+ bool "Warn for all uses unseeded randomness"
+ default n
help
Some parts of the kernel contain bugs relating to their use of
cryptographically secure random numbers before it's actually possible
@@ -1222,8 +1221,21 @@ config WARN_UNSEEDED_RANDOM
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.
+ Unfortunately, on some models of some architectures getting
+ a fully seeded CRNG is extremely difficult, and so this can
+ result in dmesg getting spammed for a surprisingly long
+ time. This is really bad from a security perspective, and
+ so architecture maintainers needed to do what they can to
+ get the CRNG seeded sooner after the system is booted.
+ However, since users can not do anything actionble to
+ address this, by default the kernel will issue only a single
+ warning for the first use of unseeded randomness.
+
+ Say Y here if you want to receive warnings for all uses of
+ unseeded randomness. This will be of use primarily for
+ those developers interersted in improving the security of
+ Linux kernels running on their architecture (or
+ subarchitecture).
config DEBUG_KOBJECT
bool "kobject debugging"
--
2.11.0.rc0.7.gbe5a750
On Tue, Jun 20, 2017 at 7:38 PM, Theodore Ts'o <[email protected]> wrote:
> On Tue, Jun 20, 2017 at 11:49:07AM +0200, Jason A. Donenfeld wrote:
>> ...
>>> I more or less agree with you that we should just turn this on for all
>>> users and they'll just have to live with the spam and report odd
>>> entries, and overtime we'll fix all the violations.
>
> There seems to be a fundamental misapprehension that it will be easy
> to "fix all the violations". For certain hardware types, this is
> not easy, and the "eh, let them get spammed until we get around to
> fixing it" attitude is precisely what I was pushing back against.
I can't speak for others, but for me: I think they will fall into
three categories:
1. easy to fix
2. difficult to fix
3. unable to fix
(1) is low hanging fruit and they will probably (hopefully?) be
cleared easily. Like systemd on x86_64 with rdrand and rdseed.
There's no reason for systemd to find itself starved of entropy on
that platform. (cf., http://github.com/systemd/systemd/issues/4167).
Organizations that find themselves in (3) can choose to use a board or
server and accept the risk, or they can choose to remediate it in
another way. The "other way" may include a capital expenditure and a
hardware refresh.
The central point is, they know about the risk and they can make the decision.
Jeff
Hi Ted,
On Wed, Jun 21, 2017 at 10:38 PM, Theodore Ts'o <[email protected]> wrote:
> I agree completely with all of this. The following patch replaces the
> current topmost patch on the random.git tree:
> For developers who want to work on improving this situation,
> CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
> CONFIG_WARN_ALL_UNSEEDED_RANDOM. By default the kernel will always
> print the first use of unseeded randomness. This way, hopefully the
> security obsessed will be happy that there is _some_ indication when
> the kernel boots there may be a potential issue with that architecture
> or subarchitecture. To see all uses of unseeded randomness,
> developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.
Seems fine to me.
Acked-by: Jason A. Donenfeld <[email protected]>
Jason