git: 2fe5a79425c7 - main - Fix dst/netmask handling in routing socket code.
Mateusz Guzik
mjguzik at gmail.com
Tue Feb 16 20:43:20 UTC 2021
This breaks the built at least without INET6.
can you please start testing your patches on NOINET kernels
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-main
mailing list