[PATCH] Pkttype match mismatches in OUTPUT chain

[PATCH] Pkttype match mismatches in OUTPUT chain

[Phil Oester]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

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 <kernel@linuxace.com>

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


[-- Attachment #2: patch-pkttype-output --]
[-- Type: text/plain, Size: 1121 bytes --]

diff --git a/net/netfilter/xt_pkttype.c b/net/netfilter/xt_pkttype.c
index 7936f7e..5575334 100644
--- a/net/netfilter/xt_pkttype.c
+++ b/net/netfilter/xt_pkttype.c
@@ -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;
 }

Re: [PATCH] Pkttype match mismatches in OUTPUT chain

[Phil Oester]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

Minor change to fix compile warning.

On Sun, Aug 10, 2008 at 03:18:35PM -0700, Phil Oester wrote:
> 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 <kernel@linuxace.com>
> 
> [1] http://marc.info/?l=netfilter-devel&m=115318495708540&w=2



[-- Attachment #2: patch-pkttype-output --]
[-- Type: text/plain, Size: 1115 bytes --]

diff --git a/net/netfilter/xt_pkttype.c b/net/netfilter/xt_pkttype.c
index 7936f7e..7036d43 100644
--- a/net/netfilter/xt_pkttype.c
+++ b/net/netfilter/xt_pkttype.c
@@ -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;
 }

Re: [PATCH] Pkttype match mismatches in OUTPUT chain

[Patrick McHardy]

Phil Oester wrote:
> Minor change to fix compile warning.
> 
> On Sun, Aug 10, 2008 at 03:18:35PM -0700, Phil Oester wrote:
>> 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 <kernel@linuxace.com>

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?

Re: [PATCH] Pkttype match mismatches in OUTPUT chain

[Phil Oester]

On Wed, Aug 13, 2008 at 03:32:24PM +0200, Patrick McHardy wrote:
> 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

Re: [PATCH] Pkttype match mismatches in OUTPUT chain

[Phil Oester]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

On Thu, Aug 14, 2008 at 05:02:40AM -0700, Phil Oester wrote:
> On Wed, Aug 13, 2008 at 03:32:24PM +0200, Patrick McHardy wrote:
> > 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

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


[-- Attachment #2: patch-pkttype-output --]
[-- Type: text/plain, Size: 1115 bytes --]

diff --git a/net/netfilter/xt_pkttype.c b/net/netfilter/xt_pkttype.c
index 7936f7e..7036d43 100644
--- a/net/netfilter/xt_pkttype.c
+++ b/net/netfilter/xt_pkttype.c
@@ -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;
 }