Date: Fri, 19 Feb 2021 20:09:41 +0000 From: Alexander V. Chernikov <melifaro@freebsd.org> To: Kristof Provost <kp@freebsd.org> 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: <144881613765282@mail.yandex.ru> In-Reply-To: <471C6715-4F53-4DE7-9233-FC1EDEE49E2D@FreeBSD.org> References: <202102162031.11GKV0T6060307@gitrepo.freebsd.org> <26E2BA35-291E-4DC5-BDCB-D98347D7E24C@FreeBSD.org> <471C6715-4F53-4DE7-9233-FC1EDEE49E2D@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
19.02.2021, 16:11, "Kristof Provost" <kp@freebsd.org>: > 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=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(-) >>> +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)) >>> + }; >>> + >> >> 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. Thanks for catching it! I've raised https://reviews.freebsd.org/D28804 to fix it. (Also: yes, this should be covered by tests). >> >> I’ve 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 = rtm->rtm_flags; >> - error = cleanup_xaddrs(info); >> - if (error != 0) >> - return (error); >> + /* XXX HACK */ >> + if (! (rtm->rtm_flags & RTF_LLDATA)) { >> + 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 >> >> But I’m 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 = 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 = 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 = (struct sockaddr_in6 *)info->rti_info[RTAX_NETMASK]; mask = 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?144881613765282>