From: YASUOKA Masahiko Subject: Re: diff: setting M_BCAST flag To: bluhm@openbsd.org Cc: tech@openbsd.org Date: Tue, 11 Mar 2025 17:42:30 +0900 On Mon, 10 Mar 2025 19:28:25 +0100 Alexander Bluhm wrote: > On Sat, Mar 08, 2025 at 12:54:35PM +0900, YASUOKA Masahiko wrote: >> Hello, >> >> Our ip_input() doesn't set M_BCAST flag for 0.0.0.0 or 255.255.255.255 >> if it's not set by the lower layer. The flag must be set since it is >> used by the upper layers. (eg. TCP use the flag to drop the packet.) >> >> The modification on in_ouraddr() is to set M_BCAST flag consistently >> if the packet is for directed broadcast. Previous the flag isn't set >> if the packet is diverted by pf(4). >> >> (eg. on the host that runs a transparent proxy for 80/tcp, when it >> receives a packet for directed broadcast but its destination mac >> address is not ff:ff:ff:ff:ff:ff (eg. wrong netmask), TCP wil not drop >> it and the connection will be established.) >> >> ok? comments? > > M_BCAST looks like a layer 2 flag to me. I we want to prevent layer > 3 broadcast addresses in TCP, tcp_input() should care about it. Actually, sys/sys/mbuf.h says the flags is for link-level #define M_BCAST 0x0100 /* sent/received as link-level broadcast */ But, see the commit below, https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/netinet/udp_usrreq.c#rev1.192 |date: 2014/11/20 11:05:19; author: mpi; state: Exp; lines: +9 -1; commitid: uo0PiO5ELdW7V3PO; |In TCP and UDP layers do not (ab)use the receiving interface to check |for a multicast/broadcast destination address. | |These checks have already been done in the Ethernet and IP layers and |the mbuf(9) should contain all the required information at this point. |But since we cannot trust this spaghetti stack, be paranoid and make |sure to set the flags in the IP input routines. | |Use explicit comments, requested by deraadt@. ok claudio@ =================================================================== RCS file: /cvsrepo/anoncvs/cvs/src/sys/netinet/udp_usrreq.c,v retrieving revision 1.191 retrieving revision 1.192 diff -u -r1.191 -r1.192 --- src/sys/netinet/udp_usrreq.c 2014/11/09 22:05:08 1.191 +++ src/sys/netinet/udp_usrreq.c 2014/11/20 11:05:19 1.192 @@ -1,4 +1,4 @@ -/* $OpenBSD: udp_usrreq.c,v 1.191 2014/11/09 22:05:08 bluhm Exp $ */ +/* $OpenBSD: udp_usrreq.c,v 1.192 2014/11/20 11:05:19 mpi Exp $ */ /* $NetBSD: udp_usrreq.c,v 1.28 1996/03/16 23:54:03 christos Exp $ */ /* @@ -400,16 +400,7 @@ } #endif -#ifdef INET6 - if ((ip6 && IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst)) || - (ip && IN_MULTICAST(ip->ip_dst.s_addr)) || - (ip && in_broadcast(ip->ip_dst, m->m_pkthdr.rcvif, - m->m_pkthdr.ph_rtableid))) { -#else /* INET6 */ - if (IN_MULTICAST(ip->ip_dst.s_addr) || - in_broadcast(ip->ip_dst, m->m_pkthdr.rcvif, - m->m_pkthdr.ph_rtableid)) { -#endif /* INET6 */ + if (m->m_flags & (M_BCAST|M_MCAST)) { struct inpcb *last; /* * Deliver a multicast or broadcast datagram to *all* sockets It seems we took the way the flags are used for TCP or UDP layers. So, I thought we should consistently set the flags on IP layer. > > NetBSD has > if (IN_MULTICAST(ip->ip_dst.s_addr) || > in_broadcast(ip->ip_dst, m_get_rcvif_NOMPSAFE(m))) > goto drop; > > FreeBSD does > if (IN_MULTICAST(ntohl(ip->ip_dst.s_addr)) || > IN_MULTICAST(ntohl(ip->ip_src.s_addr)) || > ip->ip_src.s_addr == htonl(INADDR_BROADCAST) || > in_ifnet_broadcast(ip->ip_dst, m->m_pkthdr.rcvif)) { > if ((s = tcp_log_addrs(&inc, th, NULL, NULL))) > log(LOG_DEBUG, "%s; %s: Listen socket: " > "Connection attempt from/to broad- " > "or multicast address ignored\n", > s, __func__); > goto dropunlock; > } > > When receiving a SYN packet, they take a closer look. Maybe we > need something simmilar. > > Do not move pf_ouraddr() after route_mpath(). This is a performance > optimization. If pf knows that the packet belongs to a local socket, > the route lookup is skipped. > > bluhm > >> Index: sys/netinet/ip_input.c >> =================================================================== >> RCS file: /cvs/src/sys/netinet/ip_input.c,v >> diff -u -p -r1.404 ip_input.c >> --- sys/netinet/ip_input.c 2 Mar 2025 21:28:32 -0000 1.404 >> +++ sys/netinet/ip_input.c 8 Mar 2025 03:51:45 -0000 >> @@ -508,6 +508,7 @@ ip_input_if(struct mbuf **mp, int *offp, >> >> if (ip->ip_dst.s_addr == INADDR_BROADCAST || >> ip->ip_dst.s_addr == INADDR_ANY) { >> + m->m_flags |= M_BCAST; /* Make sure M_BCAST is set */ >> nxt = ip_ours(mp, offp, nxt, af, ns); >> goto out; >> } >> @@ -518,7 +519,7 @@ ip_input_if(struct mbuf **mp, int *offp, >> } else { >> ro = &ns->ns_route; >> } >> - switch(in_ouraddr(m, ifp, ro, flags)) { >> + switch (in_ouraddr(m, ifp, ro, flags)) { >> case 2: >> goto bad; >> case 1: >> @@ -851,18 +852,6 @@ in_ouraddr(struct mbuf *m, struct ifnet >> struct ip *ip; >> int match = 0; >> >> -#if NPF > 0 >> - switch (pf_ouraddr(m)) { >> - case 0: >> - return (0); >> - case 1: >> - return (1); >> - default: >> - /* pf does not know it */ >> - break; >> - } >> -#endif >> - >> ip = mtod(m, struct ip *); >> >> rt = route_mpath(ro, &ip->ip_dst, &ip->ip_src, m->m_pkthdr.ph_rtableid); >> @@ -883,6 +872,18 @@ in_ouraddr(struct mbuf *m, struct ifnet >> m->m_flags |= M_BCAST; >> } >> } >> + >> +#if NPF > 0 >> + switch (pf_ouraddr(m)) { >> + case 0: >> + return (0); >> + case 1: >> + return (1); >> + default: >> + /* pf does not know it */ >> + break; >> + } >> +#endif >> >> if (!match) { >> struct ifaddr *ifa; > >