From owner-dev-commits-src-all@freebsd.org Tue Feb 16 20:43:20 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 5AAC954959B; Tue, 16 Feb 2021 20:43:20 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DgCbr1lR6z3Ncy; Tue, 16 Feb 2021 20:43:19 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x42b.google.com with SMTP id n6so14922569wrv.8; Tue, 16 Feb 2021 12:43:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=n1eVVOpf1nj4nNZN5WRWM+t81XvwzUdawNfx60pC99k=; b=dtqxci08VlV+uatZayGC5Z1XkFm3uzzupScvLNDo376hvEPvoOCTC/1isGBovMjny1 3dWHpySQqnrPNx7WGl1JvuC61xAlwd9y0zymO9zJChpAxdKLpF3FqVqoYVTEXH2PxyxK wICPk8Zdmg13A3gdBMMbo/RvpZXzpxR1/jYgbLhGMPXT47kZCRITr7Va/Ql1I6PqUHby sIa15ZGwJI2lKaTgpHAwE3hLnOsBYme8y9x8LEBUY0gHx4uNlmFbG5J2puB+oR88ubeR 4pfOYRfeITNlWWmf8sQ21+zt7yVBH3h0/YK5BZcFRC7HVm9MyqXzbpgule9uBBvmOFzR f6IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=n1eVVOpf1nj4nNZN5WRWM+t81XvwzUdawNfx60pC99k=; b=Qx2lLQl1A7JwhPto9P52KQLydHiuP1um5Mb0pXE4Wfl/CnJ7mOAuiYseFMbqdy1Qex iNZb7v9s4Ww+q7cxFjOMOWe0nD4kP8XdENfAPSiUMC/Ooju2AnL/WuhOEjGTy+FcbAP5 Z7j7hS44GGhogQ0KIa8Hd1DVUvlQuKIEaHSYXzjHwD+63fTMweGOgKlc9+3rwkl4qap2 2BvU4WTD54FbTR70bixdGmLPBnxH5/FIy4/BMABx9rSkrLH8heQJQd6jZR+rWimdts4o SfOolInFGwknmsEACQJZANAh2P7MWbzEDFGNmPro8cOP2ksVDCLe3aVwTW/o/SQwgfpt O6Fg== X-Gm-Message-State: AOAM531QP4PKJwO8jiwH6Hze6GTBAwUQoIa2wbcVIjh58np9zc0vlSQH N0gonDG1Iee5d4+0fz9Q0wkI+llbVh812FH9mE6uZ0HR X-Google-Smtp-Source: ABdhPJwJDIt6NQWT2vsr7KQUO8Dyj9hTtWZhAZOAk+z8JCro0cQMrZWu8yf0aJlS6MLiebMaJN/ZRe+vhP+3MbFfaNU= X-Received: by 2002:a05:6000:1565:: with SMTP id 5mr25728330wrz.109.1613508198388; Tue, 16 Feb 2021 12:43:18 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:e406:0:0:0:0:0 with HTTP; Tue, 16 Feb 2021 12:43:17 -0800 (PST) In-Reply-To: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> From: Mateusz Guzik Date: Tue, 16 Feb 2021 21:43:17 +0100 Message-ID: Subject: Re: git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code. To: "Alexander V. Chernikov" Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4DgCbr1lR6z3Ncy 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 20:43:20 -0000 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 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