From a134c703e536b786e27dd7a07825eea121502871 Mon Sep 17 00:00:00 2001 From: bluhm Date: Mon, 28 Mar 2022 16:31:26 +0000 Subject: [PATCH] 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@ --- sys/netinet/igmp.c | 20 +++++--------------- sys/netinet/igmp_var.h | 6 +++--- sys/netinet/in.c | 41 ++++++++++++++++++++--------------------- 3 files changed, 28 insertions(+), 39 deletions(-) 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)); } /* -- 2.20.1