netdev - Re: [RFC] net: tcp: null pointer crash in __inet_put

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1422479015.21689.3.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Wed, 28 Jan 2015 13:03:35 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Harout Hedeshian <harouth@...eaurora.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC] net: tcp: null pointer crash in __inet_put_port

On Wed, 2015-01-28 at 08:56 -0700, Harout Hedeshian wrote:
> Hello,
> 
> We are observing a crash in __inet_put_port() wherein icsk_bind_hash
> is null (stack trace below).
> 
> net/ipv4/inet_hashtables.c:
> tb = inet_csk(sk)->icsk_bind_hash;
> __sk_del_bind_node(sk);
> tb->num_owners--;       <-- tb is unconditionally dereferenced here
> 
> From the RAM dumps: [D:0xFFFFFFC0222FB6F8] icsk_bind_hash = 0x0 = ,
> 
> The caller of inet_put_port() in this case is tcp_set_state() which
> already does a null check on this particular field before calling
> inet_put_port().
> 
> net/ipv4/tcp.c:
> if (inet_csk(sk)->icsk_bind_hash &&
>    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
> 	inet_put_port(sk);
> 
> It seems that inet_put_port() does some locking by disabling bottom
> halves, but this locking is happening after the null check.
> 
> Additionally, I see tcp_v4_destroy_sock() also calling inet_put_port()
> after doing a similar null check. However, I am unsure if it would
> ever be called on the same socket.
> 
> Does the proposed solution of locking the bottom halves from
> tcp_set_state() make any sense here? Or use some other locking mechanism
> directly on the socket?

>  net/ipv4/tcp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 3075723..f3b5714 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1910,9 +1910,11 @@ void tcp_set_state(struct sock *sk, int state)
>  			TCP_INC_STATS(sock_net(sk), TCP_MIB_ESTABRESETS);
>  
>  		sk->sk_prot->unhash(sk);
> +		local_bh_disable();
>  		if (inet_csk(sk)->icsk_bind_hash &&
>  		    !(sk->sk_userlocks & SOCK_BINDPORT_LOCK))
>  			inet_put_port(sk);
> +		local_bh_enable();
>  		/* fall through */
>  	default:
>  		if (oldstate == TCP_ESTABLISHED)


I don't think it makes sense.

tcp_set_state() should only be used on a locked socket.

I believe you are papering over another bug.

bh are disabled in inet_put_port because of the spin_lock() that can be
taken both from process and softirq contexts.



--
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