From owner-dev-commits-src-main@freebsd.org Fri Feb 19 16:11:34 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 6CD6C531D1D; Fri, 19 Feb 2021 16:11:34 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DhxQt2jkpz4ftC; Fri, 19 Feb 2021 16:11:34 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.codepro.be", Issuer "R3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id 0633E22EBB; Fri, 19 Feb 2021 16:11:34 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: by venus.codepro.be (Postfix, authenticated sender kp) id 858344E611; Fri, 19 Feb 2021 17:11:32 +0100 (CET) From: "Kristof Provost" To: "Alexander V. Chernikov" 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. Date: Fri, 19 Feb 2021 17:11:31 +0100 X-Mailer: MailMate (1.13.2r5673) Message-ID: <471C6715-4F53-4DE7-9233-FC1EDEE49E2D@FreeBSD.org> In-Reply-To: <26E2BA35-291E-4DC5-BDCB-D98347D7E24C@FreeBSD.org> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> <26E2BA35-291E-4DC5-BDCB-D98347D7E24C@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.34 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: Fri, 19 Feb 2021 16:11:34 -0000 On 19 Feb 2021, at 16:24, Kristof Provost wrote: > On 16 Feb 2021, at 21:31, Alexander V. Chernikov wrote: >> The branch main has been updated by melifaro: >> >> URL: = >> https://cgit.FreeBSD.org/src/commit/?id=3D2fe5a79425c79f7b828acd91da66= d97230925fc8 >> >> 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<..> 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(-) >> > >> +static int >> +cleanup_xaddrs_inet(struct rt_addrinfo *info) >> +{ >> + struct sockaddr_in *dst_sa, *mask_sa; >> + >> + /* Check & fixup dst/netmask combination first */ >> + dst_sa =3D (struct sockaddr_in *)info->rti_info[RTAX_DST]; >> + mask_sa =3D (struct sockaddr_in *)info->rti_info[RTAX_NETMASK]; >> + >> + struct in_addr mask =3D { >> + .s_addr =3D mask_sa ? mask_sa->sin_addr.s_addr : INADDR_BROADCAST, >> + }; >> + struct in_addr dst =3D { >> + .s_addr =3D htonl(ntohl(dst_sa->sin_addr.s_addr) & = >> ntohl(mask.s_addr)) >> + }; >> + > This breaks things like `arp -d 10.0.2.1`. It always masks off the = > network address, which is the right thing to do in the routing table, = > but not in the arp table. > > I=E2=80=99ve worked around it for now with this hack: > > diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c > index 3c1fea497af6..533076db99a5 100644 > --- a/sys/net/rtsock.c > +++ b/sys/net/rtsock.c > @@ -638,9 +638,12 @@ fill_addrinfo(struct rt_msghdr *rtm, int len, = > u_int fibnum, struct rt_addrinfo * > return (EINVAL); > > info->rti_flags =3D rtm->rtm_flags; > - error =3D cleanup_xaddrs(info); > - if (error !=3D 0) > - return (error); > + /* XXX HACK */ > + if (! (rtm->rtm_flags & RTF_LLDATA)) { > + error =3D cleanup_xaddrs(info); > + if (error !=3D 0) > + return (error); > + } > saf =3D info->rti_info[RTAX_DST]->sa_family; > /* > * Verify that the caller has the appropriate privilege; = > RTM_GET > > But I=E2=80=99m not totally happy with this, obviously. This may be a bit more reasonable: diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c index 3c1fea497af6..5147b92e95d5 100644 --- a/sys/net/rtsock.c +++ b/sys/net/rtsock.c @@ -1393,6 +1393,10 @@ cleanup_xaddrs_inet(struct rt_addrinfo *info) .s_addr =3D htonl(ntohl(dst_sa->sin_addr.s_addr) & = ntohl(mask.s_addr)) }; + /* Keep the address if we're LL */ + if (info->rti_flags & RTF_LLDATA) + dst.s_addr =3D dst_sa->sin_addr.s_addr; + if (dst_sa->sin_len < sizeof(struct sockaddr_in)) { printf("dst sin_len too small\n"); return (EINVAL); @@ -1431,7 +1435,10 @@ cleanup_xaddrs_inet6(struct rt_addrinfo *info) mask_sa =3D (struct sockaddr_in6 *)info->rti_info[RTAX_NETMASK];= mask =3D mask_sa ? mask_sa->sin6_addr : in6mask128; - IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); + + /* Keep the address if we're LL */ + if (! (info->rti_flags & RTF_LLDATA)) + IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask); if (dst_sa->sin6_len < sizeof(struct sockaddr_in6)) { printf("dst sin6_len too small\n"); Best regards, Kristof