Date: Tue, 16 Feb 2021 21:43:17 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: "Alexander V. Chernikov" <melifaro@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code. Message-ID: <CAGudoHHc_z6m5vRhi57zqN8%2BMJzT65tGnddzC7k=1s83ncUa5g@mail.gmail.com> In-Reply-To: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This breaks the built at least without INET6. can you please start testing your patches on NOINET kernels 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?CAGudoHHc_z6m5vRhi57zqN8%2BMJzT65tGnddzC7k=1s83ncUa5g>