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 06dc4d6..9730742 100644 (file)
--- 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 920a7ba..7454631 100644 (file)
--- 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 0563d04..49850ac 100644 (file)
--- 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));
 }
 
 /*