On 08/11/20(Sun) 18:05, Denis Fondras wrote:
> ICMP error replies are sent from the IP of the interface the packet came in
> even
> when the source IP was forced with route(8).
icmp_reflect() is called without the KERNEL_LOCK(). rtable_getsource()
and ifa_ifwithaddr() are not safe to do so.
So it would be wise to turn this interface mp-safe first.
> Index: netinet/ip_icmp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.183
> diff -u -p -r1.183 ip_icmp.c
> --- netinet/ip_icmp.c 22 Aug 2020 17:55:54 -0000 1.183
> +++ netinet/ip_icmp.c 8 Nov 2020 16:48:15 -0000
> @@ -689,6 +689,8 @@ icmp_reflect(struct mbuf *m, struct mbuf
> struct mbuf *opts = NULL;
> struct sockaddr_in sin;
> struct rtentry *rt = NULL;
> + struct sockaddr *ip4_source = NULL;
> + struct in_addr src;
> int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
> u_int rtableid;
>
> @@ -707,6 +709,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
> m_resethdr(m);
> m->m_pkthdr.ph_rtableid = rtableid;
>
> + memset(&src, 0, sizeof(struct in_addr));
> /*
> * If the incoming packet was addressed directly to us,
> * use dst as the src for the reply. For broadcast, use
> @@ -721,7 +724,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
> rt = rtalloc(sintosa(&sin), 0, rtableid);
> if (rtisvalid(rt) &&
> ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
> - ia = ifatoia(rt->rt_ifa);
> + src = ifatoia(rt->rt_ifa)->ia_addr.sin_addr;
> }
>
> /*
> @@ -729,7 +732,7 @@ icmp_reflect(struct mbuf *m, struct mbuf
> * Use the new source address and do a route lookup. If it fails
> * drop the packet as there is no path to the host.
> */
> - if (ia == NULL) {
> + if (src.s_addr == 0) {
> rtfree(rt);
>
> memset(&sin, 0, sizeof(sin));
> @@ -745,14 +748,23 @@ icmp_reflect(struct mbuf *m, struct mbuf
> return (EHOSTUNREACH);
> }
>
> - ia = ifatoia(rt->rt_ifa);
> + ip4_source = rtable_getsource(rtableid, AF_INET);
> + if (ip4_source != NULL) {
> + struct ifaddr *ifa;
> + if ((ifa = ifa_ifwithaddr(ip4_source, rtableid)) !=
> + NULL && ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> + src = satosin(ip4_source)->sin_addr;
> + }
> + }
> + if (src.s_addr == 0)
> + src = ifatoia(rt->rt_ifa)->ia_addr.sin_addr;
> }
>
> ip->ip_dst = ip->ip_src;
> ip->ip_ttl = MAXTTL;
>
> /* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> - ip->ip_src = ia->ia_addr.sin_addr;
> + ip->ip_src = src;
> rtfree(rt);
>
> if (optlen > 0) {
> Index: netinet6/icmp6.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/icmp6.c,v
> retrieving revision 1.233
> diff -u -p -r1.233 icmp6.c
> --- netinet6/icmp6.c 28 Oct 2020 17:27:35 -0000 1.233
> +++ netinet6/icmp6.c 8 Nov 2020 16:48:15 -0000
> @@ -1146,6 +1146,7 @@ icmp6_reflect(struct mbuf **mp, size_t o
>
> if (src == NULL) {
> struct in6_ifaddr *ia6;
> + struct sockaddr *ip6_source = NULL;
>
> /*
> * This case matches to multicasts, our anycast, or unicasts
> @@ -1164,7 +1165,15 @@ icmp6_reflect(struct mbuf **mp, size_t o
> goto bad;
> }
> ia6 = in6_ifawithscope(rt->rt_ifa->ifa_ifp, &t, rtableid);
> - if (ia6 != NULL)
> + ip6_source = rtable_getsource(rtableid, AF_INET6);
> + if (ip6_source != NULL) {
> + struct ifaddr *ifa;
> + if ((ifa = ifa_ifwithaddr(ip6_source, rtableid)) !=
> + NULL && ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> + src = &satosin6(ip6_source)->sin6_addr;
> + }
> + }
> + if (src == NULL && ia6 != NULL)
> src = &ia6->ia_addr.sin6_addr;
> if (src == NULL)
> src = &ifatoia6(rt->rt_ifa)->ia_addr.sin6_addr;
>
