From: Alexander Bluhm Subject: Re: diff: setting M_BCAST flag To: YASUOKA Masahiko Cc: tech@openbsd.org Date: Tue, 11 Mar 2025 14:48:28 +0100 On Tue, Mar 11, 2025 at 05:42:30PM +0900, YASUOKA Masahiko wrote: > 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, You are right. Removing the check in UDP, but not adding it in IP was wrong. At least the broadcast flag in ip_input() was forgotten. The comment /* Make sure M_BCAST is set */ does not make sense. It just describes the simple statement witten in the C code. Either remove the comment, or explain why M_BCAST has to be set. Do not move pf_ouraddr() after route_mpath(). That counteracts the idea of this function. If pf is smart and knows the packet is for us, we don't need a route lookup. bluhm > 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; > > > >