* (unknown), @ 2012-02-01 0:50 Shawn Lu 2012-02-01 0:50 ` [PATCH] tcp: md5: fix md5 RST when both sides have listener Shawn Lu 0 siblings, 1 reply; 5+ messages in thread From: Shawn Lu @ 2012-02-01 0:50 UTC (permalink / raw) To: eric.dumazet; +Cc: davem, netdev, xiaoclu From: Shawn Lu <shawn.lu@ericsson.com> Subject: [PATCH] tcp: md5: fix md5 RST when both sides have listener In-Reply-To: RE: [PATCH] tcp: md5: fix md5 RST when both sides have listener ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] tcp: md5: fix md5 RST when both sides have listener 2012-02-01 0:50 (unknown), Shawn Lu @ 2012-02-01 0:50 ` Shawn Lu 2012-02-01 4:08 ` Eric Dumazet 2012-02-01 4:45 ` [PATCH net-next] tcp: md5: protects md5sig_info with RCU Eric Dumazet 0 siblings, 2 replies; 5+ messages in thread From: Shawn Lu @ 2012-02-01 0:50 UTC (permalink / raw) To: eric.dumazet; +Cc: davem, netdev, xiaoclu TCP RST mechanism is broken in TCP md5(RFC2385). When connection is gone, md5 key is lost, sending RST without md5 hash is deem to ignored by peer. This can be a problem since RST help protocal like bgp to fast recove from peer crash. In most case, users of tcp md5, such as bgp and ldp, have listeners on both sides. md5 keys for peers are saved in listening socket. When passive side connection is gone, we can still get md5 key from listening socket. When active side of connection is gone, we can try to find listening socket through source port, and then md5 key. we are not loosing sercuriy here: packet is valified checked with md5 hash. No RST is generated if md5 hash doesn't match or no md5 key can be found. Signed-off-by: Shawn Lu <shawn.lu@ericsson.com> --- net/ipv4/tcp_ipv4.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- net/ipv6/tcp_ipv6.c | 43 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index da5d322..1a761c8 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -593,6 +593,10 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) struct ip_reply_arg arg; #ifdef CONFIG_TCP_MD5SIG struct tcp_md5sig_key *key; + const __u8 *hash_location = NULL; + unsigned char newhash[16]; + int genhash; + struct sock *sk1 = NULL; #endif struct net *net; @@ -623,9 +627,36 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) arg.iov[0].iov_len = sizeof(rep.th); #ifdef CONFIG_TCP_MD5SIG - key = sk ? tcp_md5_do_lookup(sk, - (union tcp_md5_addr *)&ip_hdr(skb)->saddr, - AF_INET) : NULL; + hash_location = tcp_parse_md5sig_option(th); + if (!sk && hash_location) { + /* + * active side is lost. Try to find listening socket through + * source port, and then find md5 key through listening socket. + * we are not loose security here: + * Incoming packet is checked with md5 hash with finding key, + * no RST generated if md5 hash doesn't match. + */ + sk1 = __inet_lookup_listener(dev_net(skb_dst(skb)->dev), + &tcp_hashinfo, ip_hdr(skb)->daddr, + ntohs(th->source), inet_iif(skb)); + /* don't send rst if it can't find key */ + if (!sk1) + return; + rcu_read_lock(); + key = tcp_md5_do_lookup(sk1, (union tcp_md5_addr *) + &ip_hdr(skb)->saddr, AF_INET); + if (!key) + goto release_sk1; + + genhash = tcp_v4_md5_hash_skb(newhash, key, NULL, NULL, skb); + if (genhash || memcmp(hash_location, newhash, 16) != 0) + goto release_sk1; + } else { + key = sk ? tcp_md5_do_lookup(sk, (union tcp_md5_addr *) + &ip_hdr(skb)->saddr, + AF_INET) : NULL; + } + if (key) { rep.opt[0] = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) | @@ -653,6 +684,14 @@ static void tcp_v4_send_reset(struct sock *sk, struct sk_buff *skb) TCP_INC_STATS_BH(net, TCP_MIB_OUTSEGS); TCP_INC_STATS_BH(net, TCP_MIB_OUTRSTS); + +#ifdef CONFIG_TCP_MD5SIG +release_sk1: + if (sk1) { + rcu_read_unlock(); + sock_put(sk1); + } +#endif } /* The code following below sending ACKs in SYN-RECV and TIME-WAIT states diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index bec41f9..3b139e2 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -925,6 +925,13 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb) const struct tcphdr *th = tcp_hdr(skb); u32 seq = 0, ack_seq = 0; struct tcp_md5sig_key *key = NULL; +#ifdef CONFIG_TCP_MD5SIG + const __u8 *hash_location = NULL; + struct ipv6hdr *ipv6h = ipv6_hdr(skb); + unsigned char newhash[16]; + int genhash; + struct sock *sk1 = NULL; +#endif if (th->rst) return; @@ -933,8 +940,32 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb) return; #ifdef CONFIG_TCP_MD5SIG - if (sk) - key = tcp_v6_md5_do_lookup(sk, &ipv6_hdr(skb)->saddr); + hash_location = tcp_parse_md5sig_option(th); + if (!sk && hash_location) { + /* + * active side is lost. Try to find listening socket through + * source port, and then find md5 key through listening socket. + * we are not loose security here: + * Incoming packet is checked with md5 hash with finding key, + * no RST generated if md5 hash doesn't match. + */ + sk1 = inet6_lookup_listener(dev_net(skb_dst(skb)->dev), + &tcp_hashinfo, &ipv6h->daddr, + ntohs(th->source), inet6_iif(skb)); + if (!sk1) + return; + + rcu_read_lock(); + key = tcp_v6_md5_do_lookup(sk1, &ipv6h->saddr); + if (!key) + goto release_sk1; + + genhash = tcp_v6_md5_hash_skb(newhash, key, NULL, NULL, skb); + if (genhash || memcmp(hash_location, newhash, 16) != 0) + goto release_sk1; + } else { + key = sk ? tcp_v6_md5_do_lookup(sk, &ipv6h->saddr) : NULL; + } #endif if (th->ack) @@ -944,6 +975,14 @@ static void tcp_v6_send_reset(struct sock *sk, struct sk_buff *skb) (th->doff << 2); tcp_v6_send_response(skb, seq, ack_seq, 0, 0, key, 1, 0); + +#ifdef CONFIG_TCP_MD5SIG +release_sk1: + if (sk1) { + rcu_read_unlock(); + sock_put(sk1); + } +#endif } static void tcp_v6_send_ack(struct sk_buff *skb, u32 seq, u32 ack, u32 win, u32 ts, -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tcp: md5: fix md5 RST when both sides have listener 2012-02-01 0:50 ` [PATCH] tcp: md5: fix md5 RST when both sides have listener Shawn Lu @ 2012-02-01 4:08 ` Eric Dumazet 2012-02-01 4:45 ` [PATCH net-next] tcp: md5: protects md5sig_info with RCU Eric Dumazet 1 sibling, 0 replies; 5+ messages in thread From: Eric Dumazet @ 2012-02-01 4:08 UTC (permalink / raw) To: Shawn Lu; +Cc: davem, netdev, xiaoclu Le mardi 31 janvier 2012 à 16:50 -0800, Shawn Lu a écrit : > TCP RST mechanism is broken in TCP md5(RFC2385). When > connection is gone, md5 key is lost, sending RST > without md5 hash is deem to ignored by peer. This can > be a problem since RST help protocal like bgp to fast > recove from peer crash. > > In most case, users of tcp md5, such as bgp and ldp, > have listeners on both sides. md5 keys for peers are > saved in listening socket. When passive side connection > is gone, we can still get md5 key from listening socket. > When active side of connection is gone, we can try to > find listening socket through source port, and then md5 > key. > we are not loosing sercuriy here: > packet is valified checked with md5 hash. No RST is generated > if md5 hash doesn't match or no md5 key can be found. > > Signed-off-by: Shawn Lu <shawn.lu@ericsson.com> > --- > net/ipv4/tcp_ipv4.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- > net/ipv6/tcp_ipv6.c | 43 +++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 83 insertions(+), 5 deletions(-) > Hmm, ok but using RCU for the md5 lookup (instead of mere sock lock as done today) also means we need to protect struct tcp_md5sig_info itself I send a patch for this in a separate patch first, then David can apply your patch when I add my "Signed-off-by: ..." ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net-next] tcp: md5: protects md5sig_info with RCU 2012-02-01 0:50 ` [PATCH] tcp: md5: fix md5 RST when both sides have listener Shawn Lu 2012-02-01 4:08 ` Eric Dumazet @ 2012-02-01 4:45 ` Eric Dumazet 2012-02-01 7:18 ` David Miller 1 sibling, 1 reply; 5+ messages in thread From: Eric Dumazet @ 2012-02-01 4:45 UTC (permalink / raw) To: Shawn Lu; +Cc: davem, netdev, xiaoclu This patch makes sure we use appropriate memory barriers before publishing tp->md5sig_info, allowing tcp_md5_do_lookup() being used from tcp_v4_send_reset() without holding socket lock (upcoming patch from Shawn Lu) Note we also need to respect rcu grace period before its freeing, since we can free socket without this grace period thanks to SLAB_DESTROY_BY_RCU Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Cc: Shawn Lu <shawn.lu@ericsson.com> --- include/linux/tcp.h | 2 +- include/net/tcp.h | 1 + net/ipv4/tcp_ipv4.c | 32 ++++++++++++++++++++------------ net/ipv6/tcp_ipv6.c | 2 -- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/include/linux/tcp.h b/include/linux/tcp.h index c2025f1..115389e 100644 --- a/include/linux/tcp.h +++ b/include/linux/tcp.h @@ -463,7 +463,7 @@ struct tcp_sock { const struct tcp_sock_af_ops *af_specific; /* TCP MD5 Signature Option information */ - struct tcp_md5sig_info *md5sig_info; + struct tcp_md5sig_info __rcu *md5sig_info; #endif /* When the cookie options are generated and exchanged, then this diff --git a/include/net/tcp.h b/include/net/tcp.h index 10ae4c7..78880ba 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -1150,6 +1150,7 @@ struct tcp_md5sig_key { /* - sock block */ struct tcp_md5sig_info { struct hlist_head head; + struct rcu_head rcu; }; /* - pseudo header */ diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index da5d322..567cca9 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -879,14 +879,18 @@ struct tcp_md5sig_key *tcp_md5_do_lookup(struct sock *sk, struct tcp_md5sig_key *key; struct hlist_node *pos; unsigned int size = sizeof(struct in_addr); + struct tcp_md5sig_info *md5sig; - if (!tp->md5sig_info) + /* caller either holds rcu_read_lock() or socket lock */ + md5sig = rcu_dereference_check(tp->md5sig_info, + sock_owned_by_user(sk)); + if (!md5sig) return NULL; #if IS_ENABLED(CONFIG_IPV6) if (family == AF_INET6) size = sizeof(struct in6_addr); #endif - hlist_for_each_entry_rcu(key, pos, &tp->md5sig_info->head, node) { + hlist_for_each_entry_rcu(key, pos, &md5sig->head, node) { if (key->family != family) continue; if (!memcmp(&key->addr, addr, size)) @@ -932,7 +936,8 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, return 0; } - md5sig = tp->md5sig_info; + md5sig = rcu_dereference_protected(tp->md5sig_info, + sock_owned_by_user(sk)); if (!md5sig) { md5sig = kmalloc(sizeof(*md5sig), gfp); if (!md5sig) @@ -940,7 +945,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, sk_nocaps_add(sk, NETIF_F_GSO_MASK); INIT_HLIST_HEAD(&md5sig->head); - tp->md5sig_info = md5sig; + rcu_assign_pointer(tp->md5sig_info, md5sig); } key = sock_kmalloc(sk, sizeof(*key), gfp); @@ -966,6 +971,7 @@ int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family) { struct tcp_sock *tp = tcp_sk(sk); struct tcp_md5sig_key *key; + struct tcp_md5sig_info *md5sig; key = tcp_md5_do_lookup(sk, (union tcp_md5_addr *)&addr, AF_INET); if (!key) @@ -973,7 +979,9 @@ int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family) hlist_del_rcu(&key->node); atomic_sub(sizeof(*key), &sk->sk_omem_alloc); kfree_rcu(key, rcu); - if (hlist_empty(&tp->md5sig_info->head)) + md5sig = rcu_dereference_protected(tp->md5sig_info, + sock_owned_by_user(sk)); + if (hlist_empty(&md5sig->head)) tcp_free_md5sig_pool(); return 0; } @@ -984,10 +992,13 @@ void tcp_clear_md5_list(struct sock *sk) struct tcp_sock *tp = tcp_sk(sk); struct tcp_md5sig_key *key; struct hlist_node *pos, *n; + struct tcp_md5sig_info *md5sig; - if (!hlist_empty(&tp->md5sig_info->head)) + md5sig = rcu_dereference_protected(tp->md5sig_info, 1); + + if (!hlist_empty(&md5sig->head)) tcp_free_md5sig_pool(); - hlist_for_each_entry_safe(key, pos, n, &tp->md5sig_info->head, node) { + hlist_for_each_entry_safe(key, pos, n, &md5sig->head, node) { hlist_del_rcu(&key->node); atomic_sub(sizeof(*key), &sk->sk_omem_alloc); kfree_rcu(key, rcu); @@ -1009,12 +1020,9 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval, if (sin->sin_family != AF_INET) return -EINVAL; - if (!cmd.tcpm_key || !cmd.tcpm_keylen) { - if (!tcp_sk(sk)->md5sig_info) - return -ENOENT; + if (!cmd.tcpm_key || !cmd.tcpm_keylen) return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr, AF_INET); - } if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN) return -EINVAL; @@ -1896,7 +1904,7 @@ void tcp_v4_destroy_sock(struct sock *sk) /* Clean up the MD5 key list, if any */ if (tp->md5sig_info) { tcp_clear_md5_list(sk); - kfree(tp->md5sig_info); + kfree_rcu(tp->md5sig_info, rcu); tp->md5sig_info = NULL; } #endif diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c index bec41f9..c250181 100644 --- a/net/ipv6/tcp_ipv6.c +++ b/net/ipv6/tcp_ipv6.c @@ -571,8 +571,6 @@ static int tcp_v6_parse_md5_keys (struct sock *sk, char __user *optval, return -EINVAL; if (!cmd.tcpm_keylen) { - if (!tcp_sk(sk)->md5sig_info) - return -ENOENT; if (ipv6_addr_v4mapped(&sin6->sin6_addr)) return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3], AF_INET); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] tcp: md5: protects md5sig_info with RCU 2012-02-01 4:45 ` [PATCH net-next] tcp: md5: protects md5sig_info with RCU Eric Dumazet @ 2012-02-01 7:18 ` David Miller 0 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2012-02-01 7:18 UTC (permalink / raw) To: eric.dumazet; +Cc: shawn.lu, netdev, xiaoclu From: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 01 Feb 2012 05:45:40 +0100 > This patch makes sure we use appropriate memory barriers before > publishing tp->md5sig_info, allowing tcp_md5_do_lookup() being used from > tcp_v4_send_reset() without holding socket lock (upcoming patch from > Shawn Lu) > > Note we also need to respect rcu grace period before its freeing, since > we can free socket without this grace period thanks to > SLAB_DESTROY_BY_RCU > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-02-01 7:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-01 0:50 (unknown), Shawn Lu 2012-02-01 0:50 ` [PATCH] tcp: md5: fix md5 RST when both sides have listener Shawn Lu 2012-02-01 4:08 ` Eric Dumazet 2012-02-01 4:45 ` [PATCH net-next] tcp: md5: protects md5sig_info with RCU Eric Dumazet 2012-02-01 7:18 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox