netdev - Re: [PATCH net-2.6.26] NULL pointer dereference and other nasty things in /proc/net/(tcp|udp)[6]

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 21 Mar 2008 16:08:57 +0100
From:	Daniel Lezcano <dlezcano@...ibm.com>
To:	Pavel Emelyanov <xemul@...nvz.org>
CC:	David Miller <davem@...emloft.net>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH net-2.6.26] NULL pointer dereference and other nasty things
 in /proc/net/(tcp|udp)[6]

Pavel Emelyanov wrote:
> 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 ||.
> 
> Signed-off-by: Pavel Emelyanov <xemul@...nvz.org>
> 
> ---
> 
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 744bc9d..0ba6e91 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -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) {
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index a98c43c..fa94682 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -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.

Acked-by: Daniel Lezcano <dlezcano@...ibm.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html