git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code.
Mateusz Guzik
mjguzik at gmail.com
Tue Feb 16 21:47:07 UTC 2021
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);
^
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