Download raw body.
diff: setting M_BCAST flag
On Tue, Mar 11, 2025 at 05:42:30PM +0900, YASUOKA Masahiko wrote:
> On Mon, 10 Mar 2025 19:28:25 +0100
> Alexander Bluhm <bluhm@openbsd.org> 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;
> >
> >
diff: setting M_BCAST flag