netdev - Re: [PATCH] net: ipv4: Fix incorrect free in ICMP receive

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: <1421412775.11734.117.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Fri, 16 Jan 2015 04:52:55 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	subashab@...eaurora.org
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] net: ipv4: Fix incorrect free in ICMP receive

On Fri, 2015-01-16 at 07:48 +0000, subashab@...eaurora.org wrote:
> An exception is seen in ICMP ping receive path where the skb
> destructor sock_rfree() tries to access a freed socket. This happens
> because ping_rcv() releases socket reference with sock_put() and this
> internally frees up the socket. Later icmp_rcv() will try to free the
> skb and as part of this, skb destructor is called and panics as the
> socket is freed already in ping_rcv().
> 
> WARN stack trace @       WARN_ON(atomic_read(&sk->sk_rmem_alloc));
> dump_backtrace+0x0/0x248
> show_stack+0x10/0x1c
> dump_stack+0x1c/0x28
> warn_slowpath_common+0x74/0x9c
> warn_slowpath_null+0x14/0x20
> inet_sock_destruct+0x130/0x1a0
> __sk_free+0x1c/0x168
> sk_free+0x24/0x30
> ping_rcv+0xf4/0x124
> icmp_rcv+0x224/0x2c4
> ip_local_deliver_finish+0x108/0x214
> ip_local_deliver+0x88/0xa0
> ip_rcv_finish+0x234/0x284
> ip_rcv+0x258/0x2e8
> __netif_receive_skb_core+0x640/0x6b4
> <snip>
> 
> -->|exception
> -007|sk_mem_uncharge
> -007|sock_rfree
> -008|skb_release_head_state
> -009|skb_release_all
> -009|__kfree_skb
> -010|kfree_skb
> -011|icmp_rcv
> -012|ip_local_deliver_finish
> 
> Fix this by orphaning the skb's before freeing the socket
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
> ---
>  net/ipv4/af_inet.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index b507a47..0c58f0e5 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -147,6 +147,12 @@ EXPORT_SYMBOL(ipv4_config);
>  void inet_sock_destruct(struct sock *sk)
>  {
>  	struct inet_sock *inet = inet_sk(sk);
> +	struct sk_buff *skb;
> +
> +	skb_queue_walk(&sk->sk_receive_queue, skb)
> +		skb_orphan(skb);
> +	skb_queue_walk(&sk->sk_error_queue, skb)
> +		skb_orphan(skb);
> 
>  	__skb_queue_purge(&sk->sk_receive_queue);
>  	__skb_queue_purge(&sk->sk_error_queue);

Sorry this makes absolutely no sense to me.

skb_queue_purge() is also calling skb_orphan() on all skb.

And no, sock_put() does not free the socket.
We release a refcount that was acquired when we did the socket lookup.



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