From owner-dev-commits-src-all@freebsd.org Tue Feb 16 21:26:00 2021 Return-Path: Delivered-To: dev-commits-src-all@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 0204B54AD0E; Tue, 16 Feb 2021 21:26:00 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from forward500o.mail.yandex.net (forward500o.mail.yandex.net [37.140.190.195]) (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 4DgDY35Fkqz3hyr; Tue, 16 Feb 2021 21:25:59 +0000 (UTC) (envelope-from melifaro@ipfw.ru) Received: from iva8-4db386e07e2e.qloud-c.yandex.net (iva8-4db386e07e2e.qloud-c.yandex.net [IPv6:2a02:6b8:c0c:951e:0:640:4db3:86e0]) by forward500o.mail.yandex.net (Yandex) with ESMTP id 1B69B602F2; Wed, 17 Feb 2021 00:25:56 +0300 (MSK) Received: from localhost (localhost [::1]) by iva8-4db386e07e2e.qloud-c.yandex.net (mxback/Yandex) with ESMTP id Ado3fq6C93-PtI0URU6; Wed, 17 Feb 2021 00:25:55 +0300 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfw.ru; s=mail; t=1613510755; bh=yq4ZGXvA2xFholqixI1Px9ZlVOnv3qYqqYchFnTV4Fg=; h=Message-Id:Cc:Subject:In-Reply-To:Date:References:To:From; b=TqJbPfbRHnUE2EN9OfmW02z+utrMjkQfq3d/V3csjko15Z+4a1A4KGxm4zK4w30sc 65uFkhDBxNWLpHGu9ei2zyIF/UBGK4As/RFAv5An2m9LHZ1B+Uspb92pQa+4IFvvnn p9YJjboraXw8Rpbsp+zZy9iFY6AkSfzKiuPVwJ7E= Received: by iva2-13089525268d.qloud-c.yandex.net with HTTP; Wed, 17 Feb 2021 00:25:55 +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> 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:25:55 +0000 Message-Id: <290641613510357@mail.yandex.ru> Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=utf-8 X-Rspamd-Queue-Id: 4DgDY35Fkqz3hyr 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-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Feb 2021 21:26:00 -0000 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<..>  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