Date: Tue, 16 Feb 2021 21:52:50 +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: <276041613512339@mail.yandex.ru> In-Reply-To: <CAGudoHHxz-=nvDEZReEi%2BkJntTVVaWR9wtNLYAwhwHA4F9xq6g@mail.gmail.com> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> <CAGudoHHc_z6m5vRhi57zqN8%2BMJzT65tGnddzC7k=1s83ncUa5g@mail.gmail.com> <290641613510357@mail.yandex.ru> <CAGudoHHxz-=nvDEZReEi%2BkJntTVVaWR9wtNLYAwhwHA4F9xq6g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
16.02.2021, 21:47, "Mateusz Guzik" <mjguzik@gmail.com>: > In this context I meant NOINET and NOINET6 > > Anyhow I see the following: > sys/net/rtsock.c:1427:2: error: implicit declaration of function > 'IN6_MASK_ADDR' is invalid in C99 > [-Werror,-Wimplicit-function-declaration] > IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); Should be fixed by a4513bace0e0. Sorry for the breakage. > ^ > > On 2/16/21, Alexander V. Chernikov <melifaro@freebsd.org> wrote: >> 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> > > -- > Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?276041613512339>