Date: Thu, 22 Apr 2021 16:59:31 -0400 From: Nathan Whitehorn <nwhitehorn@freebsd.org> To: "Alexander V. Chernikov" <melifaro@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: b31fbebeb3d5 - main - Relax rtsock message restrictions. Message-ID: <8151c4a1-776e-ed67-6542-04dfbc945e52@freebsd.org> In-Reply-To: <202104202135.13KLZDPM097608@gitrepo.freebsd.org> References: <202104202135.13KLZDPM097608@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Thanks! Is this an erratum candidate for 13.0? I was pretty surprised when a production machine stopped routing after an upgrade, and I can't be the only one. -Nathan On 4/20/21 5:35 PM, Alexander V. Chernikov wrote: > The branch main has been updated by melifaro: > > URL: https://cgit.FreeBSD.org/src/commit/?id=b31fbebeb3d59af359a3417cddfbcf666b2c56c9 > > commit b31fbebeb3d59af359a3417cddfbcf666b2c56c9 > Author: Alexander V. Chernikov <melifaro@FreeBSD.org> > AuthorDate: 2021-04-19 20:49:18 +0000 > Commit: Alexander V. Chernikov <melifaro@FreeBSD.org> > CommitDate: 2021-04-20 21:34:19 +0000 > > Relax rtsock message restrictions. > > Address multiple issues with strict rtsock message validation. > > D28668 "normalisation" approach was based on the assumption that > we always have at least "standard" sockaddr len. > It turned out to be false - certain older applications like quagga > or routed abuse sin[6]_len field and set it to the offset to the > first fully-zero bit in the mask. It is impossible to normalise > such sockaddrs without reallocation. > > With that in mind, change the approach to use a distinct memory > buffer for the altered sockaddrs. This allows supporting the older > software while maintaining the guarantee on the "standard" sockaddrs. > > PR: 255273,255089 > Differential Revision: https://reviews.freebsd.org/D29826 > MFC after: 3 days > --- > sys/net/rtsock.c | 271 ++++++++++++++++++++++++++++++++++++------------------- > 1 file changed, 177 insertions(+), 94 deletions(-) > > diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c > index 5194a2a15c1e..b7a7e5170c74 100644 > --- a/sys/net/rtsock.c > +++ b/sys/net/rtsock.c > @@ -126,6 +126,13 @@ struct ifa_msghdrl32 { > > #endif /* COMPAT_FREEBSD32 */ > > +struct linear_buffer { > + char *base; /* Base allocated memory pointer */ > + uint32_t offset; /* Currently used offset */ > + uint32_t size; /* Total buffer size */ > +}; > +#define SCRATCH_BUFFER_SIZE 1024 > + > #define RTS_PID_PRINTF(_fmt, ...) \ > printf("rtsock:%s(): PID %d: " _fmt "\n", __func__, curproc->p_pid, ## __VA_ARGS__) > > @@ -177,7 +184,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 cleanup_xaddrs(struct rt_addrinfo *info, struct linear_buffer *lb); > 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); > @@ -621,7 +628,8 @@ fill_blackholeinfo(struct rt_addrinfo *info, union sockaddr_union *saun) > * Returns 0 on success. > */ > static int > -fill_addrinfo(struct rt_msghdr *rtm, int len, u_int fibnum, struct rt_addrinfo *info) > +fill_addrinfo(struct rt_msghdr *rtm, int len, struct linear_buffer *lb, u_int fibnum, > + struct rt_addrinfo *info) > { > int error; > sa_family_t saf; > @@ -641,7 +649,7 @@ 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); > + error = cleanup_xaddrs(info, lb); > if (error != 0) > return (error); > saf = info->rti_info[RTAX_DST]->sa_family; > @@ -878,6 +886,45 @@ export_rtaddrs(const struct rtentry *rt, struct sockaddr *dst, > #endif > } > > +static int > +update_rtm_from_info(struct rt_addrinfo *info, struct rt_msghdr **prtm, > + int alloc_len) > +{ > + struct rt_msghdr *rtm, *orig_rtm = NULL; > + struct walkarg w; > + int len; > + > + rtm = *prtm; > + /* Check if we need to realloc storage */ > + rtsock_msg_buffer(rtm->rtm_type, info, NULL, &len); > + if (len > alloc_len) { > + struct rt_msghdr *tmp_rtm; > + > + tmp_rtm = malloc(len, M_TEMP, M_NOWAIT); > + if (tmp_rtm == NULL) > + return (ENOBUFS); > + bcopy(rtm, tmp_rtm, rtm->rtm_msglen); > + orig_rtm = rtm; > + rtm = tmp_rtm; > + alloc_len = len; > + > + /* > + * Delay freeing original rtm as info contains > + * data referencing it. > + */ > + } > + > + w.w_tmem = (caddr_t)rtm; > + w.w_tmemsize = alloc_len; > + rtsock_msg_buffer(rtm->rtm_type, info, &w, &len); > + rtm->rtm_addrs = info->rti_addrs; > + > + if (orig_rtm != NULL) > + free(orig_rtm, M_TEMP); > + *prtm = rtm; > + return (0); > +} > + > > /* > * Update sockaddrs, flags, etc in @prtm based on @rc data. > @@ -891,11 +938,10 @@ static int > update_rtm_from_rc(struct rt_addrinfo *info, struct rt_msghdr **prtm, > int alloc_len, struct rib_cmd_info *rc, struct nhop_object *nh) > { > - struct walkarg w; > union sockaddr_union saun; > - struct rt_msghdr *rtm, *orig_rtm = NULL; > + struct rt_msghdr *rtm; > struct ifnet *ifp; > - int error, len; > + int error; > > rtm = *prtm; > union sockaddr_union sa_dst, sa_mask; > @@ -927,28 +973,8 @@ update_rtm_from_rc(struct rt_addrinfo *info, struct rt_msghdr **prtm, > } else if (ifp != NULL) > rtm->rtm_index = ifp->if_index; > > - /* Check if we need to realloc storage */ > - rtsock_msg_buffer(rtm->rtm_type, info, NULL, &len); > - if (len > alloc_len) { > - struct rt_msghdr *tmp_rtm; > - > - tmp_rtm = malloc(len, M_TEMP, M_NOWAIT); > - if (tmp_rtm == NULL) > - return (ENOBUFS); > - bcopy(rtm, tmp_rtm, rtm->rtm_msglen); > - orig_rtm = rtm; > - rtm = tmp_rtm; > - alloc_len = len; > - > - /* > - * Delay freeing original rtm as info contains > - * data referencing it. > - */ > - } > - > - w.w_tmem = (caddr_t)rtm; > - w.w_tmemsize = alloc_len; > - rtsock_msg_buffer(rtm->rtm_type, info, &w, &len); > + if ((error = update_rtm_from_info(info, prtm, alloc_len)) != 0) > + return (error); > > rtm->rtm_flags = rc->rc_rt->rte_flags | nhop_get_rtflags(nh); > if (rtm->rtm_flags & RTF_GWFLAG_COMPAT) > @@ -956,11 +982,6 @@ update_rtm_from_rc(struct rt_addrinfo *info, struct rt_msghdr **prtm, > (rtm->rtm_flags & ~RTF_GWFLAG_COMPAT); > rt_getmetrics(rc->rc_rt, nh, &rtm->rtm_rmx); > rtm->rtm_rmx.rmx_weight = rc->rc_nh_weight; > - rtm->rtm_addrs = info->rti_addrs; > - > - if (orig_rtm != NULL) > - free(orig_rtm, M_TEMP); > - *prtm = rtm; > > return (0); > } > @@ -985,6 +1006,17 @@ save_add_notification(struct rib_cmd_info *rc, void *_cbdata) > } > #endif > > +static struct sockaddr * > +alloc_sockaddr_aligned(struct linear_buffer *lb, int len) > +{ > + len |= (sizeof(uint64_t) - 1); > + if (lb->offset + len > lb->size) > + return (NULL); > + struct sockaddr *sa = (struct sockaddr *)(lb->base + lb->offset); > + lb->offset += len; > + return (sa); > +} > + > /*ARGSUSED*/ > static int > route_output(struct mbuf *m, struct socket *so, ...) > @@ -1022,12 +1054,17 @@ route_output(struct mbuf *m, struct socket *so, ...) > * buffer aligned on 1k boundaty. > */ > alloc_len = roundup2(len, 1024); > - if ((rtm = malloc(alloc_len, M_TEMP, M_NOWAIT)) == NULL) > + int total_len = alloc_len + SCRATCH_BUFFER_SIZE; > + if ((rtm = malloc(total_len, M_TEMP, M_NOWAIT)) == NULL) > senderr(ENOBUFS); > > m_copydata(m, 0, len, (caddr_t)rtm); > bzero(&info, sizeof(info)); > nh = NULL; > + struct linear_buffer lb = { > + .base = (char *)rtm + alloc_len, > + .size = SCRATCH_BUFFER_SIZE, > + }; > > if (rtm->rtm_version != RTM_VERSION) { > /* Do not touch message since format is unknown */ > @@ -1042,19 +1079,19 @@ route_output(struct mbuf *m, struct socket *so, ...) > * caller PID and error value. > */ > > - if ((error = fill_addrinfo(rtm, len, fibnum, &info)) != 0) { > + if ((error = fill_addrinfo(rtm, len, &lb, fibnum, &info)) != 0) { > senderr(error); > } > + /* fill_addringo() embeds scope into IPv6 addresses */ > +#ifdef INET6 > + rti_need_deembed = 1; > +#endif > > saf = info.rti_info[RTAX_DST]->sa_family; > > /* support for new ARP code */ > if (rtm->rtm_flags & RTF_LLDATA) { > error = lla_rt_output(rtm, &info); > -#ifdef INET6 > - if (error == 0) > - rti_need_deembed = 1; > -#endif > goto flush; > } > > @@ -1067,7 +1104,6 @@ route_output(struct mbuf *m, struct socket *so, ...) > error = EINVAL; > if (error != 0) > senderr(error); > - /* TODO: rebuild rtm from scratch */ > } > > switch (rtm->rtm_type) { > @@ -1079,9 +1115,6 @@ route_output(struct mbuf *m, struct socket *so, ...) > } > error = rib_action(fibnum, rtm->rtm_type, &info, &rc); > if (error == 0) { > -#ifdef INET6 > - rti_need_deembed = 1; > -#endif > #ifdef ROUTE_MPATH > if (NH_IS_NHGRP(rc.rc_nh_new) || > (rc.rc_nh_old && NH_IS_NHGRP(rc.rc_nh_old))) { > @@ -1110,12 +1143,7 @@ route_output(struct mbuf *m, struct socket *so, ...) > } > #endif > nh = rc.rc_nh_old; > - goto report; > } > -#ifdef INET6 > - /* rt_msg2() will not be used when RTM_DELETE fails. */ > - rti_need_deembed = 1; > -#endif > break; > > case RTM_GET: > @@ -1124,13 +1152,18 @@ route_output(struct mbuf *m, struct socket *so, ...) > senderr(error); > nh = rc.rc_nh_new; > > -report: > if (!can_export_rte(curthread->td_ucred, > info.rti_info[RTAX_NETMASK] == NULL, > info.rti_info[RTAX_DST])) { > senderr(ESRCH); > } > + break; > > + default: > + senderr(EOPNOTSUPP); > + } > + > + if (error == 0) { > error = update_rtm_from_rc(&info, &rtm, alloc_len, &rc, nh); > /* > * Note that some sockaddr pointers may have changed to > @@ -1147,12 +1180,6 @@ report: > #ifdef INET6 > rti_need_deembed = 0; > #endif > - if (error != 0) > - senderr(error); > - break; > - > - default: > - senderr(EOPNOTSUPP); > } > > flush: > @@ -1174,6 +1201,10 @@ flush: > bcopy(sin6, info.rti_info[i], > sizeof(*sin6)); > } > + if (update_rtm_from_info(&info, &rtm, alloc_len) != 0) { > + if (error != 0) > + error = ENOBUFS; > + } > } > } > #endif > @@ -1350,9 +1381,10 @@ cleanup_xaddrs_lladdr(struct rt_addrinfo *info) > } > > static int > -cleanup_xaddrs_gateway(struct rt_addrinfo *info) > +cleanup_xaddrs_gateway(struct rt_addrinfo *info, struct linear_buffer *lb) > { > struct sockaddr *gw = info->rti_info[RTAX_GATEWAY]; > + struct sockaddr *sa; > > if (info->rti_flags & RTF_LLDATA) > return (cleanup_xaddrs_lladdr(info)); > @@ -1362,11 +1394,17 @@ cleanup_xaddrs_gateway(struct rt_addrinfo *info) > case AF_INET: > { > struct sockaddr_in *gw_sin = (struct sockaddr_in *)gw; > - if (gw_sin->sin_len < sizeof(struct sockaddr_in)) { > + > + /* Ensure reads do not go beyoud SA boundary */ > + if (SA_SIZE(gw) < offsetof(struct sockaddr_in, sin_zero)) { > RTS_PID_PRINTF("gateway sin_len too small: %d", gw->sa_len); > return (EINVAL); > } > - fill_sockaddr_inet(gw_sin, gw_sin->sin_addr); > + sa = alloc_sockaddr_aligned(lb, sizeof(struct sockaddr_in)); > + if (sa == NULL) > + return (ENOBUFS); > + fill_sockaddr_inet((struct sockaddr_in *)sa, gw_sin->sin_addr); > + info->rti_info[RTAX_GATEWAY] = sa; > } > break; > #endif > @@ -1392,13 +1430,17 @@ cleanup_xaddrs_gateway(struct rt_addrinfo *info) > RTS_PID_PRINTF("gateway sdl_len too small: %d", gw_sdl->sdl_len); > return (EINVAL); > } > + sa = alloc_sockaddr_aligned(lb, sizeof(struct sockaddr_dl_short)); > + if (sa == NULL) > + return (ENOBUFS); > > const struct sockaddr_dl_short sdl = { > .sdl_family = AF_LINK, > - .sdl_len = sdl_min_len, > + .sdl_len = sizeof(struct sockaddr_dl_short), > .sdl_index = gw_sdl->sdl_index, > }; > - memcpy(gw_sdl, &sdl, sdl_min_len); > + *((struct sockaddr_dl_short *)sa) = sdl; > + info->rti_info[RTAX_GATEWAY] = sa; > break; > } > } > @@ -1416,39 +1458,57 @@ remove_netmask(struct rt_addrinfo *info) > > #ifdef INET > static int > -cleanup_xaddrs_inet(struct rt_addrinfo *info) > +cleanup_xaddrs_inet(struct rt_addrinfo *info, struct linear_buffer *lb) > { > struct sockaddr_in *dst_sa, *mask_sa; > + const int sa_len = sizeof(struct sockaddr_in); > + struct in_addr dst, mask; > > /* 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"); > + /* Ensure reads do not go beyound the buffer size */ > + if (SA_SIZE(dst_sa) < offsetof(struct sockaddr_in, sin_zero)) > return (EINVAL); > - } > - if (mask_sa && mask_sa->sin_len < sizeof(struct sockaddr_in)) { > - RTS_PID_PRINTF("prefix mask sin_len too small: %d", mask_sa->sin_len); > - return (EINVAL); > - } > + > + if ((mask_sa != NULL) && mask_sa->sin_len < sizeof(struct sockaddr_in)) { > + /* > + * Some older routing software encode mask length into the > + * sin_len, thus resulting in "truncated" sockaddr. > + */ > + int len = mask_sa->sin_len - offsetof(struct sockaddr_in, sin_addr); > + if (len >= 0) { > + mask.s_addr = 0; > + if (len > sizeof(struct in_addr)) > + len = sizeof(struct in_addr); > + memcpy(&mask, &mask_sa->sin_addr, len); > + } else { > + RTS_PID_PRINTF("prefix mask sin_len too small: %d", mask_sa->sin_len); > + return (EINVAL); > + } > + } else > + mask.s_addr = mask_sa ? mask_sa->sin_addr.s_addr : INADDR_BROADCAST; > + > + dst.s_addr = htonl(ntohl(dst_sa->sin_addr.s_addr) & ntohl(mask.s_addr)); > + > + /* Construct new "clean" dst/mask sockaddresses */ > + if ((dst_sa = (struct sockaddr_in *)alloc_sockaddr_aligned(lb, sa_len)) == NULL) > + return (ENOBUFS); > fill_sockaddr_inet(dst_sa, dst); > + info->rti_info[RTAX_DST] = (struct sockaddr *)dst_sa; > > - if (mask.s_addr != INADDR_BROADCAST) > + if (mask.s_addr != INADDR_BROADCAST) { > + if ((mask_sa = (struct sockaddr_in *)alloc_sockaddr_aligned(lb, sa_len)) == NULL) > + return (ENOBUFS); > fill_sockaddr_inet(mask_sa, mask); > - else > + info->rti_info[RTAX_NETMASK] = (struct sockaddr *)mask_sa; > + } else > remove_netmask(info); > > /* Check gateway */ > if (info->rti_info[RTAX_GATEWAY] != NULL) > - return (cleanup_xaddrs_gateway(info)); > + return (cleanup_xaddrs_gateway(info, lb)); > > return (0); > } > @@ -1456,43 +1516,66 @@ cleanup_xaddrs_inet(struct rt_addrinfo *info) > > #ifdef INET6 > static int > -cleanup_xaddrs_inet6(struct rt_addrinfo *info) > +cleanup_xaddrs_inet6(struct rt_addrinfo *info, struct linear_buffer *lb) > { > + struct sockaddr *sa; > struct sockaddr_in6 *dst_sa, *mask_sa; > - struct in6_addr mask; > + struct in6_addr mask, *dst; > + const int sa_len = sizeof(struct sockaddr_in6); > > /* 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)) { > RTS_PID_PRINTF("prefix dst sin6_len too small: %d", dst_sa->sin6_len); > return (EINVAL); > } > + > if (mask_sa && mask_sa->sin6_len < sizeof(struct sockaddr_in6)) { > - RTS_PID_PRINTF("rtsock: prefix mask sin6_len too small: %d", mask_sa->sin6_len); > - return (EINVAL); > - } > - fill_sockaddr_inet6(dst_sa, &dst_sa->sin6_addr, 0); > + /* > + * Some older routing software encode mask length into the > + * sin6_len, thus resulting in "truncated" sockaddr. > + */ > + int len = mask_sa->sin6_len - offsetof(struct sockaddr_in6, sin6_addr); > + if (len >= 0) { > + bzero(&mask, sizeof(mask)); > + if (len > sizeof(struct in6_addr)) > + len = sizeof(struct in6_addr); > + memcpy(&mask, &mask_sa->sin6_addr, len); > + } else { > + RTS_PID_PRINTF("rtsock: prefix mask sin6_len too small: %d", mask_sa->sin6_len); > + return (EINVAL); > + } > + } else > + mask = mask_sa ? mask_sa->sin6_addr : in6mask128; > > - if (!IN6_ARE_ADDR_EQUAL(&mask, &in6mask128)) > - fill_sockaddr_inet6(mask_sa, &mask, 0); > - else > + dst = &dst_sa->sin6_addr; > + IN6_MASK_ADDR(dst, &mask); > + > + if ((sa = alloc_sockaddr_aligned(lb, sa_len)) == NULL) > + return (ENOBUFS); > + fill_sockaddr_inet6((struct sockaddr_in6 *)sa, dst, 0); > + info->rti_info[RTAX_DST] = sa; > + > + if (!IN6_ARE_ADDR_EQUAL(&mask, &in6mask128)) { > + if ((sa = alloc_sockaddr_aligned(lb, sa_len)) == NULL) > + return (ENOBUFS); > + fill_sockaddr_inet6((struct sockaddr_in6 *)sa, &mask, 0); > + info->rti_info[RTAX_NETMASK] = sa; > + } else > remove_netmask(info); > > /* Check gateway */ > if (info->rti_info[RTAX_GATEWAY] != NULL) > - return (cleanup_xaddrs_gateway(info)); > + return (cleanup_xaddrs_gateway(info, lb)); > > return (0); > } > #endif > > static int > -cleanup_xaddrs(struct rt_addrinfo *info) > +cleanup_xaddrs(struct rt_addrinfo *info, struct linear_buffer *lb) > { > int error = EAFNOSUPPORT; > > @@ -1511,12 +1594,12 @@ cleanup_xaddrs(struct rt_addrinfo *info) > switch (info->rti_info[RTAX_DST]->sa_family) { > #ifdef INET > case AF_INET: > - error = cleanup_xaddrs_inet(info); > + error = cleanup_xaddrs_inet(info, lb); > break; > #endif > #ifdef INET6 > case AF_INET6: > - error = cleanup_xaddrs_inet6(info); > + error = cleanup_xaddrs_inet6(info, lb); > break; > #endif > } >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8151c4a1-776e-ed67-6542-04dfbc945e52>