git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code.
Alexander V. Chernikov
melifaro at freebsd.org
Tue Feb 16 21:26:00 UTC 2021
16.02.2021, 20:43, "Mateusz Guzik" <mjguzik at gmail.com>:
> 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 <melifaro at freebsd.org> 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 at FreeBSD.org>
>> AuthorDate: 2021-02-16 20:30:04 +0000
>> Commit: Alexander V. Chernikov <melifaro at 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(-)
>>
>> 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 <netinet/if_ether.h>
>> #include <netinet/ip_carp.h>
>> #ifdef INET6
>> +#include <netinet6/in6_var.h>
>> #include <netinet6/ip6_var.h>
>> #include <netinet6/scope6_var.h>
>> #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 at 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 at freebsd.org"
>
> --
> Mateusz Guzik <mjguzik gmail.com>
More information about the dev-commits-src-all
mailing list