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: <20150116.164830.172588830692811414.davem@davemloft.net>
Date:	Fri, 16 Jan 2015 16:48:30 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	subashab@...eaurora.org
Cc:	eric.dumazet@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: ipv4: Fix incorrect free in ICMP receive

From: subashab@...eaurora.org
Date: Fri, 16 Jan 2015 20:59:14 -0000

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

The calls to skb->destructor(), done by skb_queue_purge() (via
kfree_skb()) do this.

But even if it didn't, the fact remains that we are operating on the
socket right here in the destructor.  It still exists and has not been
freed yet.

And furthermore, exactly what skb_orphan() does is call skb->destructor(),
_JUST_LIKE_ skb_queue_purge() will via kfree_skb().

So either sock_rfree() is safe to call here, or it isn't.  You are not
eliminating the calls to sock_rfree() which operate on this socket at
all.  If you did, then the socket memory counters would end up being
corrupts and the warnings would trigger:

	WARN_ON(atomic_read(&sk->sk_rmem_alloc));
	WARN_ON(atomic_read(&sk->sk_wmem_alloc));

You're just moving the skb->destructor() call up a few lines in the
same function, it makes therefore no sense why this would fix a bug
or not.
--
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