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, 03 Apr 2015 17:44:09 -0700 From: Eric Dumazet <eric.dumazet@...il.com> To: Alexei Potashnik <alexei@...estorage.com> Cc: Joern Engel <joern@...estorage.com>, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org Subject: Re: neigh use-after-free On Fri, 2015-04-03 at 17:27 -0700, Alexei Potashnik wrote: > > -----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 Your debugging code might be wrong. At entry of neigh_add_timer(), n->refcnt can be 0 if caller holds the correct lock, preventing the timer handler to proceed and decrement refcnt. neigh_timer_handler() does : write_lock(&neigh->lock); ... write_unlock(&neigh->lock); neigh_release(neigh); -- 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