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]

Message-ID: <1421444892.11734.148.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Fri, 16 Jan 2015 13:48:12 -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 20:59 +0000, subashab@...eaurora.org wrote:
> >skb_queue_purge() is also calling skb_orphan() on all skb
> From my reading, it looked like skb_queue_purge() is dequeuing and calling
> kfree_skb() which will release a reference. I did not see skb_orphan()
> being called directly. Am I missing something?
> I think that if it had really orphaned the skb, then this crash would not
> be seen in the first place.

Well, I never said you had no crash.

I said your 'fix' made no sense.

Sure, we automatically call skb_orphan() when skb are freed, as a in
kfree_skb()

Thats whole point having skb->destructor


> >And no, sock_put() does not free the socket.
> >We release a refcount that was acquired when we did the socket lookup.
> I agree that the refcount is released, but it looked to me like inside
> sock_put() the reference count reached 0 hence the socket was freed.
> 
> Here is why I say this
> (from dmesg)
> 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            [sock_put() inlined hence not seen in the
> stack trace I believe]
> ping_rcv+0xf4/0x124
> icmp_rcv+0x224/0x2c4
> ip_local_deliver_finish+0x108/0x214
> 
> and here is stack at the time of the exception
> -007|sk_mem_uncharge   [trying to access freed sk structure]
> -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
> 
> I am not sure why the ref reached zero and if that is even the right trail
> of thought. Could the userspace have released the sock reference also and
> this is race condition? Any thoughts?

Normally it is not possible. By definition, ping_lookup() does a
sock_hold() on a socket that is still alive (since it was found in hash
table). Hash table is protected by ping_table.lock rwlock.

> 
> Note that we are using the 3.10 kernel version and does not have Rick
> Jones patch in this area e3e3217029a35c579bf100998b43976d0b1cb8d7
> "icmp: Remove some spurious dropped packet profile hits from the ICMP",
> although it looks to me that this patch would not solve this problem
> either.


Seriously guys.

Do not post upstream patches if you cannot reproduce the issue on
current kernel.

Thanks



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