Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

2017-06-07 Thread Jason A. Donenfeld
On Thu, Jun 8, 2017 at 2:50 AM, Jason A. Donenfeld  wrote:
> On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o  wrote:
>> I'm guessing you changed key_alloc_serial() to return an int back when
>> you were thinking that you might use get_random_bytes_wait(), which
>> could return -ERESTARTSYS.
>>
>> Now that you're not doing this, but using get_random_u32() instead,
>> there's no point to change the function signature of
>> key_alloc_serial() and add an error check in key_alloc() that will
>> never fail, right?  That's just adding a dead code path.  Which the
>> compiler can probably optimize away, but why make the code slightly
>> harder to read than necessasry?
>
> Good catch, and thanks for reading these so thoroughly that you caught
> the churn artifacts. Do you want me to clean this up and resubmit, or
> are you planning on adjusting it in the dev branch?

Fixed it up here if you just want to grab this instead:

https://git.kernel.org/pub/scm/linux/kernel/git/zx2c4/linux.git/patch/?id=a0361e55bce30ace529ed8b28bd452e3ac0ee91f


Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

2017-06-07 Thread Jason A. Donenfeld
On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o  wrote:
> I'm guessing you changed key_alloc_serial() to return an int back when
> you were thinking that you might use get_random_bytes_wait(), which
> could return -ERESTARTSYS.
>
> Now that you're not doing this, but using get_random_u32() instead,
> there's no point to change the function signature of
> key_alloc_serial() and add an error check in key_alloc() that will
> never fail, right?  That's just adding a dead code path.  Which the
> compiler can probably optimize away, but why make the code slightly
> harder to read than necessasry?

Good catch, and thanks for reading these so thoroughly that you caught
the churn artifacts. Do you want me to clean this up and resubmit, or
are you planning on adjusting it in the dev branch?


Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use

2017-06-07 Thread Theodore Ts'o
On Tue, Jun 06, 2017 at 07:47:55PM +0200, Jason A. Donenfeld wrote:
> -static inline void key_alloc_serial(struct key *key)
> +static inline int key_alloc_serial(struct key *key)

> @@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
>   rb_insert_color(>serial_node, _serial_tree);
>  
>   spin_unlock(_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(>nkeys);
> - key_alloc_serial(key);
> + ret = key_alloc_serial(key);
> + if (ret < 0)
> + goto security_error;
>  
>  error:
>   return key;

I'm guessing you changed key_alloc_serial() to return an int back when
you were thinking that you might use get_random_bytes_wait(), which
could return -ERESTARTSYS.

Now that you're not doing this, but using get_random_u32() instead,
there's no point to change the function signature of
key_alloc_serial() and add an error check in key_alloc() that will
never fail, right?  That's just adding a dead code path.  Which the
compiler can probably optimize away, but why make the code slightly
harder to read than necessasry?

- Ted


[PATCH v4 04/13] security/keys: ensure RNG is seeded before use

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

Signed-off-by: Jason A. Donenfeld 
Cc: David Howells 
Cc: Mimi Zohar 
Cc: David Safford 
---
 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(>serial, sizeof(key->serial));
-
-   key->serial >>= 1; /* negative numbers are not permitted */
+   key->serial = get_random_u32() >> 1;
} while (key->serial < 3);
 
spin_lock(_serial_lock);
@@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
rb_insert_color(>serial_node, _serial_tree);
 
spin_unlock(_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(>nkeys);
-   key_alloc_serial(key);
+   ret = key_alloc_serial(key);
+   if (ret < 0)
+   goto security_error;
 
 error:
return key;
-- 
2.13.0