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, 5 of which are included in this patch set.
[1] http://www.openwall.com/lists/kernel-hardening/2017/06/02/2
Changes v2->v3:
- Since this issue, in general, is going to take a long time to fully
fix, the patch turning on the warning is now dependent on DEBUG_KERNEL
so that the right people see the messages but the others aren't annoyed.
- Fixed some inappropriate blocking for functions that load during module
insertion. As discussed in [1], module insertion deferal is a topic for
another patch set.
- An interesting and essential patch has been added for invalidating the
batched entropy pool after the crng initializes.
- Some places that need randomness at bootup for just small integers would
be better served by get_random_{u32,u64}, so this series makes those
changes in a few places. It's useful here, since on some architectures
that delivers better early randomness.
Jason A. Donenfeld (13):
random: add synchronous API for the urandom pool
random: add get_random_{bytes,u32,u64,int,long,once}_wait family
random: invalidate batched entropy after crng init
crypto/rng: ensure that the RNG is ready before using
security/keys: ensure RNG is seeded before use
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 | 90 ++++++++++++++++++++++++++-----
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, 195 insertions(+), 47 deletions(-)
--
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
Otherwise, we might be seeding the RNG using bad randomness, which is
dangerous.
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
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 0ab024918907..035a5d7c06bd 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -844,11 +844,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])
{
@@ -1466,7 +1461,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)
{
@@ -1496,6 +1494,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.
*
@@ -1849,6 +1865,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;
@@ -1861,9 +1879,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);
}
@@ -2023,7 +2041,10 @@ struct 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
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
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.
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
drivers/char/random.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 035a5d7c06bd..c328e9b11f1f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -762,6 +762,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;
@@ -800,6 +802,7 @@ static int crng_fast_load(const char *cp, size_t len)
}
if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
crng_init = 1;
+ invalidate_batched_entropy();
wake_up_interruptible(&crng_init_wait);
pr_notice("random: fast init done\n");
}
@@ -837,6 +840,7 @@ static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
crng->init_time = jiffies;
if (crng == &primary_crng && crng_init < 2) {
crng_init = 2;
+ invalidate_batched_entropy();
process_random_ready_list();
wake_up_interruptible(&crng_init_wait);
pr_notice("random: crng init done\n");
@@ -2037,6 +2041,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
@@ -2050,6 +2055,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;
struct batched_entropy *batch;
#if BITS_PER_LONG == 64
@@ -2062,11 +2069,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;
}
@@ -2076,22 +2087,45 @@ 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;
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
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 4fd02831beed..26ab58665f77 100644
--- a/net/ceph/ceph_common.c
+++ b/net/ceph/ceph_common.c
@@ -611,7 +611,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
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 c328e9b11f1f..d4698c8bc35f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -285,7 +285,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))
@@ -1474,7 +1473,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);
@@ -2068,6 +2067,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);
@@ -2094,6 +2099,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
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
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
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 655d9eebe43e..11e001a42094 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2936,8 +2936,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
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
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
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 66238477137b..5ef028c11738 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
On Tue, Jun 06, 2017 at 02:50:59AM +0200, Jason A. Donenfeld wrote:
> Otherwise, we might be seeding the RNG using bad randomness, which is
> dangerous.
>
> 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);
Note that crypto_rng_reset() is called by big_key_init() in
security/keys/big_key.c as a late_initcall(). So if we are on a
system where the crng doesn't get initialized until during the system
boot scripts, and big_key is compiled directly into the kernel, the
boot could end up deadlocking.
There may be other instances of where crypto_rng_reset() is called by
an initcall, so big_key_init() may not be an exhaustive enumeration of
potential problems. But this is an example of why the synchronous
API, although definitely much more convenient, can end up being a trap
for the unwary....
- Ted
Hey Ted,
On Tue, Jun 6, 2017 at 5:00 AM, Theodore Ts'o <[email protected]> wrote:
> Note that crypto_rng_reset() is called by big_key_init() in
> security/keys/big_key.c as a late_initcall(). So if we are on a
> system where the crng doesn't get initialized until during the system
> boot scripts, and big_key is compiled directly into the kernel, the
> boot could end up deadlocking.
>
> There may be other instances of where crypto_rng_reset() is called by
> an initcall, so big_key_init() may not be an exhaustive enumeration of
> potential problems. But this is an example of why the synchronous
> API, although definitely much more convenient, can end up being a trap
> for the unwary....
Thanks for pointing this out. I'll look more closely into it and see
if I can figure out a good way of approaching this.
Indeed you're right -- that we have to be really quite careful every
time we use the synchronous API. For this reason, I separated things
out into the wait_for_random_bytes and then the wrapper around
wait_for_random_bytes+get_random_bytes of get_random_bytes_wait. The
idea here would be that drivers could place a single
wait_for_random_bytes at some userspace entry point -- a configuration
ioctl, for example -- and then try to ensure that all calls to
get_random_bytes are ordered _after_ that wait_for_random_bytes call.
While this pattern doesn't fix all cases of unseeded get_random_bytes
calls -- we'll need to do some module loading order cleverness for
that, as we discussed in the other thread -- I think this pattern will
fix an acceptable amount of call sites, as seen here in this patchset,
that it makes it worthwhile. Having it, too, I think would encourage
other new drivers to think about when their calls to get_random_bytes
happens, and if it's possible for them to defer it until after a
userspace-blocking call to wait_for_random_bytes.
Anyway, I'll look into and fix up the problem you mentioned. Looking
forward to your feedback on the other patches here.
Regards,
Jason
On Tue, Jun 06, 2017 at 05:56:20AM +0200, Jason A. Donenfeld wrote:
> Hey Ted,
>
> On Tue, Jun 6, 2017 at 5:00 AM, Theodore Ts'o <[email protected]> wrote:
> > Note that crypto_rng_reset() is called by big_key_init() in
> > security/keys/big_key.c as a late_initcall(). So if we are on a
> > system where the crng doesn't get initialized until during the system
> > boot scripts, and big_key is compiled directly into the kernel, the
> > boot could end up deadlocking.
> >
> > There may be other instances of where crypto_rng_reset() is called by
> > an initcall, so big_key_init() may not be an exhaustive enumeration of
> > potential problems. But this is an example of why the synchronous
> > API, although definitely much more convenient, can end up being a trap
> > for the unwary....
>
> Thanks for pointing this out. I'll look more closely into it and see
> if I can figure out a good way of approaching this.
I don't think big_key even needs randomness at init time. The 'big_key_rng'
could just be removed and big_key_gen_enckey() changed to call
get_random_bytes(). (Or get_random_bytes_wait(), I guess; it's only reachable
via the keyring syscalls.)
It's going to take a while to go through all 217 users of get_random_bytes()
like this, though... It's really a shame there's no way to guarantee good
randomness at boot time.
Eric
On Mon, Jun 5, 2017 at 8:50 PM, Jason A. Donenfeld <[email protected]> wrote:
> These functions are simple convenience wrappers that call
> wait_for_random_bytes before calling the respective get_random_*
> function.
It may be advantageous to add a timeout, too.
There's been a number of times I did not want to wait an INFINITE
amount of time for a completion. (In another context).
Jeff
> 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);
Jason A. Donenfeld <[email protected]> wrote:
> + key->serial = get_random_u32() >> 1;
If this may sleep, it must be interruptible.
David
On Tue, Jun 6, 2017 at 7:11 AM, Jeffrey Walton <[email protected]> wrote:
> On Mon, Jun 5, 2017 at 8:50 PM, Jason A. Donenfeld <[email protected]> wrote:
>> These functions are simple convenience wrappers that call
>> wait_for_random_bytes before calling the respective get_random_*
>> function.
>
> It may be advantageous to add a timeout, too.
This was in v1, but was removed because of a lack of particular use
case in this context.
On Tue, Jun 6, 2017 at 12:08 PM, David Howells <[email protected]> wrote:
> Jason A. Donenfeld <[email protected]> wrote:
>
>> + key->serial = get_random_u32() >> 1;
>
> If this may sleep, it must be interruptible.
That won't sleep. I could have made it get_random_u32_wait(), but we'd
get into trouble at boottime. So instead, for now, I just use
get_random_u32 rather than get_random_bytes, which can use the
architectural random number generator, when the platform has one,
which is available early on.
Hi Eric,
On Tue, Jun 6, 2017 at 6:44 AM, Eric Biggers <[email protected]> wrote:
> I don't think big_key even needs randomness at init time. The 'big_key_rng'
> could just be removed and big_key_gen_enckey() changed to call
> get_random_bytes(). (Or get_random_bytes_wait(), I guess; it's only reachable
> via the keyring syscalls.)
That sounds good to me. I'll go ahead and make these changes, and will
add you to the Cc for the patch. You'll find the latest version in
here:
https://git.zx2c4.com/linux-dev/log/?h=jd/rng-blocker
> It's going to take a while to go through all 217 users of get_random_bytes()
> like this, though... It's really a shame there's no way to guarantee good
> randomness at boot time.
Yes, I agree whole-heartedly. A lot of people have proposals for
fixing the direct idea of entropy gathering, but for whatever reason,
Ted hasn't merged stuff. I think Stephan (CCd) rewrote big critical
sections of the RNG, called LRNG, and published a big paper for peer
review and did a lot of cool engineering, but for some reason this
hasn't been integrated. I look forward to movement on this front in
the future, if it ever happens. Would be great.
However, in lieu of that, I agree that playing whack a mole with all
call sites is mega arduous and error prone. In my original message to
Ted about this, I proposed instead a more global approach of
introducing an rng_init() to complement things like late_init() and
device_init() and such. The idea here would be two-fold:
- Modules that are built in would only be loaded as a callback to the
initialization of the RNG. An API for that already exists.
- Modules that are external would simply block userspace in
request_module until the RNG is initialized. This patch series adds
that kind of API.
If I understood correctly, Ted was worried that this might introduce
some headaches with module load ordering. However, IMHO, dealing with
the very few use cases of ordering issues is going to be far less
arduous than playing whack-a-mole with every call site. But, given the
fact that we still do need a blocking API (this patch series), I
decided to go with implementing this first, and then second attacking
the more difficult issue of adding rng_init().
So hopefully a combination of this patch series and the next one will
amount to something workable.
Long term, though, I think we need Stephan's work, in one form or
another, to be merged.
Jason
Hey again Eric,
One thing led to another and I wound up just rewriting all the crypto
in big_keys.c. I'll include this for v4:
https://git.zx2c4.com/linux-dev/commit/?h=jd/rng-blocker&id=886ff283b9808aecb14aa8e397da8496a9635aed
Not only was the use of crypto/rng inappropriate, but the decision to
go with aes-ecb is shocking. Seeing that this author had no other
commits in the tree, and that all subsequent commits that mentioned
his name were cleaning up his mess, I just went ahead and removed both
the crypto/rng misusage and changed from aes-ecb to aes-gcm.
Anyway, I'll wait for some more reviews on v3, and then this can be
reviewed for v4.
Regards,
Jason
On Tue, Jun 06, 2017 at 02:34:43PM +0200, Jason A. Donenfeld wrote:
>
> Yes, I agree whole-heartedly. A lot of people have proposals for
> fixing the direct idea of entropy gathering, but for whatever reason,
> Ted hasn't merged stuff. I think Stephan (CCd) rewrote big critical
> sections of the RNG, called LRNG, and published a big paper for peer
> review and did a lot of cool engineering, but for some reason this
> hasn't been integrated. I look forward to movement on this front in
> the future, if it ever happens. Would be great.
So it's not clear what you mean by Stephan's work. It can be
separated into multiple pieces; one is simply using a mechanism which
can be directly mapped to NIST's DRBG framework. I don't believe this
actually adds any real security per se, but it can make it easier to
get certification for people who care about getting FIPS
certification. Since I've seen a lot of snake oil and massive waste
of taxpayer and industry dollars by FIPS certification firms, it's not
a thing I particularly find particularly compelling.
The second bit is "Jitter Entropy". The problem I have with that is
there isn't any convincing explanation about why it can't be predicted
to some degree of accuracy with someone who understands what's going
on with Intel's cache architecture. (And this isn't just me, I've
talked to people who work at Intel and they are at best skeptical of
the whole idea.)
To be honest, there is a certain amount of this which is true with
harvesting interrupt timestamps, since for at least some interrupts
(in the worst case, the timer interrupt, especially on SOC's where all
of the clocks are generated from a single master oscillator) at least
some of the unpredictability is due to fact that the attacker needs to
understand what's going on with cache hits and misses, and that in
turn is impacted by compiler code generation, yadda, yadda, yadda.
The main thing then with trying to get entropy from sampling from the
environment is to have a mixing function that you trust, and that you
capture enough environmental data which hopefully is not available to
the attacker. So for example, radio strength measurements from the
WiFi data is not necessarily secret, but hopefully the information of
whether the cell phone is on your desk, or in your knapsack, either on
the desk, or under the desk, etc., is not available the analyst
sitting in Fort Meade (or Beijing, if you trust the NSA but not the
Ministry of State Security :-).
The judgement call is when you've gathered enough environmental data
(whether it is from CPU timing and cache misses if you are using
Jitter Entropy), or interupt timing, etc., is when you have enough
unpredictable data that it will be sufficient to protect you against
the attacker. We try to make some guesses of when we've gathered a
"bit" of entropy, but it's important to be humble here. We don't have
a theoretical framework for *any* of this, so the way we gather
metrics is really not all that scientific.
We also need to be careful not to fall into the trap of wishful
thinking. Yes, if we can say that the CRNG is fully initialized
before the init scripts are started, or even very early in the
initcall, then we can say yay! Problem solved!! But just because
someone *claims* that JitterEntropy will solve the problem, doesn't
necessarily mean it really does. I'm not accusing Stephan of trying
to deliberately sell snake oil; just that at least some poeople have
looked at it dubiously, and I would at least prefer to gather a lot
more environmental noise, and be more conservative before saying that
we're sure the CRNG is fully initialized.
The other approach is to find a way to have initialized "seed" entropy
which we can count on at every boot. The problem is that this is very
much dependent on how the bootloader works. It's easy to say "store
it in the kernel", but where the kernel is stored varies greatly from
architecture to architecture. In some cases, the kernel can stored in
ROM, where it can't be modified at all.
It might be possible, for example, to store a cryptographic key in a
UEFI boot-services variable, where the key becomes inaccessible after
the boot-time services terminate. But you also need either a reliable
time-of-day clock, or a reliable counter which is incremented each
time the system that boots, and which can't be messed with by an
attacker, or trivially reset by a clueless user/sysadmin.
Or maybe we can have a script that is run at shutdown and boot-up that
stashes 32 bytes of entropy in a reserved space accessible to GRUB,
and which GRUB then passes to the kernel using an extension to the
Linux/x86 Boot Protocol. (See Documentation/x86/boot.txt)
Quite frankly, I think this is actually a more useful and fruitful
path than either the whack-a-mole audit of all of the calls to
get_random_bytes() or adding a blocking variant to get_random_bytes()
(since in my opinion this becomes yet another version of whack-a-mole,
since each change to use the blocking variant requires an audit of how
the randomness is used, or where the function is called).
The reality though is that Linux is a volunteer effort, and so all a
maintainer can control is (a) is personal time, (b) whatever resources
his company may have entrusted him with, (c) trying to pursuade others
in the development community to do things (for which this e-mail is an
example :-), and ultimately, (d) the maintainer can say NO to a patch.
I try as much as possible to do (c), but the reality is that
/dev/random is sexiest thing, and to be honest, I suspect that there
are many more sources of vulnerability which are easier for an
attacker than attacking the random number generator. So it may in
fact be _rational_ for people who are working on hardening the kernel
to focus on other areas. That being said, we should be trying to
improve things on all fronts, not just the sexy ones.
> Ted about this, I proposed instead a more global approach of
> introducing an rng_init() to complement things like late_init() and
> device_init() and such. The idea here would be two-fold:
>
> - Modules that are built in would only be loaded as a callback to the
> initialization of the RNG. An API for that already exists.
> - Modules that are external would simply block userspace in
> request_module until the RNG is initialized. This patch series adds
> that kind of API.
>
> If I understood correctly, Ted was worried that this might introduce
> some headaches with module load ordering.
My concern is while it might work on one architecture, it would break
on another architecture. And even on one architecture, it might be
that it works on bare metal hardware, but on in a virtual environment,
there aren't enough interrupts for us to fully initialize the CRNG.
So it might be that Fedora with its kernel config file work fine in one area, but
it mysteriously fails if you install Fedora in a VM --- and worse,
maybe it works in Cloud Platform A, but not Cloud Platform B. (And
then the rumor mongers will come out and claim that the failure on one
Cloud Platform is due to the fact that some set of enigneers work for
one company versus another... not that we've seen any kind of rants
like that on the kernel-hardening mailing list! :-)
I think this is a soluble problem, but it may be rather tricky. For
example, it may be that for a certain class of init calls, even though
they are in subsystems that are compiled into the kernel, those init
calls perhaps could be deferred so they are running in parallel with
the init scripts. (Or maybe we could just require that certain kernel
modules can *only* be compiled as modules if they use rng_init ---
although that may get annoying for those of us who like being able to
build custom configured monolithic kernels. So I'd prefer the first
possibility if at all possible.)
Cheers,
- Ted
Hi Jason,
On Tue, Jun 06, 2017 at 05:23:04PM +0200, Jason A. Donenfeld wrote:
> Hey again Eric,
>
> One thing led to another and I wound up just rewriting all the crypto
> in big_keys.c. I'll include this for v4:
>
> https://git.zx2c4.com/linux-dev/commit/?h=jd/rng-blocker&id=886ff283b9808aecb14aa8e397da8496a9635aed
>
> Not only was the use of crypto/rng inappropriate, but the decision to
> go with aes-ecb is shocking. Seeing that this author had no other
> commits in the tree, and that all subsequent commits that mentioned
> his name were cleaning up his mess, I just went ahead and removed both
> the crypto/rng misusage and changed from aes-ecb to aes-gcm.
>
> Anyway, I'll wait for some more reviews on v3, and then this can be
> reviewed for v4.
>
> Regards,
> Jason
I agree that the use of ECB mode in big_key is broken, and thanks for trying to
fix it! I think using GCM is good, but please leave a very conspicuous comment
where the nonce is being set to 0, noting that it's safe only because a unique
key is used to encrypt every big_key *and* the big_keys are not updatable (via
an .update method in the key_type), resulting in each GCM key being used for
only a single encryption.
Also, I think you should send this to the keyrings mailing list and maintainer
so it can be discussed and merged separately from your RNG changes.
Eric
On Tue, Jun 6, 2017 at 7:03 PM, Theodore Ts'o <[email protected]> wrote:
> So it's not clear what you mean by Stephan's work.
I just meant that there's a guy out there who seems really motivated
to work on this stuff in detail, but hasn't seen too much love, AFAIK.
I'm sure there's an interesting technical discussion to be had about
his contributions.
> The reality though is that Linux is a volunteer effort
Yep! And here I am volunteering, writing some code, in my free time,
just for willies. I hope you'll be a kind, helpful, and supportive
maintainer who welcomes contributions and discussion.
> So it may in
> fact be _rational_ for people who are working on hardening the kernel
> to focus on other areas.
> improve things on all fronts, not just the sexy ones.
I don't want people to use some aspects of a module I'm writing before
get_random_bytes() will return good randomness. You made some point
about what's sexy and what isn't and what is rational for people to
work on, but I think I missed the thrust of it. I'm working on this
particular problem now with get_random_bytes, as something motivated
by simple considerations, not complex ulterior motives.
But anyway, moving on...
> I think this is a soluble problem, but it may be rather tricky. For
> example, it may be that for a certain class of init calls, even though
> they are in subsystems that are compiled into the kernel, those init
> calls perhaps could be deferred so they are running in parallel with
> the init scripts. (Or maybe we could just require that certain kernel
> modules can *only* be compiled as modules if they use rng_init ---
> although that may get annoying for those of us who like being able to
> build custom configured monolithic kernels. So I'd prefer the first
> possibility if at all possible.)
Right, indeed this is tricky, and you bring up good points about
complex interactions on different architectures. Based on your
comments about whack-a-mole, I think we're mostly on the same page
about how arduous this is going to be to fix. My plan is to first get
in this simple blocking API, because while it doesn't solve _every_
use case, there are particular use cases that are helped nearly
perfectly by it. Namely, places where userspace is going to configure
something. So, soon after I finish up this testing I've been doing all
day on my series, I'll post v4, and hopefully we can get that merged,
and then move onto interesting other solutions for the general
problem.
Regards,
Jason
On Tue, Jun 6, 2017 at 7:26 PM, Eric Biggers <[email protected]> wrote:
> I agree that the use of ECB mode in big_key is broken, and thanks for trying to
> fix it! I think using GCM is good, but please leave a very conspicuous comment
> where the nonce is being set to 0, noting that it's safe only because a unique
> key is used to encrypt every big_key *and* the big_keys are not updatable (via
> an .update method in the key_type), resulting in each GCM key being used for
> only a single encryption.
Good idea. I'll make a large comment about this.
>
> Also, I think you should send this to the keyrings mailing list and maintainer
> so it can be discussed and merged separately from your RNG changes.
Yea, that seems like a good idea. I'll separate things out right now.
Am Dienstag, 6. Juni 2017, 19:03:19 CEST schrieb Theodore Ts'o:
Hi Theodore,
> On Tue, Jun 06, 2017 at 02:34:43PM +0200, Jason A. Donenfeld wrote:
> > Yes, I agree whole-heartedly. A lot of people have proposals for
> > fixing the direct idea of entropy gathering, but for whatever reason,
> > Ted hasn't merged stuff. I think Stephan (CCd) rewrote big critical
> > sections of the RNG, called LRNG, and published a big paper for peer
> > review and did a lot of cool engineering, but for some reason this
> > hasn't been integrated. I look forward to movement on this front in
> > the future, if it ever happens. Would be great.
>
> So it's not clear what you mean by Stephan's work. It can be
> separated into multiple pieces; one is simply using a mechanism which
> can be directly mapped to NIST's DRBG framework. I don't believe this
> actually adds any real security per se, but it can make it easier to
> get certification for people who care about getting FIPS
> certification. Since I've seen a lot of snake oil and massive waste
> of taxpayer and industry dollars by FIPS certification firms, it's not
> a thing I particularly find particularly compelling.
>
> The second bit is "Jitter Entropy". The problem I have with that is
> there isn't any convincing explanation about why it can't be predicted
> to some degree of accuracy with someone who understands what's going
> on with Intel's cache architecture. (And this isn't just me, I've
> talked to people who work at Intel and they are at best skeptical of
> the whole idea.)
My LRNG approach covers many more concerns rather than just using the Jitter
RNG or using the DRBG. Using the Jitter RNG should just beef up the lacking
entropy at boot time. Irrespective of what you think of it, it will not
destroy existing entropy. Using the DRBG should allow crypto offloading and
provides a small API for other users to plug in their favorite DRNG (like the
ChaCha20 DRNG).
I think I mentioned several times already which are the core concerns I have.
But allow me to re-iterate them again as I have not seen any answer so far:
- There is per definition a high correlation between interrupts and HID/block
device events. The legacy /dev/random by far weights HID/block device noise
higher in entropy than interrupts and awards interrupts hardly any entropy.
But let us face it, HID and block devices are just a "derivative" of
interrupts. Instead of weighting HID/block devices higher than interrupts, we
should get rid of them when counting entropy and focus on interrupts.
Interrupts fare very well even in virtualized environments where the legacy /
dev/random hardly collects any entropy. Note, this interrupt behavior in
virtual environments was the core motivation for developing the LRNG.
- By not having such collision problems and the related low validations of
entropy from interrupts, a much faster initialization with sufficient entropy
is possible. This is now visible with the current initialization of the
ChaCha20 part of the legacy /dev/random. That comes, however, at the cost that
HID/disk events happening before the ChaCha20 is initialized are affected by
the aforementioned correlation. Just to say it again, correlation destroys
entropy.
- The entropy estimate is based on first, second and third derivative of
Jiffies. As Jiffies hardly contribute any entropy per event, using this number
for an entropy estimation for an event is just coincidence that the legacy /
dev/random underestimates entropy. And then using such coincidential estimates
to apply an asymptotic calculation how much the entropy estimator is
increased, is not really helpful.
- The entropy transport within the legacy /dev/random allows small quantums
(down to 8 bit minimums) of entropy to be transported. Such approach is a
concern which can be illustrated with a pathological analogy (I understand
that this pathological case is not present for the legacy /dev/random, but it
illustrates the problem with small quantities of entropy). Assume that only
one bit of entropy is conveyed from input_pool to blocking_pool during each
read operation by an attacker from /dev/random (and assume that the attacker
an read that one bit). Now, if 128 bits of entropy are transported with 128
individual transactions where the attacker can read data from the RNG between
each transport, the final crypto strength is only 2 * 128 bits and not 2^128
bits. Thus, transports of entropy should be done in larger quantities (like
128 bits at least).
- The DRNGs are fully testable by itself. The DRBG is tested using the kernel
crypto API's testmgr using blessed test vectors. The ChaCha20 DRNG is
implemented such that it can be extracted in a user space app to study it
further (such extraction of the ChaCha20 into a standalone DRNG is provided at
[1]).
I tried to address those issues in the LRNG.
Finally, I am very surprised that I get hardly any answers on patches to
random.c let alone that any changes to random.c will be applied at all.
Lastly, it is very easy to call an approach (JitterRNG) flawed, but I would
like to see some back-up of such claim after the analysis that is provided on
that topic. This analysis refers to the wait states between individual CPU
components as the root of the noise knowing that the ISA != the hardware CPU
instruction set and the evidence collected on various different CPUs.
[1] https://github.com/smuellerDD/chacha20_drng
Ciao
Stephan
On Tue, Jun 6, 2017 at 7:57 PM, Stephan Müller <[email protected]> wrote:
> Finally, I am very surprised that I get hardly any answers on patches to
> random.c let alone that any changes to random.c will be applied at all.
FWIW, this is my biggest concern too. You seem willing to work on this
difficult problem. I'm simultaneously working on a different but
related issue. I hope we're enabled to contribute.
On Tue, 06 Jun 2017, Theodore Ts'o wrote:
> It might be possible, for example, to store a cryptographic key in a
> UEFI boot-services variable, where the key becomes inaccessible after
> the boot-time services terminate. But you also need either a reliable
> time-of-day clock, or a reliable counter which is incremented each
> time the system that boots, and which can't be messed with by an
> attacker, or trivially reset by a clueless user/sysadmin.
>
> Or maybe we can have a script that is run at shutdown and boot-up that
> stashes 32 bytes of entropy in a reserved space accessible to GRUB,
> and which GRUB then passes to the kernel using an extension to the
> Linux/x86 Boot Protocol. (See Documentation/x86/boot.txt)
On that same idea, one could add an early_initramfs handler for entropy
data.
One could also ensure the kernel command line is used to feed some
entropy for the CRNG init (for all I know, this is already being done),
and add a do-nothing parameter (that gets sanitized away after use) that
can be used to add entropy to that command line. Something like
random.someentropy=<bootloader-supplied random stuff goes here>. This
might be more generic than the x86 boot protocol reserved space...
On the better bootloaders, an initramfs segment can be loaded
independently (and you can have as many as required), which makes an
early_initramfs a more palatable vector to inject large amounts of
entropy into the next boot than, say, modifying the kernel image
directly at every boot/shutdown to stash entropy in there somewhere.
These are all easy to implement, I just don't know how *useful* they
would really be.
--
Henrique Holschuh
On Tue, Jun 06, 2017 at 07:19:10PM -0300, Henrique de Moraes Holschuh wrote:
> On that same idea, one could add an early_initramfs handler for entropy
> data.
>
> One could also ensure the kernel command line is used to feed some
> entropy for the CRNG init (for all I know, this is already being done),
> and add a do-nothing parameter (that gets sanitized away after use) that
> can be used to add entropy to that command line. Something like
> random.someentropy=<bootloader-supplied random stuff goes here>. This
> might be more generic than the x86 boot protocol reserved space...
>
> On the better bootloaders, an initramfs segment can be loaded
> independently (and you can have as many as required), which makes an
> early_initramfs a more palatable vector to inject large amounts of
> entropy into the next boot than, say, modifying the kernel image
> directly at every boot/shutdown to stash entropy in there somewhere.
>
> These are all easy to implement, I just don't know how *useful* they
> would really be.
+1
The kernel side for all of these are relatively easy. The hard part
is to do an end-to-end solution. Which means the changes to the
bootloader, the initramfs tools, etc.
As I recall one issue with doing things in the initramfs scripts is
that for certain uses of randomness (such as the stack canary), it's
hard to change the valid canary after it's been used for a userspace
process, since you might have to support more than one valid canary
value until all of the proceses using the original (not necessarily
cryptographically initialized) stack canary has exited. So while the
upside is that it might not require any kernel changes to inject the
randomness into the non-blocking pool via one of the initramfs
scripts, from an overall simplicity for the kernel users, it's nice if
we can initialize the CRNG as early possible --- in the ideal world,
even before KASLR has been initialized, which means ****really****
early in the boot process.
That's the advantage of doing it as part of the Linux/x86 boot
protocol, since it's super simple to get at the entropy seed. It
doesn't require parsing the kernel command-line. The tradeoff is that
it is x86 specific, and the ARM, ppcle folks, etc. would have to
implement their own way of injecting entropy super-early into the boot
process.
One advantage of doing this somewhat harder thing is that **all** of
the issues around re-architecting a new rng_init initcall level, and
dealing with module load order, etc., disappear if we can figure out a
way to initialize the entropy pre-KASLR. Yes, it's harder; but it
solves all of the issues at one fell swoop.
- Ted
Am Mittwoch, 7. Juni 2017, 00:19:10 CEST schrieb Henrique de Moraes Holschuh:
Hi Henrique,
> On that same idea, one could add an early_initramfs handler for entropy
> data.
Any data that comes from outside during the boot process, be it some NVRAM
location, the /var/lib...seed file for /dev/random or other approaches are
viewed by a number of folks to have zero bits of entropy.
I.e. this data is nice for stirring the pool, but is not considered to help
our entropy problem.
Ciao
Stephan
On Wed, 07 Jun 2017, Stephan M?ller wrote:
> Am Mittwoch, 7. Juni 2017, 00:19:10 CEST schrieb Henrique de Moraes Holschuh:
> > On that same idea, one could add an early_initramfs handler for entropy
> > data.
>
> Any data that comes from outside during the boot process, be it some NVRAM
> location, the /var/lib...seed file for /dev/random or other approaches are
> viewed by a number of folks to have zero bits of entropy.
>
> I.e. this data is nice for stirring the pool, but is not considered to help
> our entropy problem.
Stirring the pool is actually the objective, not entropy accounting.
Let's not lose sight of what is really important.
But yes, you are quite correct in that it won't help anything that would
like to block due to entropy accouting, or which needs to be certain
about the amount of randomness in the pools.
--
Henrique Holschuh
> On the better bootloaders, an initramfs segment can be loaded
> independently (and you can have as many as required), which makes an
> early_initramfs a more palatable vector to inject large amounts of
> entropy into the next boot than, say, modifying the kernel image
> directly at every boot/shutdown to stash entropy in there somewhere.
Modifying the kernel image on storage isn't compatible with verified
boot so it's not really a solution. The kernel, initrd and rest of the
OS are signed and verified on operating systems like Android, Android
Things, ChromeOS and many embedded devices, etc. putting some basic
effort into security. I didn't really understand the device tree
approach and mentioned a few times before. Passing via the kernel
cmdline is a lot simpler than modifying the device tree in-memory and
persistent modification isn't an option unless verified boot is missing
anyway.
On Wed, Jun 07, 2017 at 01:00:25PM -0400, Daniel Micay wrote:
> > On the better bootloaders, an initramfs segment can be loaded
> > independently (and you can have as many as required), which makes an
> > early_initramfs a more palatable vector to inject large amounts of
> > entropy into the next boot than, say, modifying the kernel image
> > directly at every boot/shutdown to stash entropy in there somewhere.
[...]
> I didn't really understand the device tree approach and mentioned a
> few times before. Passing via the kernel cmdline is a lot simpler than
> modifying the device tree in-memory and persistent modification isn't
> an option unless verified boot is missing anyway.
I might be missing something here, but the command line is inside of the
device tree, at /chosen/bootargs, so modifying the kernel command line
*is* modifying the device tree in-memory.
For arm64, we have a /chosen/kaslr-seed property that we hope
FW/bootloaders fill in, and similar could be done for some initial
entropy, provided appropriate HW/FW support.
Thanks,
Mark.
On Tue, Jun 06, 2017 at 01:03:19PM -0400, Theodore Ts'o wrote:
> The other approach is to find a way to have initialized "seed" entropy
> which we can count on at every boot. The problem is that this is very
> much dependent on how the bootloader works. It's easy to say "store
> it in the kernel", but where the kernel is stored varies greatly from
> architecture to architecture. In some cases, the kernel can stored in
> ROM, where it can't be modified at all.
>
> It might be possible, for example, to store a cryptographic key in a
> UEFI boot-services variable, where the key becomes inaccessible after
> the boot-time services terminate. But you also need either a reliable
> time-of-day clock, or a reliable counter which is incremented each
> time the system that boots, and which can't be messed with by an
> attacker, or trivially reset by a clueless user/sysadmin.
FWIW, EFI has an (optional) EFI_RNG_PROTOCOL, that we currently use to
seed the kernel's entropy pool. The EFI stub creates a config table with
the entropy, which the kernel reads.
This is re-seeded prior to kexec() to avoid the entropy being recycled.
See commits:
636259880a7e7d34 ("efi: Add support for seeding the RNG from a UEFI config table")
568bc4e87033d232 (" efi/arm*/libstub: Invoke EFI_RNG_PROTOCOL to seed the UEFI RNG table")
Unfortunately, I beleive that support for the protocol is currently rare
in practice.
Thanks,
Mark.
Hi Jason,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc4]
[cannot apply to next-20170607]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jason-A-Donenfeld/Unseeded-In-Kernel-Randomness-Fixes/20170606-171249
config: sh-ul2_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sh
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
In file included from include/linux/spinlock.h:53:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:10,
from include/linux/pid.h:4,
from include/linux/sched.h:13,
from include/linux/utsname.h:5,
from drivers//char/random.c:238:
drivers//char/random.c: In function 'get_random_u64':
>> include/linux/irqflags.h:69:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
arch_local_irq_restore(flags); \
^~~~~~~~~~~~~~~~~~~~~~
drivers//char/random.c:2063:16: note: 'flags' was declared here
unsigned long flags;
^~~~~
In file included from include/linux/spinlock.h:53:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:10,
from include/linux/pid.h:4,
from include/linux/sched.h:13,
from include/linux/utsname.h:5,
from drivers//char/random.c:238:
drivers//char/random.c: In function 'get_random_u32':
>> include/linux/irqflags.h:69:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
arch_local_irq_restore(flags); \
^~~~~~~~~~~~~~~~~~~~~~
drivers//char/random.c:2095:16: note: 'flags' was declared here
unsigned long flags;
^~~~~
--
In file included from include/linux/spinlock.h:53:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:10,
from include/linux/pid.h:4,
from include/linux/sched.h:13,
from include/linux/utsname.h:5,
from drivers/char/random.c:238:
drivers/char/random.c: In function 'get_random_u64':
>> include/linux/irqflags.h:69:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
arch_local_irq_restore(flags); \
^~~~~~~~~~~~~~~~~~~~~~
drivers/char/random.c:2063:16: note: 'flags' was declared here
unsigned long flags;
^~~~~
In file included from include/linux/spinlock.h:53:0,
from include/linux/rcupdate.h:38,
from include/linux/rculist.h:10,
from include/linux/pid.h:4,
from include/linux/sched.h:13,
from include/linux/utsname.h:5,
from drivers/char/random.c:238:
drivers/char/random.c: In function 'get_random_u32':
>> include/linux/irqflags.h:69:3: warning: 'flags' may be used uninitialized in this function [-Wmaybe-uninitialized]
arch_local_irq_restore(flags); \
^~~~~~~~~~~~~~~~~~~~~~
drivers/char/random.c:2095:16: note: 'flags' was declared here
unsigned long flags;
^~~~~
vim +/flags +69 include/linux/irqflags.h
81d68a96a Steven Rostedt 2008-05-12 53 # define start_critical_timings() do { } while (0)
81d68a96a Steven Rostedt 2008-05-12 54 #endif
81d68a96a Steven Rostedt 2008-05-12 55
df9ee2927 David Howells 2010-10-07 56 /*
df9ee2927 David Howells 2010-10-07 57 * Wrap the arch provided IRQ routines to provide appropriate checks.
df9ee2927 David Howells 2010-10-07 58 */
df9ee2927 David Howells 2010-10-07 59 #define raw_local_irq_disable() arch_local_irq_disable()
df9ee2927 David Howells 2010-10-07 60 #define raw_local_irq_enable() arch_local_irq_enable()
df9ee2927 David Howells 2010-10-07 61 #define raw_local_irq_save(flags) \
df9ee2927 David Howells 2010-10-07 62 do { \
df9ee2927 David Howells 2010-10-07 63 typecheck(unsigned long, flags); \
df9ee2927 David Howells 2010-10-07 64 flags = arch_local_irq_save(); \
df9ee2927 David Howells 2010-10-07 65 } while (0)
df9ee2927 David Howells 2010-10-07 66 #define raw_local_irq_restore(flags) \
df9ee2927 David Howells 2010-10-07 67 do { \
df9ee2927 David Howells 2010-10-07 68 typecheck(unsigned long, flags); \
df9ee2927 David Howells 2010-10-07 @69 arch_local_irq_restore(flags); \
df9ee2927 David Howells 2010-10-07 70 } while (0)
df9ee2927 David Howells 2010-10-07 71 #define raw_local_save_flags(flags) \
df9ee2927 David Howells 2010-10-07 72 do { \
df9ee2927 David Howells 2010-10-07 73 typecheck(unsigned long, flags); \
df9ee2927 David Howells 2010-10-07 74 flags = arch_local_save_flags(); \
df9ee2927 David Howells 2010-10-07 75 } while (0)
df9ee2927 David Howells 2010-10-07 76 #define raw_irqs_disabled_flags(flags) \
df9ee2927 David Howells 2010-10-07 77 ({ \
:::::: The code at line 69 was first introduced by commit
:::::: df9ee29270c11dba7d0fe0b83ce47a4d8e8d2101 Fix IRQ flag handling naming
:::::: TO: David Howells <[email protected]>
:::::: CC: David Howells <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Strange, not all compilers do this warning. Fixing with:
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 12758db..5252690 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2061,8 +2061,8 @@ static DEFINE_PER_CPU(struct batched_entropy,
batched_entropy_u64);
u64 get_random_u64(void)
{
u64 ret;
- bool use_lock = crng_init < 2;
- unsigned long flags;
+ const bool use_lock = READ_ONCE(crng_init) < 2;
+ unsigned long flags = 0;
struct batched_entropy *batch;
#if BITS_PER_LONG == 64
@@ -2099,8 +2099,8 @@ static DEFINE_PER_CPU(struct batched_entropy,
batched_entropy_u32);
u32 get_random_u32(void)
{
u32 ret;
- bool use_lock = crng_init < 2;
- unsigned long flags;
+ const bool use_lock = READ_ONCE(crng_init) < 2;
+ unsigned long flags = 0;
struct batched_entropy *batch;
if (arch_get_random_int(&ret))
Const, because it's more correct. READ_ONCE to be certain that the
compiler doesn't try to paste the expression into both uses. And
finally flags=0 to shutup the compiler.
If anybody has a better way of approaching this, feel free to chime in.
On Wed, Jun 07, 2017 at 07:00:17AM +0200, Stephan M?ller wrote:
> > On that same idea, one could add an early_initramfs handler for entropy
> > data.
>
> Any data that comes from outside during the boot process, be it some NVRAM
> location, the /var/lib...seed file for /dev/random or other approaches are
> viewed by a number of folks to have zero bits of entropy.
The Open BSD folks would disagree with you. They've designed their
whole system around saving entropy at shutdown, reading it as early as
possible by the bootloader, and then as soon as possible after the
reboot, to overwrite and reinitialize the entropy seed stored on disk
so that on a reboot after a crash. They consider this good enough to
assume that their CRNG is *always* strongly initialized.
I'll let you have that discussion/argument with Theo de Raadt, though.
Be warned that he has opinions about security that are almost
certainly as strong (and held with the same level of certainty) as a
certain Brad Spengler...
- Ted
On Wed, 2017-06-07 at 18:26 +0100, Mark Rutland wrote:
> On Wed, Jun 07, 2017 at 01:00:25PM -0400, Daniel Micay wrote:
> > > On the better bootloaders, an initramfs segment can be loaded
> > > independently (and you can have as many as required), which makes
> > > an
> > > early_initramfs a more palatable vector to inject large amounts of
> > > entropy into the next boot than, say, modifying the kernel image
> > > directly at every boot/shutdown to stash entropy in there
> > > somewhere.
>
> [...]
>
> > I didn't really understand the device tree approach and mentioned a
> > few times before. Passing via the kernel cmdline is a lot simpler
> > than
> > modifying the device tree in-memory and persistent modification
> > isn't
> > an option unless verified boot is missing anyway.
>
> I might be missing something here, but the command line is inside of
> the
> device tree, at /chosen/bootargs, so modifying the kernel command line
> *is* modifying the device tree in-memory.
>
> For arm64, we have a /chosen/kaslr-seed property that we hope
> FW/bootloaders fill in, and similar could be done for some initial
> entropy, provided appropriate HW/FW support.
>
> Thanks,
> Mark.
I was assuming it was simpler since bootloaders are already setting it,
but it seems I'm wrong about that.
On Tue, Jun 06, 2017 at 05:56:20AM +0200, Jason A. Donenfeld wrote:
> Hey Ted,
>
> On Tue, Jun 6, 2017 at 5:00 AM, Theodore Ts'o <[email protected]> wrote:
> > Note that crypto_rng_reset() is called by big_key_init() in
> > security/keys/big_key.c as a late_initcall(). So if we are on a
> > system where the crng doesn't get initialized until during the system
> > boot scripts, and big_key is compiled directly into the kernel, the
> > boot could end up deadlocking.
> >
> > There may be other instances of where crypto_rng_reset() is called by
> > an initcall, so big_key_init() may not be an exhaustive enumeration of
> > potential problems. But this is an example of why the synchronous
> > API, although definitely much more convenient, can end up being a trap
> > for the unwary....
>
> Thanks for pointing this out. I'll look more closely into it and see
> if I can figure out a good way of approaching this.
Would it work for wait_for_random_bytes() to include a
WARN_ON(system_state < SYSTEM_RUNNING);
to catch those kinds of cases?
- Kevin