netdev - RE: neigh use-after-free

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 for Android: free password hash cracker in your pocket

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

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