Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660632 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use Theodore Ts'o <tytso@mit.edu> - 2017-06-08 02:40 +0200
    Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 03:00 +0200
      Re: [PATCH v4 04/13] security/keys: ensure RNG is seeded before use "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-08 03:10 +0200

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

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(&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;

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

[toc] | [next] | [standalone]

#1660639

On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o <tytso@mit.edu> 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?

[toc] | [prev] | [next] | [standalone]

#1660641

On Thu, Jun 8, 2017 at 2:50 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, Jun 8, 2017 at 2:31 AM, Theodore Ts'o <tytso@mit.edu> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web