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.
Hi Martin,
in case carpr.carpr_carpdev is zeroed (like it was in ifconfig
-carpdev) ifunit will not be invoked as carpr.carpr_carpdev[0] == '\0'.
As a result carp_set_ifp will be called with ifp0 == NULL (line 2032) which
doesn't
check ifp0 for NULL and will try dereference ifp0 (line 1661). This does
break my system.
What this patch tries to archive is to unconditionally call ifunit (line 2026)
to
make sure ifp0 is always set or we return EINVAL.
Are you worried about the case where carpr.carp_carpdev is not a \0
terminated string and if ifunit can handle these? (Note that this can
happen with the current code too)
I've done some simple testing and it seems to me, that strcmp does work
in such a case as long as ifp->if_xname (net/if.c:1468) is always \0 terminated?
strcmp(3) says that it compares \0 terminated strings though.
Would it make sense to convert the strcmp in ifunit to strncmp?
Index: if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.425
diff -u -p -r1.425 if.c
--- if.c 9 Dec 2015 03:22:39 -0000 1.425
+++ if.c 3 Jan 2016 13:15:31 -0000
@@ -1465,7 +1465,7 @@ ifunit(const char *name)
struct ifnet *ifp;
TAILQ_FOREACH(ifp, &ifnet, if_list) {
- if (strcmp(ifp->if_xname, name) == 0)
+ if (strncmp(ifp->if_xname, name, IFNAMSIZ) == 0)
return (ifp);
}
return (NULL);
Or are you worried about something completly different and i'm missing
the point?
>
> > I will send another patch to @tech to remove -carpdev from ifconfig(8)
> >
> > Cheers,
> > Fabian
> >
> >
> > Index: 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
> > --- ip_carp.c 19 Dec 2015 11:19:35 -0000 1.284
> > +++ ip_carp.c 30 Dec 2015 11:31:57 -0000
> > @@ -2022,8 +2022,7 @@ carp_ioctl(struct ifnet *ifp, u_long cmd
> > if ((error = copyin(ifr->ifr_data, &carpr, sizeof carpr)))
> > break;
> > error = 1;
> > - if (carpr.carpr_carpdev[0] != '\0' &&
> > - (ifp0 = ifunit(carpr.carpr_carpdev)) == NULL)
> > + if ((ifp0 = ifunit(carpr.carpr_carpdev)) == NULL)
> > return (EINVAL);
> > if (carpr.carpr_peer.s_addr == 0)
> > sc->sc_peer.s_addr = INADDR_CARP_GROUP;
> >
