From cb800e3529aa19d36565acda41c79cb0b19a1d3b Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Wed, 5 Dec 2018 11:50:25 -0200 Subject: [PATCH 1/3] zebra: always define ROUNDUP and ROUND_TYPE Move the declaration of ROUNDUP and ROUND_TYPE to outside of `ifdef SA_SIZE`. We'll use these definitions in the next commit. Signed-off-by: Rafael Zalamena --- zebra/kernel_socket.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c index dcc22d2162ff..25c6e6c64b67 100644 --- a/zebra/kernel_socket.c +++ b/zebra/kernel_socket.c @@ -64,17 +64,18 @@ extern struct zebra_privs_t zserv_privs; * 0). We follow this practice without questioning it, but it is a * bug if quagga calls ROUNDUP with 0. */ +#ifdef __APPLE__ +#define ROUNDUP_TYPE int +#else +#define ROUNDUP_TYPE long +#endif /* * Because of these varying conventions, the only sane approach is for * the header to define some flavor of ROUNDUP macro. */ -#if defined(SA_SIZE) -/* SAROUNDUP is the only thing we need, and SA_SIZE provides that */ -#define SAROUNDUP(a) SA_SIZE(a) -#else /* !SA_SIZE */ - +/* OS X (Xcode as of 2014-12) is known not to define RT_ROUNDUP */ #if defined(RT_ROUNDUP) #define ROUNDUP(a) RT_ROUNDUP(a) #endif /* defined(RT_ROUNDUP) */ @@ -96,20 +97,17 @@ extern struct zebra_privs_t zserv_privs; * have it in its headers, this will break rather obviously and you'll * have to fix it here. */ - -/* OS X (Xcode as of 2014-12) is known not to define RT_ROUNDUP */ -#ifdef __APPLE__ -#define ROUNDUP_TYPE int -#else -#define ROUNDUP_TYPE long -#endif - #define ROUNDUP(a) \ ((a) > 0 ? (1 + (((a)-1) | (sizeof(ROUNDUP_TYPE) - 1))) \ : sizeof(ROUNDUP_TYPE)) #endif /* defined(ROUNDUP) */ + +#if defined(SA_SIZE) +/* SAROUNDUP is the only thing we need, and SA_SIZE provides that */ +#define SAROUNDUP(a) SA_SIZE(a) +#else /* !SA_SIZE */ /* * Given a pointer (sockaddr or void *), return the number of bytes * taken up by the sockaddr and any padding needed for alignment. From 86c57af534173d417189404ae005a610d84ee155 Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Wed, 5 Dec 2018 11:51:25 -0200 Subject: [PATCH 2/3] zebra: refactor route socket message handling Some address types were not being skipped triggering a warning log message, so lets refactor this code to properly handle known and unknown types. Signed-off-by: Rafael Zalamena --- zebra/kernel_socket.c | 246 ++++++++++++++++++++++++------------------ 1 file changed, 143 insertions(+), 103 deletions(-) diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c index 25c6e6c64b67..8a7cb0e5284c 100644 --- a/zebra/kernel_socket.c +++ b/zebra/kernel_socket.c @@ -131,60 +131,6 @@ extern struct zebra_privs_t zserv_privs; #endif /* !SA_SIZE */ -/* - * We use a call to an inline function to copy (PNT) to (DEST) - * 1. Calculating the length of the copy requires an #ifdef to determine - * if sa_len is a field and can't be used directly inside a #define - * 2. So the compiler doesn't complain when DEST is NULL, which is only true - * when we are skipping the copy and incrementing to the next SA - */ -static inline void rta_copy(union sockunion *dest, caddr_t src) -{ - int len; - if (!dest) - return; -#ifdef HAVE_STRUCT_SOCKADDR_SA_LEN - len = (((struct sockaddr *)src)->sa_len > sizeof(*dest)) - ? sizeof(*dest) - : ((struct sockaddr *)src)->sa_len; -#else - len = (SAROUNDUP(src) > sizeof(*dest)) ? sizeof(*dest) : SAROUNDUP(src); -#endif - memcpy(dest, src, len); -} - -#define RTA_ADDR_GET(DEST, RTA, RTMADDRS, PNT) \ - if ((RTMADDRS) & (RTA)) { \ - int len = SAROUNDUP((PNT)); \ - if (af_check(((struct sockaddr *)(PNT))->sa_family)) \ - rta_copy((DEST), (PNT)); \ - (PNT) += len; \ - } -#define RTA_ATTR_GET(DEST, RTA, RTMADDRS, PNT) \ - if ((RTMADDRS) & (RTA)) { \ - int len = SAROUNDUP((PNT)); \ - rta_copy((DEST), (PNT)); \ - (PNT) += len; \ - } - -#define RTA_NAME_GET(DEST, RTA, RTMADDRS, PNT, LEN) \ - if ((RTMADDRS) & (RTA)) { \ - uint8_t *pdest = (uint8_t *)(DEST); \ - int len = SAROUNDUP((PNT)); \ - struct sockaddr_dl *sdl = (struct sockaddr_dl *)(PNT); \ - if (IS_ZEBRA_DEBUG_KERNEL) \ - zlog_debug("%s: RTA_SDL_GET nlen %d, alen %d", \ - __func__, sdl->sdl_nlen, sdl->sdl_alen); \ - if (((DEST) != NULL) && (sdl->sdl_family == AF_LINK) \ - && (sdl->sdl_nlen < IFNAMSIZ) && (sdl->sdl_nlen <= len)) { \ - memcpy(pdest, sdl->sdl_data, sdl->sdl_nlen); \ - pdest[sdl->sdl_nlen] = '\0'; \ - (LEN) = sdl->sdl_nlen; \ - } \ - (PNT) += len; \ - } else { \ - (LEN) = 0; \ - } /* Routing socket message types. */ const struct message rtm_type_str[] = {{RTM_ADD, "RTM_ADD"}, {RTM_DELETE, "RTM_DELETE"}, @@ -284,6 +230,9 @@ int dplane_routing_sock = -1; /* Yes I'm checking ugly routing socket behavior. */ /* #define DEBUG */ +size_t rta_get(caddr_t sap, void *dest, size_t destlen); +size_t rta_getsdlname(caddr_t sap, void *dest, short *destlen); + /* Supported address family check. */ static inline int af_check(int family) { @@ -294,6 +243,63 @@ static inline int af_check(int family) return 0; } +size_t rta_get(caddr_t sap, void *destp, size_t destlen) +{ + struct sockaddr *sa = (struct sockaddr *)sap; + uint8_t *dest = destp; + size_t tlen, copylen; + +#ifdef HAVE_STRUCT_SOCKADDR_SA_LEN + copylen = sa->sa_len; + tlen = (copylen == 0) ? sizeof(ROUNDUP_TYPE) : ROUNDUP(copylen); +#else /* !HAVE_STRUCT_SOCKADDR_SA_LEN */ + copylen = tlen = SAROUNDUP(sap); +#endif /* !HAVE_STRUCT_SOCKADDR_SA_LEN */ + + if (copylen > 0 && dest != NULL && af_check(sa->sa_family)) { + if (copylen > destlen) { + zlog_warn("%s: destination buffer too small (%lu vs %lu)", + __func__, copylen, destlen); + memcpy(dest, sap, destlen); + } else + memcpy(dest, sap, copylen); + } + + return tlen; +} + +size_t rta_getsdlname(caddr_t sap, void *destp, short *destlen) +{ + struct sockaddr_dl *sdl = (struct sockaddr_dl *)sap; + struct sockaddr *sa = (struct sockaddr *)sap; + uint8_t *dest = destp; + size_t tlen, copylen; + + copylen = sdl->sdl_nlen; +#ifdef HAVE_STRUCT_SOCKADDR_SA_LEN + tlen = (sa->sa_len == 0) ? sizeof(ROUNDUP_TYPE) : ROUNDUP(sa->sa_len); +#else /* !HAVE_STRUCT_SOCKADDR_SA_LEN */ + tlen = SAROUNDUP(sap); +#endif /* !HAVE_STRUCT_SOCKADDR_SA_LEN */ + + if (copylen > 0 && dest != NULL && sdl->sdl_family == AF_LINK) { + if (copylen > IFNAMSIZ) { + zlog_warn("%s: destination buffer too small (%lu vs %d)", + __func__, copylen, IFNAMSIZ); + memcpy(dest, sdl->sdl_data, IFNAMSIZ); + dest[IFNAMSIZ] = 0; + *destlen = IFNAMSIZ; + } else { + memcpy(dest, sdl->sdl_data, copylen); + dest[copylen] = 0; + *destlen = copylen; + } + } else + *destlen = 0; + + return tlen; +} + /* Dump routing table flag for debug purpose. */ static void rtm_flag_dump(int flag) { @@ -406,6 +412,7 @@ int ifm_read(struct if_msghdr *ifm) struct sockaddr_dl *sdl; char ifname[IFNAMSIZ]; short ifnlen = 0; + int maskbit; caddr_t cp; /* terminate ifname at head (for strnlen) and tail (for safety) */ @@ -437,21 +444,19 @@ int ifm_read(struct if_msghdr *ifm) cp = cp + 12; #endif - RTA_ADDR_GET(NULL, RTA_DST, ifm->ifm_addrs, cp); - RTA_ADDR_GET(NULL, RTA_GATEWAY, ifm->ifm_addrs, cp); - RTA_ATTR_GET(NULL, RTA_NETMASK, ifm->ifm_addrs, cp); - RTA_ADDR_GET(NULL, RTA_GENMASK, ifm->ifm_addrs, cp); - sdl = (struct sockaddr_dl *)cp; - RTA_NAME_GET(ifname, RTA_IFP, ifm->ifm_addrs, cp, ifnlen); - RTA_ADDR_GET(NULL, RTA_IFA, ifm->ifm_addrs, cp); - RTA_ADDR_GET(NULL, RTA_AUTHOR, ifm->ifm_addrs, cp); - RTA_ADDR_GET(NULL, RTA_BRD, ifm->ifm_addrs, cp); -#ifdef RTA_LABEL - RTA_ATTR_GET(NULL, RTA_LABEL, ifm->ifm_addrs, cp); -#endif -#ifdef RTA_SRC - RTA_ADDR_GET(NULL, RTA_SRC, ifm->ifm_addrs, cp); -#endif + /* Look up for RTA_IFP and skip others. */ + for (maskbit = 1; maskbit; maskbit <<= 1) { + if ((maskbit & ifm->ifm_addrs) == 0) + continue; + if (maskbit != RTA_IFP) { + cp += rta_get(cp, NULL, 0); + continue; + } + + /* Save the pointer to the structure. */ + sdl = (struct sockaddr_dl *)cp; + cp += rta_getsdlname(cp, ifname, &ifnlen); + } if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug("%s: sdl ifname %s", __func__, @@ -558,7 +563,7 @@ int ifm_read(struct if_msghdr *ifm) * - Solaris has no sdl_len, but sdl_data[244] * presumably, it's not going to run past that, so sizeof() * is fine here. - * a nonzero ifnlen from RTA_NAME_GET() means sdl is valid + * a nonzero ifnlen from rta_getsdlname() means sdl is valid */ ifp->ll_type = ZEBRA_LLT_UNKNOWN; ifp->hw_addr_len = 0; @@ -652,6 +657,7 @@ static void ifam_read_mesg(struct ifa_msghdr *ifm, union sockunion *addr, caddr_t pnt, end; union sockunion dst; union sockunion gateway; + int maskbit; pnt = (caddr_t)(ifm + 1); end = ((caddr_t)ifm) + ifm->ifam_msglen; @@ -664,20 +670,41 @@ static void ifam_read_mesg(struct ifa_msghdr *ifm, union sockunion *addr, memset(&gateway, 0, sizeof(union sockunion)); /* We fetch each socket variable into sockunion. */ - RTA_ADDR_GET(&dst, RTA_DST, ifm->ifam_addrs, pnt); - RTA_ADDR_GET(&gateway, RTA_GATEWAY, ifm->ifam_addrs, pnt); - RTA_ATTR_GET(mask, RTA_NETMASK, ifm->ifam_addrs, pnt); - RTA_ADDR_GET(NULL, RTA_GENMASK, ifm->ifam_addrs, pnt); - RTA_NAME_GET(ifname, RTA_IFP, ifm->ifam_addrs, pnt, *ifnlen); - RTA_ADDR_GET(addr, RTA_IFA, ifm->ifam_addrs, pnt); - RTA_ADDR_GET(NULL, RTA_AUTHOR, ifm->ifam_addrs, pnt); - RTA_ADDR_GET(brd, RTA_BRD, ifm->ifam_addrs, pnt); -#ifdef RTA_LABEL - RTA_ATTR_GET(NULL, RTA_LABEL, ifm->ifam_addrs, pnt); -#endif -#ifdef RTA_SRC - RTA_ADDR_GET(NULL, RTA_SRC, ifm->ifam_addrs, pnt); -#endif + for (maskbit = 1; maskbit; maskbit <<= 1) { + if ((maskbit & ifm->ifam_addrs) == 0) + continue; + + switch (maskbit) { + case RTA_DST: + pnt += rta_get(pnt, &dst, sizeof(dst)); + break; + case RTA_GATEWAY: + pnt += rta_get(pnt, &gateway, sizeof(gateway)); + break; + case RTA_NETMASK: + pnt += rta_get(pnt, mask, sizeof(*mask)); + break; + case RTA_IFP: + pnt += rta_getsdlname(pnt, ifname, ifnlen); + break; + case RTA_IFA: + pnt += rta_get(pnt, addr, sizeof(*addr)); + break; + case RTA_BRD: + pnt += rta_get(pnt, brd, sizeof(*brd)); + break; + + default: + pnt += rta_get(pnt, NULL, 0); + break; + } + + if (pnt > end) { + zlog_warn("%s: overflow detected (pnt:%p end:%p)", + __func__, pnt, end); + break; + } + } if (IS_ZEBRA_DEBUG_KERNEL) { int family = sockunion_family(addr); @@ -712,6 +739,7 @@ static void ifam_read_mesg(struct ifa_msghdr *ifm, union sockunion *addr, } /* Assert read up end point matches to end point */ + pnt = (caddr_t)ROUNDUP((size_t)pnt); if (pnt != end) zlog_debug("ifam_read() doesn't read all socket data"); } @@ -820,6 +848,7 @@ static int rtm_read_mesg(struct rt_msghdr *rtm, union sockunion *dest, char *ifname, short *ifnlen) { caddr_t pnt, end; + int maskbit; /* Pnt points out socket data start point. */ pnt = (caddr_t)(rtm + 1); @@ -838,25 +867,36 @@ static int rtm_read_mesg(struct rt_msghdr *rtm, union sockunion *dest, memset(mask, 0, sizeof(union sockunion)); /* We fetch each socket variable into sockunion. */ - RTA_ADDR_GET(dest, RTA_DST, rtm->rtm_addrs, pnt); - RTA_ADDR_GET(gate, RTA_GATEWAY, rtm->rtm_addrs, pnt); - RTA_ATTR_GET(mask, RTA_NETMASK, rtm->rtm_addrs, pnt); - RTA_ADDR_GET(NULL, RTA_GENMASK, rtm->rtm_addrs, pnt); - RTA_NAME_GET(ifname, RTA_IFP, rtm->rtm_addrs, pnt, *ifnlen); - RTA_ADDR_GET(NULL, RTA_IFA, rtm->rtm_addrs, pnt); - RTA_ADDR_GET(NULL, RTA_AUTHOR, rtm->rtm_addrs, pnt); - RTA_ADDR_GET(NULL, RTA_BRD, rtm->rtm_addrs, pnt); -#ifdef RTA_LABEL -#if 0 - union sockunion label; - memset(&label, 0, sizeof(label)); - RTA_ATTR_GET(&label, RTA_LABEL, rtm->rtm_addrs, pnt); -#endif - RTA_ATTR_GET(NULL, RTA_LABEL, rtm->rtm_addrs, pnt); -#endif -#ifdef RTA_SRC - RTA_ADDR_GET(NULL, RTA_SRC, rtm->rtm_addrs, pnt); -#endif + /* We fetch each socket variable into sockunion. */ + for (maskbit = 1; maskbit; maskbit <<= 1) { + if ((maskbit & rtm->rtm_addrs) == 0) + continue; + + switch (maskbit) { + case RTA_DST: + pnt += rta_get(pnt, dest, sizeof(*dest)); + break; + case RTA_GATEWAY: + pnt += rta_get(pnt, gate, sizeof(*gate)); + break; + case RTA_NETMASK: + pnt += rta_get(pnt, mask, sizeof(*mask)); + break; + case RTA_IFP: + pnt += rta_getsdlname(pnt, ifname, ifnlen); + break; + + default: + pnt += rta_get(pnt, NULL, 0); + break; + } + + if (pnt > end) { + zlog_warn("%s: overflow detected (pnt:%p end:%p)", + __func__, pnt, end); + break; + } + } /* If there is netmask information set it's family same as destination family*/ From c69f2c1fffbae8d0bd5b69c627c4217d6ce97aed Mon Sep 17 00:00:00 2001 From: Rafael Zalamena Date: Wed, 5 Dec 2018 17:22:56 -0200 Subject: [PATCH 3/3] zebra: don't log errors on unsupported medias When using `SIOCGIFMEDIA` check for `EINVAL`, otherwise we might print an error message on an unsupported interface. FreeBSD source code reference: https://github.com/freebsd/freebsd/blob/master/sys/net/if_media.c#L300 And: https://github.com/freebsd/freebsd/blob/8cb4b0c0181bd45318ee8977f77aea90c53bb224/usr.sbin/rtsold/if.c#L211 /* * EINVAL simply means that the interface does not support * the SIOCGIFMEDIA ioctl. We regard it alive. */ Signed-off-by: Rafael Zalamena --- zebra/ioctl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/zebra/ioctl.c b/zebra/ioctl.c index 87e98032a21e..ebe1edcaef0a 100644 --- a/zebra/ioctl.c +++ b/zebra/ioctl.c @@ -415,7 +415,8 @@ void if_get_flags(struct interface *ifp) strncpy(ifmr.ifm_name, ifp->name, IFNAMSIZ); /* Seems not all interfaces implement this ioctl */ - if (if_ioctl(SIOCGIFMEDIA, (caddr_t)&ifmr) < 0) + if (if_ioctl(SIOCGIFMEDIA, (caddr_t)&ifmr) == -1 && + errno != EINVAL) flog_err_sys(EC_LIB_SYSTEM_CALL, "if_ioctl(SIOCGIFMEDIA) failed: %s", safe_strerror(errno));