Le Tue, Mar 22, 2022 at 02:57:31PM +0100, Alexander Bluhm a écrit : > anyone? >
It looks OK denis@ > On Fri, Mar 04, 2022 at 12:09:03PM +0100, Alexander Bluhm wrote: > > Hi, > > > > syzkaller has found this race in arp. > > > > https://syzkaller.appspot.com/bug?id=e3dc94533ddee95b6d69c2e7049360022f4190d3 > > > > The assumption of the code is that either the arp entry or the > > interface is removed. > > > > But in if_detach() if_remove() is called without net lock and all > > arp entries are removed later in in_ifdetach() -> in_purgeaddr() > > -> rt_ifa_purge() -> rtdeletemsg(). > > > > When the arp timeout fires while if_detach() is between if_remove() > > and NET_LOCK() then arptfree() has do deal with partially destroyed > > interfaces. We can skip rtdeletemsg() as if_detach() will take > > care of it. > > > > > > While syzkaller has not found it, nd6 has to deal with the same > > problem. Make nd6_free() simmilar to arptfree(). > > > > > > This crash may have the same source of problem. > > > > https://syzkaller.appspot.com/bug?id=9649f7319437a49298a38572b83f38f0b7d37fbe > > > > if_detach() does if_remove(ifp); NET_LOCK(); rti_delete(). So new > > igmp groups may appear during interface destruction. igmp_joingroup() > > does not call rti_fill() as if_get() fails. Then inm->inm_rti may > > be NULL. This is the condition when syzkaller crashes in > > igmp_leavegroup(). > > > > When we pass the ifp this CPU is already holding, we avoid half > > constructed igmp groups. Calling if_get() multiple times in caller > > and callee makes no sense anyway. > > > > ok? partial ok for one of the fixes also welcome. > > > > bluhm > > > > > > Index: netinet/if_ether.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v > > retrieving revision 1.248 > > diff -u -p -r1.248 if_ether.c > > --- netinet/if_ether.c 28 Apr 2021 21:21:44 -0000 1.248 > > +++ netinet/if_ether.c 3 Mar 2022 23:31:55 -0000 > > @@ -722,7 +722,9 @@ arptfree(struct rtentry *rt) > > arpinvalidate(rt); > > > > ifp = if_get(rt->rt_ifidx); > > - KASSERT(ifp != NULL); > > + if (ifp == NULL) > > + return; > > + > > if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED)) > > rtdeletemsg(rt, ifp, ifp->if_rdomain); > > if_put(ifp); > > Index: netinet/igmp.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp.c,v > > retrieving revision 1.77 > > diff -u -p -r1.77 igmp.c > > --- netinet/igmp.c 15 Dec 2021 15:58:01 -0000 1.77 > > +++ netinet/igmp.c 3 Mar 2022 23:58:32 -0000 > > @@ -483,17 +483,14 @@ igmp_input_if(struct ifnet *ifp, struct > > } > > > > 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 > > Index: netinet/igmp_var.h > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/igmp_var.h,v > > retrieving revision 1.14 > > diff -u -p -r1.14 igmp_var.h > > --- netinet/igmp_var.h 17 Aug 2020 16:25:34 -0000 1.14 > > +++ netinet/igmp_var.h 4 Mar 2022 00:02:45 -0000 > > @@ -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); > > Index: netinet/in.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in.c,v > > retrieving revision 1.171 > > diff -u -p -r1.171 in.c > > --- netinet/in.c 10 Mar 2021 10:21:48 -0000 1.171 > > +++ netinet/in.c 3 Mar 2022 23:57:49 -0000 > > @@ -894,7 +894,7 @@ in_addmulti(struct in_addr *ap, struct i > > /* > > * Let IGMP know that we have joined a new IP multicast group. > > */ > > - igmp_joingroup(inm); > > + igmp_joingroup(inm, ifp); > > } > > > > return (inm); > > @@ -911,35 +911,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); > > + 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(); > > > > - free(inm, M_IPMADDR, sizeof(*inm)); > > + TAILQ_REMOVE(&ifp->if_maddrlist, &inm->inm_ifma, ifma_list); > > } > > + if_put(ifp); > > + > > + free(inm, M_IPMADDR, sizeof(*inm)); > > } > > > > /* > > Index: netinet6/nd6.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v > > retrieving revision 1.238 > > diff -u -p -r1.238 nd6.c > > --- netinet6/nd6.c 22 Feb 2022 01:15:02 -0000 1.238 > > +++ netinet6/nd6.c 3 Mar 2022 23:38:56 -0000 > > @@ -716,7 +716,12 @@ nd6_free(struct rtentry *rt) > > > > NET_ASSERT_LOCKED(); > > > > + KASSERT(!ISSET(rt->rt_flags, RTF_LOCAL)); > > + nd6_invalidate(rt); > > + > > ifp = if_get(rt->rt_ifidx); > > + if (ifp == NULL) > > + return; > > > > if (!ip6_forwarding) { > > if (ln->ln_router) { > > @@ -729,9 +734,6 @@ nd6_free(struct rtentry *rt) > > } > > } > > > > - KASSERT(!ISSET(rt->rt_flags, RTF_LOCAL)); > > - nd6_invalidate(rt); > > - > > /* > > * Detach the route from the routing tree and the list of neighbor > > * caches, and disable the route entry not to be used in already > > @@ -739,7 +741,6 @@ nd6_free(struct rtentry *rt) > > */ > > if (!ISSET(rt->rt_flags, RTF_STATIC|RTF_CACHED)) > > rtdeletemsg(rt, ifp, ifp->if_rdomain); > > - > > if_put(ifp); > > } > > >
