Download raw body.
diff: setting M_BCAST flag
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,
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