Date: Tue, 16 Feb 2021 21:25:55 +0000 From: Alexander V. Chernikov <melifaro@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code. Message-ID: <290641613510357@mail.yandex.ru> In-Reply-To: <CAGudoHHc_z6m5vRhi57zqN8%2BMJzT65tGnddzC7k=1s83ncUa5g@mail.gmail.com> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> <CAGudoHHc_z6m5vRhi57zqN8%2BMJzT65tGnddzC7k=1s83ncUa5g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
16.02.2021, 20:43, "Mateusz Guzik" <mjguzik@gmail.com>: > This breaks the built at least without INET6. It would help if you could share the actual build error. I see -Wunused for 2 function (which I will fix soon), but I'm not sure if that's the error you're running into. > > can you please start testing your patches on NOINET kernels Well, it actually builds for me: -------------------------------------------------------------- >>> Kernel build for LINT-NOINET completed on Tue Feb 16 21:21:39 UTC 2021 -------------------------------------------------------------- >>> Kernel(s) LINT-NOINET built in 28 seconds, ncpu: 2, make -j6 -------------------------------------------------------------- > > On 2/16/21, Alexander V. Chernikov <melifaro@freebsd.org> wrote: >> The branch main has been updated by melifaro: >> >> URL: >> https://cgit.FreeBSD.org/src/commit/?id=2fe5a79425c79f7b828acd91da66d97230925fc8 >> >> commit 2fe5a79425c79f7b828acd91da66d97230925fc8 >> Author: Alexander V. Chernikov <melifaro@FreeBSD.org> >> AuthorDate: 2021-02-16 20:30:04 +0000 >> Commit: Alexander V. Chernikov <melifaro@FreeBSD.org> >> CommitDate: 2021-02-16 20:30:04 +0000 >> >> Fix dst/netmask handling in routing socket code. >> >> Traditionally routing socket code did almost zero checks on >> the input message except for the most basic size checks. >> >> This resulted in the unclear KPI boundary for the routing system code >> (`rtrequest*` and now `rib_action()`) w.r.t message validness. >> >> Multiple potential problems and nuances exists: >> * Host bits in RTAX_DST sockaddr. Existing applications do send >> prefixes >> with hostbits uncleared. Even `route(8)` does this, as they hope the >> kernel >> would do the job of fixing it. Code inside `rib_action()` needs to >> handle >> it on its own (see `rt_maskedcopy()` ugly hack). >> * There are multiple way of adding the host route: it can be DST >> without >> netmask or DST with /32(/128) netmask. Also, RTF_HOST has to be set >> correspondingly. >> Currently, these 2 options create 2 DIFFERENT routes in the kernel. >> * no sockaddr length/content checking for the "secondary" fields exists: >> nothing >> stops rtsock application to send sockaddr_in with length of 25 (instead >> of 16). >> Kernel will accept it, install to RIB as is and propagate to all rtsock >> consumers, >> potentially triggering bugs in their code. Same goes for sin_port, >> sin_zero, etc. >> >> The goal of this change is to make rtsock verify all sockaddr and prefix >> consistency. >> Said differently, `rib_action()` or internals should NOT require to >> change any of the >> sockaddrs supplied by `rt_addrinfo` structure due to incorrectness. >> >> To be more specific, this change implements the following: >> * sockaddr cleanup/validation check is added immediately after getting >> sockaddrs from rtm. >> * Per-family dst/netmask checks clears host bits in dst and zeros all >> dst/netmask "secondary" fields. >> * The same netmask checking code converts /32(/128) netmasks to "host" >> route case >> (NULL netmask, RTF_HOST), removing the dualism. >> * Instead of allowing ANY "known" sockaddr families (0<..<AF_MAX), allow >> only actually >> supported ones (inet, inet6, link). >> * Automatically convert `sockaddr_sdl` (AF_LINK) gateways to >> `sockaddr_sdl_short`. >> >> Reported by: Guy Yur <guyyur at gmail.com> >> Reviewed By: donner >> Differential Revision: https://reviews.freebsd.org/D28668 >> MFC after: 3 days >> --- >> sys/net/rtsock.c | 201 >> +++++++++++++++++++++++++++++++++- >> tests/sys/net/routing/rtsock_common.h | 4 - >> 2 files changed, 195 insertions(+), 10 deletions(-) >> >> diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c >> index 3a98b366dfc3..40ce62c77c2a 100644 >> --- a/sys/net/rtsock.c >> +++ b/sys/net/rtsock.c >> @@ -70,6 +70,7 @@ >> #include <netinet/if_ether.h> >> #include <netinet/ip_carp.h> >> #ifdef INET6 >> +#include <netinet6/in6_var.h> >> #include <netinet6/ip6_var.h> >> #include <netinet6/scope6_var.h> >> #endif >> @@ -173,6 +174,7 @@ static int rtsock_msg_buffer(int type, struct >> rt_addrinfo *rtinfo, >> struct walkarg *w, int *plen); >> static int rt_xaddrs(caddr_t cp, caddr_t cplim, >> struct rt_addrinfo *rtinfo); >> +static int cleanup_xaddrs(struct rt_addrinfo *info); >> static int sysctl_dumpentry(struct rtentry *rt, void *vw); >> static int sysctl_dumpnhop(struct rtentry *rt, struct nhop_object *nh, >> uint32_t weight, struct walkarg *w); >> @@ -636,11 +638,9 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, u_int >> fibnum, struct rt_addrinfo * >> return (EINVAL); >> >> info->rti_flags = rtm->rtm_flags; >> - if (info->rti_info[RTAX_DST] == NULL || >> - info->rti_info[RTAX_DST]->sa_family >= AF_MAX || >> - (info->rti_info[RTAX_GATEWAY] != NULL && >> - info->rti_info[RTAX_GATEWAY]->sa_family >= AF_MAX)) >> - return (EINVAL); >> + error = cleanup_xaddrs(info); >> + if (error != 0) >> + return (error); >> saf = info->rti_info[RTAX_DST]->sa_family; >> /* >> * Verify that the caller has the appropriate privilege; RTM_GET >> @@ -739,7 +739,14 @@ handle_rtm_get(struct rt_addrinfo *info, u_int fibnum, >> >> RIB_RLOCK(rnh); >> >> - if (info->rti_info[RTAX_NETMASK] == NULL) { >> + /* >> + * By (implicit) convention host route (one without netmask) >> + * means longest-prefix-match request and the route with netmask >> + * means exact-match lookup. >> + * As cleanup_xaddrs() cleans up info flags&addrs for the /32,/128 >> + * prefixes, use original data to check for the netmask presence. >> + */ >> + if ((rtm->rtm_addrs & RTA_NETMASK) == 0) { >> /* >> * Provide longest prefix match for >> * address lookup (no mask). >> @@ -1286,6 +1293,188 @@ rt_xaddrs(caddr_t cp, caddr_t cplim, struct >> rt_addrinfo *rtinfo) >> return (0); >> } >> >> +static inline void >> +fill_sockaddr_inet(struct sockaddr_in *sin, struct in_addr addr) >> +{ >> + >> + const struct sockaddr_in nsin = { >> + .sin_family = AF_INET, >> + .sin_len = sizeof(struct sockaddr_in), >> + .sin_addr = addr, >> + }; >> + *sin = nsin; >> +} >> + >> +static inline void >> +fill_sockaddr_inet6(struct sockaddr_in6 *sin6, const struct in6_addr >> *addr6, >> + uint32_t scopeid) >> +{ >> + >> + const struct sockaddr_in6 nsin6 = { >> + .sin6_family = AF_INET6, >> + .sin6_len = sizeof(struct sockaddr_in6), >> + .sin6_addr = *addr6, >> + .sin6_scope_id = scopeid, >> + }; >> + *sin6 = nsin6; >> +} >> + >> +static int >> +cleanup_xaddrs_gateway(struct rt_addrinfo *info) >> +{ >> + struct sockaddr *gw = info->rti_info[RTAX_GATEWAY]; >> + >> + switch (gw->sa_family) { >> +#ifdef INET >> + case AF_INET: >> + { >> + struct sockaddr_in *gw_sin = (struct sockaddr_in *)gw; >> + if (gw_sin->sin_len < sizeof(struct sockaddr_in)) { >> + printf("gw sin_len too small\n"); >> + return (EINVAL); >> + } >> + fill_sockaddr_inet(gw_sin, gw_sin->sin_addr); >> + } >> + break; >> +#endif >> +#ifdef INET6 >> + case AF_INET6: >> + { >> + struct sockaddr_in6 *gw_sin6 = (struct sockaddr_in6 *)gw; >> + if (gw_sin6->sin6_len < sizeof(struct sockaddr_in6)) { >> + printf("gw sin6_len too small\n"); >> + return (EINVAL); >> + } >> + fill_sockaddr_inet6(gw_sin6, &gw_sin6->sin6_addr, 0); >> + break; >> + } >> +#endif >> + case AF_LINK: >> + { >> + struct sockaddr_dl_short *gw_sdl; >> + >> + gw_sdl = (struct sockaddr_dl_short *)gw; >> + if (gw_sdl->sdl_len < sizeof(struct sockaddr_dl_short)) { >> + printf("gw sdl_len too small\n"); >> + return (EINVAL); >> + } >> + >> + const struct sockaddr_dl_short sdl = { >> + .sdl_family = AF_LINK, >> + .sdl_len = sizeof(struct sockaddr_dl_short), >> + .sdl_index = gw_sdl->sdl_index, >> + }; >> + *gw_sdl = sdl; >> + break; >> + } >> + } >> + >> + return (0); >> +} >> + >> +static int >> +cleanup_xaddrs_inet(struct rt_addrinfo *info) >> +{ >> + struct sockaddr_in *dst_sa, *mask_sa; >> + >> + /* Check & fixup dst/netmask combination first */ >> + dst_sa = (struct sockaddr_in *)info->rti_info[RTAX_DST]; >> + mask_sa = (struct sockaddr_in *)info->rti_info[RTAX_NETMASK]; >> + >> + struct in_addr mask = { >> + .s_addr = mask_sa ? mask_sa->sin_addr.s_addr : INADDR_BROADCAST, >> + }; >> + struct in_addr dst = { >> + .s_addr = htonl(ntohl(dst_sa->sin_addr.s_addr) & ntohl(mask.s_addr)) >> + }; >> + >> + if (dst_sa->sin_len < sizeof(struct sockaddr_in)) { >> + printf("dst sin_len too small\n"); >> + return (EINVAL); >> + } >> + if (mask_sa && mask_sa->sin_len < sizeof(struct sockaddr_in)) { >> + printf("mask sin_len too small\n"); >> + return (EINVAL); >> + } >> + fill_sockaddr_inet(dst_sa, dst); >> + >> + if (mask.s_addr != INADDR_BROADCAST) >> + fill_sockaddr_inet(mask_sa, mask); >> + else { >> + info->rti_info[RTAX_NETMASK] = NULL; >> + info->rti_flags |= RTF_HOST; >> + info->rti_addrs &= ~RTA_NETMASK; >> + } >> + >> + /* Check gateway */ >> + if (info->rti_info[RTAX_GATEWAY] != NULL) >> + return (cleanup_xaddrs_gateway(info)); >> + >> + return (0); >> +} >> + >> +static int >> +cleanup_xaddrs_inet6(struct rt_addrinfo *info) >> +{ >> + struct sockaddr_in6 *dst_sa, *mask_sa; >> + struct in6_addr mask; >> + >> + /* Check & fixup dst/netmask combination first */ >> + dst_sa = (struct sockaddr_in6 *)info->rti_info[RTAX_DST]; >> + mask_sa = (struct sockaddr_in6 *)info->rti_info[RTAX_NETMASK]; >> + >> + mask = mask_sa ? mask_sa->sin6_addr : in6mask128; >> + IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); >> + >> + if (dst_sa->sin6_len < sizeof(struct sockaddr_in6)) { >> + printf("dst sin6_len too small\n"); >> + return (EINVAL); >> + } >> + if (mask_sa && mask_sa->sin6_len < sizeof(struct sockaddr_in6)) { >> + printf("mask sin6_len too small\n"); >> + return (EINVAL); >> + } >> + fill_sockaddr_inet6(dst_sa, &dst_sa->sin6_addr, 0); >> + >> + if (!IN6_ARE_ADDR_EQUAL(&mask, &in6mask128)) >> + fill_sockaddr_inet6(mask_sa, &mask, 0); >> + else { >> + info->rti_info[RTAX_NETMASK] = NULL; >> + info->rti_flags |= RTF_HOST; >> + info->rti_addrs &= ~RTA_NETMASK; >> + } >> + >> + /* Check gateway */ >> + if (info->rti_info[RTAX_GATEWAY] != NULL) >> + return (cleanup_xaddrs_gateway(info)); >> + >> + return (0); >> +} >> + >> +static int >> +cleanup_xaddrs(struct rt_addrinfo *info) >> +{ >> + int error = EAFNOSUPPORT; >> + >> + if (info->rti_info[RTAX_DST] == NULL) >> + return (EINVAL); >> + >> + switch (info->rti_info[RTAX_DST]->sa_family) { >> +#ifdef INET >> + case AF_INET: >> + error = cleanup_xaddrs_inet(info); >> + break; >> +#endif >> +#ifdef INET6 >> + case AF_INET6: >> + error = cleanup_xaddrs_inet6(info); >> + break; >> +#endif >> + } >> + >> + return (error); >> +} >> + >> /* >> * Fill in @dmask with valid netmask leaving original @smask >> * intact. Mostly used with radix netmasks. >> diff --git a/tests/sys/net/routing/rtsock_common.h >> b/tests/sys/net/routing/rtsock_common.h >> index 7da88e0eb512..71476d2b5f3c 100644 >> --- a/tests/sys/net/routing/rtsock_common.h >> +++ b/tests/sys/net/routing/rtsock_common.h >> @@ -826,10 +826,6 @@ _validate_message_sockaddrs(char *buffer, int rtm_len, >> size_t offset, int rtm_ad >> } >> sa = (struct sockaddr *)((char *)sa + SA_SIZE(sa)); >> } >> - >> - RTSOCK_ATF_REQUIRE_MSG((struct rt_msghdr *)buffer, parsed_len == rtm_len, >> - "message len != parsed len: expected %d parsed %d", >> - rtm_len, (int)parsed_len); >> } >> >> /* >> _______________________________________________ >> dev-commits-src-all@freebsd.org mailing list >> https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all >> To unsubscribe, send any mail to >> "dev-commits-src-all-unsubscribe@freebsd.org" > > -- > Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?290641613510357>