git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code.
Alexander V. Chernikov
melifaro at freebsd.org
Tue Feb 16 21:52:54 UTC 2021
16.02.2021, 21:47, "Mateusz Guzik" <mjguzik at gmail.com>:
> In this context I meant NOINET and NOINET6
>
> Anyhow I see the following:
> sys/net/rtsock.c:1427:2: error: implicit declaration of function
> 'IN6_MASK_ADDR' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
> IN6_MASK_ADDR(&dst_sa->sin6_addr, &mask);
Should be fixed by a4513bace0e0.
Sorry for the breakage.
> ^
>
> On 2/16/21, Alexander V. Chernikov <melifaro at freebsd.org> wrote:
>> 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>
>
> --
> Mateusz Guzik <mjguzik gmail.com>
More information about the dev-commits-src-all
mailing list