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
| ||
|
Date: Fri, 3 Apr 2015 17:27:16 -0700 From: Alexei Potashnik <alexei@...estorage.com> To: Eric Dumazet <eric.dumazet@...il.com> Cc: Joern Engel <joern@...estorage.com>, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org Subject: RE: neigh use-after-free > -----Original Message----- > From: Eric Dumazet [mailto:eric.dumazet@...il.com] > > On Fri, 2015-04-03 at 13:52 -0700, Alexei Potashnik wrote: > > Would this be an appropriate solution: > > > > diff --git a/net/core/neighbour.c b/net/core/neighbour.c index > > b49e8ba..38265f2 100644 > > --- a/net/core/neighbour.c > > +++ b/net/core/neighbour.c > > @@ -168,7 +168,8 @@ static int neigh_forced_gc(struct neigh_table > > *tbl) > > > > static void neigh_add_timer(struct neighbour *n, unsigned long when) > > { > > - neigh_hold(n); > > + if (!atomic_inc_not_zero(&n->refcnt)) > > + return; > > if (unlikely(mod_timer(&n->timer, when))) { > > printk("NEIGH: BUG, double timer add, state is %x\n", > > n->nud_state); > > This is a very ugly hack. Please find root cause. > > The correct way to implement refcount on a timer is : > > if (!mod_timer(&n->timer, when)) > neigh_hold(&n->refcnt); > > And current code seems to do that (It dumps a printk() and stack trace > otherwise) The printk() and stack trace don't get printed because this is a different case -- it’s not double timer add, but rather add of the timer after neighbor object is deleted. > If refcnt is 0 at this point, it means there is another bug, and we should > fix it > instead of trying to work around it. I agree. And it seems to be clear what code does it: ip_finish_output2 +-> __ipv4_neigh_lookup_noref Lookup name is clearly indicating that noref is intentional. What's not clear is why. The change seems to have originated in commit a263b3093641fb1ec377582c90986a7fd0625184 Author: David S. Miller <davem@...emloft.net> Date: Mon Jul 2 02:02:15 2012 ipv4: Make neigh lookups directly in output packet path. Do not use the dst cached neigh, we'll be getting rid of that. Signed-off-by: David S. Miller <davem@...emloft.net> Before that neigh object came from dst_get_neighbour_noref(), which used to return dst->_neighbour. Now, if dst->_neighbour used own a reference, than the above commit has introduced the bug. Otherwise, the issue has always been there and, perhaps, David Miller can explain why it was deemed safe to operate on neigh object here without extra ref. Alexei -- 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
Powered by blists - more mailing lists