Skip to content

Commit

Permalink
ip multicast: fix use-after-free
Browse files Browse the repository at this point in the history
During if_detach(), we get a race where a closing socket is
releasing multicast data (via inp_freemoptions()) at the same
time as igmp_ifdetach() is releasing all multicast data for
the interface, resulting in a potential double teardown and
double free.

This bug has been present since late 2011:

    Author: jhb <jhb@FreeBSD.org>

    Defer the work of freeing IPv4 multicast options from a
    socket to an asychronous task. ...

It is very hard to trip over.  You must create and delete
interfaces (bridges that join real interfaces are good
candidates) repeatedly, and even then, if M_IPMADDR (in_multi
data structure) memory is not reused for something else during
the race, the reference count in inm->inm_refcount is an
unsigned int, so it decrements from the left-over 0 to
4294967295, avoiding a second free.

Turning on the memory debug options (scribbling values over
freed memory) catches the problem more quickly, though you
still must create and destroy interfaces that partcipate in
multicast to see it.

The fix here is a kludge, but should serve until the entire
network locking code and up/downcall system is reworked.
  • Loading branch information
Chris Torek authored and kmoore134 committed Apr 17, 2017
1 parent f768c70 commit 8385428
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 0 deletions.
4 changes: 4 additions & 0 deletions sys/netinet/in.c
Original file line number Diff line number Diff line change
Expand Up @@ -1003,6 +1003,8 @@ in_purgemaddrs(struct ifnet *ifp)
struct in_multi *inm, *tinm;
struct ifmultiaddr *ifma;

inp_freemopt_wait(); /* XXX kludge */

LIST_INIT(&purgeinms);
IN_MULTI_LOCK();

Expand Down Expand Up @@ -1033,6 +1035,8 @@ in_purgemaddrs(struct ifnet *ifp)
igmp_ifdetach(ifp);

IN_MULTI_UNLOCK();

inp_freemopt_wait(); /* XXX kludge */
}

struct in_llentry {
Expand Down
11 changes: 11 additions & 0 deletions sys/netinet/in_mcast.c
Original file line number Diff line number Diff line change
Expand Up @@ -1585,6 +1585,17 @@ inp_freemoptions(struct ip_moptions *imo)
taskqueue_enqueue(taskqueue_thread, &imo_gc_task);
}

/*
* Wait for inp_freemoptions() to complete any current work.
* Used because inp_freemoptions is no longer synchronous.
*/
void
inp_freemopt_wait(void)
{

taskqueue_drain(taskqueue_thread, &imo_gc_task);
}

static void
inp_freemoptions_internal(struct ip_moptions *imo)
{
Expand Down
1 change: 1 addition & 0 deletions sys/netinet/ip_var.h
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ extern struct pr_usrreqs rip_usrreqs;
#define V_drop_redirect VNET(drop_redirect)

void inp_freemoptions(struct ip_moptions *);
void inp_freemopt_wait(void);
int inp_getmoptions(struct inpcb *, struct sockopt *);
int inp_setmoptions(struct inpcb *, struct sockopt *);

Expand Down

0 comments on commit 8385428

Please sign in to comment.