On Sun, Jan 03, 2016 at 03:54:16PM +0100, Martin Pieuchot wrote: > On 03/01/16(Sun) 14:19, Fabian Raetz wrote: > > On Sun, Jan 03, 2016 at 11:54:18AM +0100, Martin Pieuchot wrote: > > > On 30/12/15(Wed) 12:51, Fabian Raetz wrote: > > > > Hi tech@, > > > > > > > > i've found the undocumented -carpdev option in ifconfig(8) which freezes > > > > my sytem if executed. > > > > > > > > As the -carpdev option is undocumented in both ifconfig(8) and carp(4) i > > > > propose two patches to remove this functionality. > > > > > > > > The patch below will return EINVAL in SIOCSVH if carpr.carpr_carpdev is > > > > not a > > > > valid interface name. > > > > > > Did you try this diff? The SIOCGVH ioctl(2) is not always issued with > > > carpr_carpdev set, so this will break your system. > > [...] > > Or are you worried about something completly different and i'm missing > > the point? > > I'm worried about the fact that your diff breaks ifconfig(8) with > carp(4) interfaces because, as I said previously, the SIOCGVH ioctl(2) > is not always issued with carpr_carpdev set :)
Ok, i've got it xD. I've tested your patch and it fixes the observed NULL dereference and ifconfig seems to be working. > > Here's a different approach that should prevent your NULL dereference > (untested): > > Index: netinet/ip_carp.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_carp.c,v > retrieving revision 1.284 > diff -u -p -r1.284 ip_carp.c > --- netinet/ip_carp.c 19 Dec 2015 11:19:35 -0000 1.284 > +++ netinet/ip_carp.c 3 Jan 2016 14:51:20 -0000 > @@ -1653,11 +1653,9 @@ carp_set_ifp(struct carp_softc *sc, stru > int myself = 0, error = 0; > int s; > > + KASSERT(ifp0 != sc->sc_carpdev); > KERNEL_ASSERT_LOCKED(); /* touching vhif_vrs */ > > - if (ifp0 == sc->sc_carpdev) > - return (0); > - > if ((ifp0->if_flags & IFF_MULTICAST) == 0) > return (EADDRNOTAVAIL); > > @@ -1968,12 +1966,12 @@ carp_ioctl(struct ifnet *ifp, u_long cmd > struct carpreq carpr; > struct ifaddr *ifa = (struct ifaddr *)addr; > struct ifreq *ifr = (struct ifreq *)addr; > - struct ifnet *ifp0 = NULL; > + struct ifnet *ifp0 = sc->sc_carpdev; > int i, error = 0; > > switch (cmd) { > case SIOCSIFADDR: > - if (sc->sc_carpdev == NULL) > + if (ifp0 == NULL) > return (EINVAL); > > switch (ifa->ifa_addr->sa_family) { > @@ -2029,8 +2027,10 @@ carp_ioctl(struct ifnet *ifp, u_long cmd > sc->sc_peer.s_addr = INADDR_CARP_GROUP; > else > sc->sc_peer.s_addr = carpr.carpr_peer.s_addr; > - if ((error = carp_set_ifp(sc, ifp0))) > - return (error); > + if (ifp0 != sc->sc_carpdev) { > + if ((error = carp_set_ifp(sc, ifp0))) > + return (error); > + } > if (vhe->state != INIT && carpr.carpr_state != vhe->state) { > switch (carpr.carpr_state) { > case BACKUP: > @@ -2090,9 +2090,8 @@ carp_ioctl(struct ifnet *ifp, u_long cmd > > case SIOCGVH: > memset(&carpr, 0, sizeof(carpr)); > - if (sc->sc_carpdev != NULL) > - strlcpy(carpr.carpr_carpdev, sc->sc_carpdev->if_xname, > - IFNAMSIZ); > + if (ifp0 != NULL) > + strlcpy(carpr.carpr_carpdev, ifp0->if_xname, IFNAMSIZ); > i = 0; > KERNEL_ASSERT_LOCKED(); /* touching carp_vhosts */ > SRPL_FOREACH_LOCKED(vhe, &sc->carp_vhosts, vhost_entries) {