Re: [PATCH] tcp: efficient port randomisation (revised)

[Michael Vittrup Larsen <michael.vittrup.larsen@ericsson.com>]

I have looked through this patch and found no problems - thank you for 
implementing the draft.

/Michael

On Thursday 18 November 2004 00:30, Stephen Hemminger wrote:
> Here is a more conservative version of earlier patch vthat keeps the same
> port rover locking and global port rover. This randomizes TCP ephemeral
> ports of incoming connections using variation of existing sequence number
> hash.
>
> Thanks to original author Michael Larsen.
> http://www.ietf.org/internet-drafts/draft-larsen-tsvwg-port-randomisation-0
>0.txt
>
> It behaves correctly if someone is perverse and sets low > high
> and it separates the outgoing port rover (tcp_port_rover) from the incoming
> port rover (start_rover).
>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
>
> diff -Nru a/drivers/char/random.c b/drivers/char/random.c
> --- a/drivers/char/random.c 2004-11-17 15:21:43 -08:00
> +++ b/drivers/char/random.c 2004-11-17 15:21:43 -08:00
> @@ -2347,6 +2347,24 @@
>   return halfMD4Transform(hash, keyptr->secret);
>  }
>
> +/* Generate secure starting point for ephemeral TCP port search */
> +u32 secure_tcp_port_ephemeral(__u32 saddr, __u32 daddr, __u16 dport)
> +{
> + struct keydata *keyptr = get_keyptr();
> + u32 hash[4];
> +
> + /*
> +  *  Pick a unique starting offset for each ephemeral port search
> +  *  (saddr, daddr, dport) and 48bits of random data.
> +  */
> + hash[0] = saddr;
> + hash[1] = daddr;
> + hash[2] = dport ^ keyptr->secret[10];
> + hash[3] = keyptr->secret[11];
> +
> + return halfMD4Transform(hash, keyptr->secret);
> +}
> +
>  #ifdef CONFIG_SYN_COOKIES
>  /*
>   * Secure SYN cookie computation. This is the algorithm worked out by
> diff -Nru a/include/linux/random.h b/include/linux/random.h
> --- a/include/linux/random.h 2004-11-17 15:21:43 -08:00
> +++ b/include/linux/random.h 2004-11-17 15:21:43 -08:00
> @@ -52,6 +52,7 @@
>  void generate_random_uuid(unsigned char uuid_out[16]);
>
>  extern __u32 secure_ip_id(__u32 daddr);
> +extern u32 secure_tcp_port_ephemeral(__u32 saddr, __u32 daddr, __u16
> dport); extern __u32 secure_tcp_sequence_number(__u32 saddr, __u32 daddr,
> __u16 sport, __u16 dport);
>  extern __u32 secure_tcp_syn_cookie(__u32 saddr, __u32 daddr,
> diff -Nru a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> --- a/net/ipv4/tcp_ipv4.c 2004-11-17 15:21:43 -08:00
> +++ b/net/ipv4/tcp_ipv4.c 2004-11-17 15:21:43 -08:00
> @@ -636,6 +636,13 @@
>   return -EADDRNOTAVAIL;
>  }
>
> +static inline u32 connect_port_offset(const struct sock *sk)
> +{
> + const struct inet_opt *inet = inet_sk(sk);
> + return secure_tcp_port_ephemeral(inet->rcv_saddr, inet->daddr,
> +      inet->dport);
> +}
> +
>  /*
>   * Bind a port for a connect operation and hash it.
>   */
> @@ -647,10 +654,12 @@
>   int ret;
>
>    if (!snum) {
> -   int rover;
> -   int low = sysctl_local_port_range[0];
> -   int high = sysctl_local_port_range[1];
> -   int remaining = (high - low) + 1;
> +   const u16 low = sysctl_local_port_range[0];
> +   const u16 high = sysctl_local_port_range[1];
> +   u16 rover = low;
> +   int remaining = (high-low) + 1;
> +  u32 offset = connect_port_offset(sk);
> +  static u32 rover_start;
>    struct hlist_node *node;
>     struct tcp_tw_bucket *tw = NULL;
>
> @@ -660,7 +669,7 @@
>      * tcp_portalloc_lock before next submission to Linus.
>      * As soon as we touch this place at all it is time to think.
>      *
> -    * Now it protects single _advisory_ variable tcp_port_rover,
> +    * Now it protects single _advisory_ variable rover_start,
>      * hence it is mostly useless.
>      * Code will work nicely if we just delete it, but
>      * I am afraid in contented case it will work not better or
> @@ -670,12 +679,9 @@
>      * memory pingpong. Any ideas how to do this in a nice way?
>      */
>     spin_lock(&tcp_portalloc_lock);
> -   rover = tcp_port_rover;
> -
> -   do {
> -    rover++;
> -    if ((rover < low) || (rover > high))
> -     rover = low;
> +  while (remaining > 0) {
> +   rover = low + (rover_start + offset) %
> +    (high - low);
>      head = &tcp_bhash[tcp_bhashfn(rover)];
>      spin_lock(&head->lock);
>
> @@ -706,8 +712,9 @@
>
>     next_port:
>      spin_unlock(&head->lock);
> -   } while (--remaining > 0);
> -   tcp_port_rover = rover;
> +   --remaining;
> +   ++rover_start;
> +   }
>     spin_unlock(&tcp_portalloc_lock);
>
>     local_bh_enable();
> @@ -716,7 +723,6 @@
>
>  ok:
>     /* All locks still held and bhs disabled */
> -   tcp_port_rover = rover;
>     spin_unlock(&tcp_portalloc_lock);
>
>     tcp_bind_hash(sk, tb, rover);