From: bluhm <bluhm@openbsd.org>
Date: Mon, 28 Mar 2022 16:31:26 +0000 (+0000)
Subject: if_detach() does if_remove(ifp); NET_LOCK(); rti_delete().  New
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a134c703e536b786e27dd7a07825eea121502871;p=openbsd

if_detach() does if_remove(ifp); NET_LOCK(); rti_delete().  New
igmp groups may join while sleeping in interface destruction.  In
this case if_get() in igmp_joingroup() fails and rti_fill() is not
called.  Then inm->inm_rti may be NULL.  This is the condition when
syzkaller crashes in igmp_leavegroup().
Pass the ifp the current CPU is already holding down to igmp_joingroup()
and igmp_leavegroup() to avoid half constructed igmp groups.  Calling
if_get() in caller and callee makes no sense anyway.
Reported-by: syzbot+146823a676b7bea83649@syzkaller.appspotmail.com
OK denis@
---

diff --git a/sys/netinet/igmp.c b/sys/netinet/igmp.c
index 06dc4d6551c..973074266c5 100644
--- a/sys/netinet/igmp.c
+++ b/sys/netinet/igmp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: igmp.c,v 1.77 2021/12/15 15:58:01 bluhm Exp $	*/
+/*	$OpenBSD: igmp.c,v 1.78 2022/03/28 16:31:26 bluhm Exp $	*/
 /*	$NetBSD: igmp.c,v 1.15 1996/02/13 23:41:25 christos Exp $	*/
 
 /*
@@ -483,17 +483,14 @@ igmp_input_if(struct ifnet *ifp, struct mbuf **mp, int *offp, int proto, int af)
 }
 
 void
-igmp_joingroup(struct in_multi *inm)
+igmp_joingroup(struct in_multi *inm, struct ifnet *ifp)
 {
-	struct ifnet* ifp;
 	int i;
 
-	ifp = if_get(inm->inm_ifidx);
-
 	inm->inm_state = IGMP_IDLE_MEMBER;
 
 	if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
-	    ifp && (ifp->if_flags & IFF_LOOPBACK) == 0) {
+	    (ifp->if_flags & IFF_LOOPBACK) == 0) {
 		i = rti_fill(inm);
 		igmp_sendpkt(ifp, inm, i, 0);
 		inm->inm_state = IGMP_DELAYING_MEMBER;
@@ -502,22 +499,16 @@ igmp_joingroup(struct in_multi *inm)
 		igmp_timers_are_running = 1;
 	} else
 		inm->inm_timer = 0;
-
-	if_put(ifp);
 }
 
 void
-igmp_leavegroup(struct in_multi *inm)
+igmp_leavegroup(struct in_multi *inm, struct ifnet *ifp)
 {
-	struct ifnet* ifp;
-
-	ifp = if_get(inm->inm_ifidx);
-
 	switch (inm->inm_state) {
 	case IGMP_DELAYING_MEMBER:
 	case IGMP_IDLE_MEMBER:
 		if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
-		    ifp && (ifp->if_flags & IFF_LOOPBACK) == 0)
+		    (ifp->if_flags & IFF_LOOPBACK) == 0)
 			if (inm->inm_rti->rti_type != IGMP_v1_ROUTER)
 				igmp_sendpkt(ifp, inm,
 				    IGMP_HOST_LEAVE_MESSAGE,
@@ -528,7 +519,6 @@ igmp_leavegroup(struct in_multi *inm)
 	case IGMP_SLEEPING_MEMBER:
 		break;
 	}
-	if_put(ifp);
 }
 
 void
diff --git a/sys/netinet/igmp_var.h b/sys/netinet/igmp_var.h
index 920a7ba7053..7454631c840 100644
--- a/sys/netinet/igmp_var.h
+++ b/sys/netinet/igmp_var.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: igmp_var.h,v 1.14 2020/08/17 16:25:34 gnezdo Exp $	*/
+/*	$OpenBSD: igmp_var.h,v 1.15 2022/03/28 16:31:26 bluhm Exp $	*/
 /*	$NetBSD: igmp_var.h,v 1.9 1996/02/13 23:41:31 christos Exp $	*/
 
 /*
@@ -107,8 +107,8 @@ igmpstat_inc(enum igmpstat_counters c)
 
 void	igmp_init(void);
 int	igmp_input(struct mbuf **, int *, int, int);
-void	igmp_joingroup(struct in_multi *);
-void	igmp_leavegroup(struct in_multi *);
+void	igmp_joingroup(struct in_multi *, struct ifnet *);
+void	igmp_leavegroup(struct in_multi *, struct ifnet *);
 void	igmp_fasttimo(void);
 void	igmp_slowtimo(void);
 int	igmp_sysctl(int *, u_int, void *, size_t *, void *, size_t);
diff --git a/sys/netinet/in.c b/sys/netinet/in.c
index 0563d04f876..49850acaff9 100644
--- a/sys/netinet/in.c
+++ b/sys/netinet/in.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: in.c,v 1.172 2022/03/04 21:09:03 bluhm Exp $	*/
+/*	$OpenBSD: in.c,v 1.173 2022/03/28 16:31:26 bluhm Exp $	*/
 /*	$NetBSD: in.c,v 1.26 1996/02/13 23:41:39 christos Exp $	*/
 
 /*
@@ -891,7 +891,7 @@ in_addmulti(struct in_addr *ap, struct ifnet *ifp)
 		/*
 		 * Let IGMP know that we have joined a new IP multicast group.
 		 */
-		igmp_joingroup(inm);
+		igmp_joingroup(inm, ifp);
 	}
 
 	return (inm);
@@ -908,35 +908,34 @@ in_delmulti(struct in_multi *inm)
 
 	NET_ASSERT_LOCKED();
 
-	if (--inm->inm_refcnt == 0) {
+	if (--inm->inm_refcnt != 0)
+		return;
+
+	ifp = if_get(inm->inm_ifidx);
+	if (ifp != NULL) {
 		/*
 		 * No remaining claims to this record; let IGMP know that
 		 * we are leaving the multicast group.
 		 */
-		igmp_leavegroup(inm);
-		ifp = if_get(inm->inm_ifidx);
+		igmp_leavegroup(inm, ifp);
 
 		/*
 		 * Notify the network driver to update its multicast
 		 * reception filter.
 		 */
-		if (ifp != NULL) {
-			memset(&ifr, 0, sizeof(ifr));
-			satosin(&ifr.ifr_addr)->sin_len =
-			    sizeof(struct sockaddr_in);
-			satosin(&ifr.ifr_addr)->sin_family = AF_INET;
-			satosin(&ifr.ifr_addr)->sin_addr = inm->inm_addr;
-			KERNEL_LOCK();
-			(*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)&ifr);
-			KERNEL_UNLOCK();
-
-			TAILQ_REMOVE(&ifp->if_maddrlist, &inm->inm_ifma,
-			    ifma_list);
-		}
-		if_put(ifp);
-
-		free(inm, M_IPMADDR, sizeof(*inm));
+		memset(&ifr, 0, sizeof(ifr));
+		satosin(&ifr.ifr_addr)->sin_len = sizeof(struct sockaddr_in);
+		satosin(&ifr.ifr_addr)->sin_family = AF_INET;
+		satosin(&ifr.ifr_addr)->sin_addr = inm->inm_addr;
+		KERNEL_LOCK();
+		(*ifp->if_ioctl)(ifp, SIOCDELMULTI, (caddr_t)&ifr);
+		KERNEL_UNLOCK();
+
+		TAILQ_REMOVE(&ifp->if_maddrlist, &inm->inm_ifma, ifma_list);
 	}
+	if_put(ifp);
+
+	free(inm, M_IPMADDR, sizeof(*inm));
 }
 
 /*