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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 19 Jan 2015 19:50:37 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	subashab@...eaurora.org
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: ipv4: Fix incorrect free in ICMP receive

On Tue, 2015-01-20 at 02:34 +0000, subashab@...eaurora.org wrote:
> Thanks David and Eric for the insights. In order for me to steer this
> debug in the right direction, can you please help me? Based on your input
> I looked into this a little deeper to understand the refcnts for sockets
> and skb's in this ping receive path.
> 
> from ping_rcv()
> 
>                 sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
>                 if (sk != NULL) {
>                                 pr_debug("rcv on socket %p\n", sk);
>                                 ping_queue_rcv_skb(sk, skb_get(skb));
>                                 sock_put(sk);
>                                 return;
>                 }
> 
> From my understanding I have made the following analysis, please correct
> if I am wrong.
> 
> 1) There is no guarantee that sock_put() in the above code snippet
> will not drop the socket refcount to 0 and free the socket. This can
> hypothetically happen if say
> sock_close()->ping_close()->*->ping_unhash()->sock_put()
> can  happen between in a different context between ping_lookup() and
> sock_put() in the above code snippet. Is this observation accurate?
> 
> 2)  Now since this socket is being freed in the ping receive path, I think
> the following is what is happening with the skb.
> alloc_skb()[skb->users=1]   -> deliver_skb()[skb->users=2]  -> * ->
> icmp_rcv() -> ping_rcv() -> sk_free --> inet_sock_destruct()->
> __skb_queue_purge()->kfree_skb()[dec ref cnt, skb->users=1]
> 
> when stack unwinds to icmp_rcv(), refcnt actually hits zero and packet is
> freed calling the destructor which tries to access the freed socket.
> 
> If these observations are right, Can you please tell me what is the call
> flow that is not supposed to happen but is happening in this issue? I am
> trying to understand better to identify next steps to tackle this issue.

This is why skb_get() is very often a bug.

There is no guarantee the consume_skb() in icmp_rcv() is done before the
skb_queue_purge().

diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index c0d82f78d364..2a3720fb5a5f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -966,8 +966,11 @@ bool ping_rcv(struct sk_buff *skb)
 
 	sk = ping_lookup(net, skb, ntohs(icmph->un.echo.id));
 	if (sk != NULL) {
+		struct sk_buff *skb2 = skb_clone(skb, GFP_ATOMIC);
+
 		pr_debug("rcv on socket %p\n", sk);
-		ping_queue_rcv_skb(sk, skb_get(skb));
+		if (skb2)
+			ping_queue_rcv_skb(sk, skb2);
 		sock_put(sk);
 		return true;
 	}


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