NULL pointer dereference and other nasty things in /proc/net/(tcp|udp)[6]
Subsystems (4)
PATCH 1/1 NULL pointer dereference and other nasty things in /proc/net/(tcp|udp)[6]
Commits f40c81 ([NETNS][IPV4] tcp - make proc handle the network namespaces) and a91275 ([NETNS][IPV6] udp - make proc handle the network namespace) both introduced bad checks on sockets and tw buckets to belong to proper net namespace.
I.e. when checking for socket to belong to given net and family the
do {
sk = sk_next(sk);
} while (sk && sk->sk_net != net && sk->sk_family != family);constructions were used. This is wrong, since as soon as the sk->sk_net fits the net the socket is immediately returned, even if it belongs to other family.
As the result four /proc/net/(udp|tcp)[6] entries show wrong info. The udp6 entry even oopses when dereferencing inet6_sk(sk) pointer:
static void udp6_sock_seq_show(struct seq_file *seq, struct sock *sp, int bucket)
{
...
struct ipv6_pinfo *np = inet6_sk(sp);
...
dest = &np->daddr; /* will be NULL for AF_INET sockets */
...
seq_printf(...
dest->s6_addr32[0], dest->s6_addr32[1],
dest->s6_addr32[2], dest->s6_addr32[3],
...Fix it by converting && to ||.
---
net/ipv4/tcp_ipv4.c +2 -2
@@ -2050,7 +2050,7 @@ static void *established_get_first(struct seq_file *seq) st->state =TCP_SEQ_STATE_TIME_WAIT ;inet_twsk_for_each (tw, node, &tcp_hashinfo.ehash[st->bucket].twchain) { -if (tw->tw_family != st->family && +if (tw->tw_family != st->family || tw->tw_net != net) {continue ; }
@@ -2078,7 +2078,7 @@ static void *established_get_next(struct seq_file *seq, void *cur) tw = cur; tw =tw_next (tw); get_tw: -while (tw && tw->tw_family != st->family && tw->tw_net != net) { +while (tw && (tw->tw_family != st->family || tw->tw_net != net)) { tw =tw_next (tw); }if (tw) {
net/ipv4/udp.c +1 -1
@@ -1537,7 +1537,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) sk =sk_next (sk); try_again: ; - }while (sk && sk->sk_net != net && sk->sk_family != state->family); + }while (sk && (sk->sk_net != net || sk->sk_family != state->family));if (!sk && ++state->bucket <UDP_HTABLE_SIZE ) { sk =sk_head (state->hashtable + state->bucket);
Quoted 61 lines · Pavel Emelyanov
Commits f40c81 ([NETNS][IPV4] tcp - make proc handle the network namespaces) and a91275 ([NETNS][IPV6] udp - make proc handle the network namespace) both introduced bad checks on sockets and tw buckets to belong to proper net namespace.
I.e. when checking for socket to belong to given net and family the
do { sk = sk_next(sk); } while (sk && sk->sk_net != net && sk->sk_family != family);constructions were used. This is wrong, since as soon as the
sk->sk_netfits the net the socket is immediately returned, even if it belongs to other family.As the result four /proc/net/(udp|tcp)[6] entries show wrong info. The udp6 entry even oopses when dereferencing
inet6_sk(sk)pointer:static void
udp6_sock_seq_show(struct seq_file *seq, struct sock *sp, int bucket){ ... struct ipv6_pinfo *np = inet6_sk(sp); ... dest = &np->daddr; /* will be NULL for AF_INET sockets */ ... seq_printf(... dest->s6_addr32[0], dest->s6_addr32[1], dest->s6_addr32[2], dest->s6_addr32[3], ...Fix it by converting && to ||.
Signed-off-by Pavel Emelyanov <[email protected]>---
net/ipv4/tcp_ipv4.c +2
-2@@ -2050,7 +2050,7 @@ static void *established_get_first(struct seq_file *seq) st->state =TCP_SEQ_STATE_TIME_WAIT ;inet_twsk_for_each (tw, node, &tcp_hashinfo.ehash[st->bucket].twchain) { -if (tw->tw_family != st->family && +if (tw->tw_family != st->family || tw->tw_net != net) {continue ; }@@ -2078,7 +2078,7 @@ static void *established_get_next(struct seq_file *seq, void *cur) tw = cur; tw =tw_next (tw); get_tw: -while (tw && tw->tw_family != st->family && tw->tw_net != net) { +while (tw && (tw->tw_family != st->family || tw->tw_net != net)) { tw =tw_next (tw); }if (tw) {net/ipv4/udp.c +0
-0
net/ipv4/udp.c static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) · line 1543
Quoted 8 lines
net/ipv4/udp.c +1
-1@@ -1537,7 +1537,7 @@ static struct sock *udp_get_next(struct seq_file *seq, struct sock *sk) sk =sk_next (sk); try_again: ; - }while (sk && sk->sk_net != net && sk->sk_family != state->family); + }while (sk && (sk->sk_net != net || sk->sk_family != state->family));if (!sk && ++state->bucket <UDP_HTABLE_SIZE ) { sk =sk_head (state->hashtable + state->bucket);
good catch.
Weird I missed that :(
Thank you for fixing this so quickly.
From: Daniel Lezcano <[email protected]>
Date: Fri, 21 Mar 2008 16:08:57 +0100
Quoted 3 lines
Thank you for fixing this so quickly.
Acked-by Daniel Lezcano <[email protected]>
Applied, thanks.