Skip to contentSkip to conversation

Pkttype match mismatches in OUTPUT chain

PATCH In review Phil Oester 2 participants 5 messages Aug 10, 2008 – Sep 3, 2008

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Phil Oester

author
#0 lore raw

PATCH 1/1 Pkttype match mismatches in OUTPUT chain

Back in 7/2006, we fixed an issue with the pkttype match mismatching on locally generated packets.[1] At the time, I didn't test the fix in the OUTPUT chain, but only in the INPUT chain, where packets showed up as PACKET_LOOPBACK. Unfortunately, when packets are output, they aren't tagged as PACKET_LOOPBACK so the fix was incomplete.

Below is another attempt at fixing the problem in all cases, and fixes the original netfilter bugzilla #484 as well as a new bug submission (which I can't get the number of since bugzilla is presently down).

The fix is somewhat complicated because when broadcast packets hit the OUTPUT chain they have no destination MAC attached, so the "simple" test for 'all FF' doesn't work for us, and instead we have to consult the routing table.

One other note: since IPv6 doesn't have the concept of "broadcast", perhaps the userspace extension shouldn't allow broadcast rules to be added for that family?

Phil

Signed-off-by Phil Oester <[email protected]>

[1] http://marc.info/?l=netfilter-devel&m=115318495708540&w=2

net/netfilter/xt_pkttype.c +12 -9
index 7936f7e..5575334 100644
@@ -29,18 +29,21 @@ pkttype_mt(const struct sk_buff *skb, const struct net_device *in,
            bool *hotdrop)
 {
 	const struct xt_pkttype_info *info = matchinfo;
-	u_int8_t type;
+	u_int8_t type = 0; 
 
-	if (skb->pkt_type != PACKET_LOOPBACK)
-		type = skb->pkt_type;
-	else if (match->family == AF_INET &&
-	    ipv4_is_multicast(ip_hdr(skb)->daddr))
-		type = PACKET_MULTICAST;
-	else if (match->family == AF_INET6 &&
+	if (match->family == AF_INET) {
+		const struct net *net = dev_net(skb->dst->dev);
+		const struct iphdr *iph = ip_hdr(skb);
+		if (ipv4_is_multicast(iph->daddr))
+			type = PACKET_MULTICAST;
+		else if (inet_addr_type(net, iph->daddr) == RTN_BROADCAST)
+			type = PACKET_BROADCAST;
+	} else if (match->family == AF_INET6 &&
 	    ipv6_hdr(skb)->daddr.s6_addr[0] == 0xFF)
 		type = PACKET_MULTICAST;
-	else
-		type = PACKET_BROADCAST;
+
+	if (!type)
+		type = skb->pkt_type;
 
 	return (type == info->pkttype) ^ info->invert;
 }
author
#1 lore raw

Minor change to fix compile warning.

Quoted 19 lines · Phil Oester

Back in 7/2006, we fixed an issue with the pkttype match mismatching on locally generated packets.[1] At the time, I didn't test the fix in the OUTPUT chain, but only in the INPUT chain, where packets showed up as PACKET_LOOPBACK. Unfortunately, when packets are output, they aren't tagged as PACKET_LOOPBACK so the fix was incomplete.

Below is another attempt at fixing the problem in all cases, and fixes the original netfilter bugzilla #484 as well as a new bug submission (which I can't get the number of since bugzilla is presently down).

The fix is somewhat complicated because when broadcast packets hit the OUTPUT chain they have no destination MAC attached, so the "simple" test for 'all FF' doesn't work for us, and instead we have to consult the routing table.

One other note: since IPv6 doesn't have the concept of "broadcast", perhaps the userspace extension shouldn't allow broadcast rules to be added for that family?

Quoted 5 lines

Phil

Signed-off-by Phil Oester <[email protected]>

[1] http://marc.info/?l=netfilter-devel&m=115318495708540&w=2

net/netfilter/xt_pkttype.c +12 -9
index 7936f7e..7036d43 100644
@@ -29,18 +29,21 @@ pkttype_mt(const struct sk_buff *skb, const struct net_device *in,
            bool *hotdrop)
 {
 	const struct xt_pkttype_info *info = matchinfo;
-	u_int8_t type;
+	u_int8_t type = 0; 
 
-	if (skb->pkt_type != PACKET_LOOPBACK)
-		type = skb->pkt_type;
-	else if (match->family == AF_INET &&
-	    ipv4_is_multicast(ip_hdr(skb)->daddr))
-		type = PACKET_MULTICAST;
-	else if (match->family == AF_INET6 &&
+	if (match->family == AF_INET) {
+		struct net *net = dev_net(skb->dst->dev);
+		const struct iphdr *iph = ip_hdr(skb);
+		if (ipv4_is_multicast(iph->daddr))
+			type = PACKET_MULTICAST;
+		else if (inet_addr_type(net, iph->daddr) == RTN_BROADCAST)
+			type = PACKET_BROADCAST;
+	} else if (match->family == AF_INET6 &&
 	    ipv6_hdr(skb)->daddr.s6_addr[0] == 0xFF)
 		type = PACKET_MULTICAST;
-	else
-		type = PACKET_BROADCAST;
+
+	if (!type)
+		type = skb->pkt_type;
 
 	return (type == info->pkttype) ^ info->invert;
 }
#2 lore raw
Quoted 25 lines · Phil Oester

Minor change to fix compile warning.

Quoted 22 lines · Phil Oester

Back in 7/2006, we fixed an issue with the pkttype match mismatching on locally generated packets.[1] At the time, I didn't test the fix in the OUTPUT chain, but only in the INPUT chain, where packets showed up as PACKET_LOOPBACK. Unfortunately, when packets are output, they aren't tagged as PACKET_LOOPBACK so the fix was incomplete.

Below is another attempt at fixing the problem in all cases, and fixes the original netfilter bugzilla #484 as well as a new bug submission (which I can't get the number of since bugzilla is presently down).

The fix is somewhat complicated because when broadcast packets hit the OUTPUT chain they have no destination MAC attached, so the "simple" test for 'all FF' doesn't work for us, and instead we have to consult the routing table.

One other note: since IPv6 doesn't have the concept of "broadcast", perhaps the userspace extension shouldn't allow broadcast rules to be added for that family?

Phil

Signed-off-by Phil Oester <[email protected]>

This is getting more and more kludgy, wouldn't it make more sense to move the pkt_type initialisation from the device layer to the protocol layer?

author
#3 lore raw
Quoted 3 lines · Patrick McHardy

This is getting more and more kludgy, wouldn't it make more sense to move the pkt_type initialisation from the device layer to the protocol layer?

It would, but that's a large-ish change, and unknown if DaveM would support it just to make an iptables match less kludgy.

Unfortunately, it doesn't appear this match was tested very thoroughly prior to being added to the tree...

Phil

author
#4 lore raw
Quoted 5 lines · Phil Oester
Quoted 3 lines · Patrick McHardy

This is getting more and more kludgy, wouldn't it make more sense to move the pkt_type initialisation from the device layer to the protocol layer?

Quoted 7 lines

It would, but that's a large-ish change, and unknown if DaveM would support it just to make an iptables match less kludgy.

Unfortunately, it doesn't appear this match was tested very thoroughly prior to being added to the tree...

Phil

Any further thoughts on this Patrick? Longer-term, your plan would help, but perhaps in the interim we should get this queued up for 2.6.27 so we have a working match for OUTPUT packets?

Original patch included below.

Phil

net/netfilter/xt_pkttype.c +12 -9
index 7936f7e..7036d43 100644
@@ -29,18 +29,21 @@ pkttype_mt(const struct sk_buff *skb, const struct net_device *in,
            bool *hotdrop)
 {
 	const struct xt_pkttype_info *info = matchinfo;
-	u_int8_t type;
+	u_int8_t type = 0; 
 
-	if (skb->pkt_type != PACKET_LOOPBACK)
-		type = skb->pkt_type;
-	else if (match->family == AF_INET &&
-	    ipv4_is_multicast(ip_hdr(skb)->daddr))
-		type = PACKET_MULTICAST;
-	else if (match->family == AF_INET6 &&
+	if (match->family == AF_INET) {
+		struct net *net = dev_net(skb->dst->dev);
+		const struct iphdr *iph = ip_hdr(skb);
+		if (ipv4_is_multicast(iph->daddr))
+			type = PACKET_MULTICAST;
+		else if (inet_addr_type(net, iph->daddr) == RTN_BROADCAST)
+			type = PACKET_BROADCAST;
+	} else if (match->family == AF_INET6 &&
 	    ipv6_hdr(skb)->daddr.s6_addr[0] == 0xFF)
 		type = PACKET_MULTICAST;
-	else
-		type = PACKET_BROADCAST;
+
+	if (!type)
+		type = skb->pkt_type;
 
 	return (type == info->pkttype) ^ info->invert;
 }