Re: [RFC] tcp: randomize TCP source ports

[Daniel Borkmann <dborkman@redhat.com>]

On 11/09/2013 05:47 AM, Hannes Frederic Sowa wrote:
> On Fri, Nov 08, 2013 at 07:11:18AM -0800, Eric Dumazet wrote:
>> On Fri, 2013-11-08 at 15:28 +0100, Hannes Frederic Sowa wrote:
>>
>>> What do you think about using a timer to keep the reseed out of fast-path
>>> and switch to the non-arch get_random_bytes instead?
>>
>> Well, the initial seed value is get_random_bytes(). I felt that using a
>> xor with the _arch() version would be safe enough.
>>
>> For the timer, I do not think its worth the pain : Do you want a per cpu
>> timer, or a global one ?
>
> This untested diff came to my mind (it is based on the random tree). I
> actually consider to propose something like this for 3.13. UDP port
> randomization is really critical.
>
> In 3.14 timeframe I suggest abandon net_random and use prandom_u32
> directly so code gets easier to audit.
>
> Would it hurt to use "proper" get_random_byte calls for port randomization?
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index cdf4cfb..e9d0136 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -657,9 +657,11 @@ retry:
>   	r->entropy_total += nbits;
>   	if (!r->initialized && nbits > 0) {
>   		if (r->entropy_total > 128) {
> -			if (r == &nonblocking_pool)
> +			if (r == &nonblocking_pool) {
>   				pr_notice("random: %s pool is initialized\n",
>   					  r->name);
> +				prandom_reseed();
> +			}
>   			r->initialized = 1;
>   			r->entropy_total = 0;
>   		}
> diff --git a/include/linux/random.h b/include/linux/random.h
> index 6312dd9..4f878c0 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -29,6 +29,7 @@ unsigned long randomize_range(unsigned long start, unsigned long end, unsigned l
>   u32 prandom_u32(void);
>   void prandom_bytes(void *buf, int nbytes);
>   void prandom_seed(u32 seed);
> +void prandom_reseed(void);
>
>   u32 prandom_u32_state(struct rnd_state *);
>   void prandom_bytes_state(struct rnd_state *state, void *buf, int nbytes);
> diff --git a/lib/random32.c b/lib/random32.c
> index 52280d5..1ee611f 100644
> --- a/lib/random32.c
> +++ b/lib/random32.c
> @@ -174,11 +174,31 @@ static int __init prandom_init(void)
>   }
>   core_initcall(prandom_init);
>
> +static void __prandom_timer(unsigned long dontcare);
> +static DEFINE_TIMER(seed_timer, __prandom_timer, 0, 0);
> +
> +static void __prandom_timer(unsigned long dontcare)
> +{
> +	u32 entropy;
> +	get_random_bytes(&entropy, sizeof(entropy));
> +	prandom_seed(entropy);
> +	seed_timer.expires = jiffies + 60 * HZ;
> +	add_timer(&seed_timer);
> +}
> +
> +static int prandom_start_seed_timer(void)

       ^^^^^^ __init

> +{
	prandom_reseed();

What are the objectives against initializing prandom here in
the late initcall [instead of doing so in drivers/char/random.c]
as it was the case before?

Probably for security reasons, I think you actually don't want
anyone (incl. external 3rd party modules) to call prandom_reseed()
again after this has been done once initially. So I think it
would be better to make this function not visible to anyone
outside of random32.c.

> +	seed_timer.expires = jiffies + 60 * HZ;
> +	add_timer(&seed_timer);
> +	return 0;
> +}
> +late_initcall(prandom_start_seed_timer);
> +
>   /*
>    *	Generate better values after random number generator
>    *	is fully initialized.
>    */
> -static int __init prandom_reseed(void)
> +void prandom_reseed(void)
>   {
>   	int i;
>
> @@ -196,4 +216,3 @@ static int __init prandom_reseed(void)
>   	}
>   	return 0;
>   }
> -late_initcall(prandom_reseed);
>
> Greetings,
>
>    Hannes