From owner-dev-commits-src-main@freebsd.org Tue Feb 16 21:52:54 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 7D6E854BC73; Tue, 16 Feb 2021 21:52:54 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from forward500j.mail.yandex.net (forward500j.mail.yandex.net [5.45.198.250]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4DgF861k5Xz3m2K; Tue, 16 Feb 2021 21:52:54 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from iva4-c9485a9ab4a2.qloud-c.yandex.net (iva4-c9485a9ab4a2.qloud-c.yandex.net [IPv6:2a02:6b8:c0c:788c:0:640:c948:5a9a]) by forward500j.mail.yandex.net (Yandex) with ESMTP id 8762F11C1B74; Wed, 17 Feb 2021 00:52:51 +0300 (MSK) Received: from localhost (localhost [::1]) by iva4-c9485a9ab4a2.qloud-c.yandex.net (mxback/Yandex) with ESMTP id TzR8VoIp0b-qoISaQMC; Wed, 17 Feb 2021 00:52:50 +0300 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfw.ru; s=mail; t=1613512370; bh=iOmIVOzM0KYgfQgfJVw1TTdkO0vTyCREHPriluqsVrc=; h=Message-Id:Cc:Subject:In-Reply-To:Date:References:To:From; b=qxfjAAfv+AtRcig0tH4JLP+UQIllDvF9GzaOltCepGDh3wsKSkpBUSLNqgSW9yBkD UEByevy6VVyaSFEMphel4fOUA613Hnwy++OMRR0scUz9ByhLa0RyFIO6BS1Q2hOKXW v9ljBtfeqLX4+vYhVD3JPXQLcxc+LVa0VS/QZhgc= Received: by iva4-74e42d77d21f.qloud-c.yandex.net with HTTP; Wed, 17 Feb 2021 00:52:50 +0300 From: Alexander V. Chernikov Envelope-From: melifaro@ipfw.ru To: Mateusz Guzik Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" In-Reply-To: References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> <290641613510357@mail.yandex.ru> Subject: Re: git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code. MIME-Version: 1.0 X-Mailer: Yamail [ http://yandex.ru ] 5.0 Date: Tue, 16 Feb 2021 21:52:50 +0000 Message-Id: <276041613512339@mail.yandex.ru> Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=utf-8 X-Rspamd-Queue-Id: 4DgF861k5Xz3m2K X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Feb 2021 21:52:54 -0000 16.02.2021, 21:47, "Mateusz Guzik" : > 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 wrote: >>  16.02.2021, 20:43, "Mateusz Guzik" : >>>  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 wrote: >>>>   The branch main has been updated by melifaro: >>>> >>>>   URL: >>>>   https://cgit.FreeBSD.org/src/commit/?id=2fe5a79425c79f7b828acd91da66d97230925fc8 >>>> >>>>   commit 2fe5a79425c79f7b828acd91da66d97230925fc8 >>>>   Author: Alexander V. Chernikov >>>>   AuthorDate: 2021-02-16 20:30:04 +0000 >>>>   Commit: Alexander V. Chernikov >>>>   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<..>>>  allow >>>>   only actually >>>>        supported ones (inet, inet6, link). >>>>       * Automatically convert `sockaddr_sdl` (AF_LINK) gateways to >>>>         `sockaddr_sdl_short`. >>>> >>>>       Reported by: Guy Yur >>>>       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 >>>>    #include >>>>    #ifdef INET6 >>>>   +#include >>>>    #include >>>>    #include >>>>    #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 > > -- > Mateusz Guzik