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) {
