Fix wrong use of skb->protocol
Subsystems (4)
PATCH 1/1 Fix wrong use of skb->protocol
Hi Dave,
ip_sabotage_out() needs to distinguish IPv4 and IPv6 traffic. It currently does that by looking at skb->protocol. However, for locally originated packets, skb->protocol is not initialized.
The patch below instead looks at the version number of the packet's data, which should be 4 or 6.
Thanks to Pasha (Crazy AMD K7 <[email protected]>) for his patience.
net/bridge/br_netfilter.c renamed from net/bridge/br_netfilter.c.old +18 -15
@@ -845,19 +845,6 @@ static unsigned int ip_sabotage_out(unsi {struct sk_buff *skb = *pskb; -#ifdef CONFIG_SYSCTL -if (!skb->nf_bridge) { -struct vlan_ethhdr *hdr =vlan_eth_hdr (skb); - -if (skb->protocol ==__constant_htons (ETH_P_IP ) || -IS_VLAN_IP ) { -if (!brnf_call_iptables) -return NF_ACCEPT ; - }else if (!brnf_call_ip6tables) -return NF_ACCEPT ; - } -#endif - if ((out->hard_start_xmit == br_dev_xmit && okfn != br_nf_forward_finish && okfn != br_nf_local_out_finish &&
@@ -869,8 +856,24 @@ static unsigned int ip_sabotage_out(unsi ) {struct nf_bridge_info *nf_bridge; -if (!skb->nf_bridge && !nf_bridge_alloc (skb)) -return NF_DROP ; +if (!skb->nf_bridge) { +#ifdef CONFIG_SYSCTL +/* This code is executed while in the IP(v6) stack, +the version should be 4 or 6. We can't use +skb->protocol because that isn't set on +PF_INET(6)/LOCAL_OUT. */ +struct iphdr *ip = skb->nh.iph; + +if (ip->version ==4 && !brnf_call_iptables) +return NF_ACCEPT ; +else if (ip->version ==6 && !brnf_call_ip6tables) +return NF_ACCEPT ; +#endif +if (hook ==NF_IP_POST_ROUTING ) +return NF_ACCEPT ; +if (!nf_bridge_alloc (skb)) +return NF_DROP ; + } nf_bridge = skb->nf_bridge;
Quoted 5 lines · Bart De Schuymer
ip_sabotage_out()needs to distinguish IPv4 and IPv6 traffic. It currently does that by looking atskb->protocol. However, for locally originated packets,skb->protocolis not initialized.
The patch below instead looks at the version number of the packet's data, which should be 4 or 6.
A while ago there were a number of problems with bridging CIPE ethernet devices, which turned out to be the bridge code not initialising skb->protocol for locally originated STP frames.
At the time I was told that initialising skb->protocol for locally originated packets is required, so that is how I fixed it then.
cheers,
Lennert
Op do, 30-12-2004 te 23:24 +0100, schreef Lennert Buytenhek:
Quoted 8 lines · Bart De Schuymer
Quoted 6 lines
ip_sabotage_out()needs to distinguish IPv4 and IPv6 traffic. It currently does that by looking atskb->protocol. However, for locally originated packets,skb->protocolis not initialized.
The patch below instead looks at the version number of the packet's data, which should be 4 or 6.
Quoted 6 lines
A while ago there were a number of problems with bridging CIPE ethernet devices, which turned out to be the bridge code not initialising
skb->protocolfor locally originated STP frames.At the time I was told that initialising
skb->protocolfor locally originated packets is required, so that is how I fixed it then.
Hi Lennert,
skb->protocol is not set for locally generated packets when the packet is still in the IP stack. I don't know what happens with it after the IP stack is finished with the packet.
The comment in skbuff.h says "packet protocol from driver", from which I tend to conclude that skb->protocol is only set by drivers when a packet enters the box. Too bad stuff like this isn't clearly spelled out, the FIXME for the dst field has been sitting there for probably more than a year too. Anyway, it wouldn't hurt if the skb->protocol field always held the right value.
cheers,
Bart
Quoted 5 lines · Bart De Schuymer
Hi Lennert,
skb->protocolis not set for locally generated packets when the packet is still in the IP stack. I don't know what happens with it after the IP stack is finished with the packet.
It is set shortly before the packet leaves the IP stack, in ip_finish_output. This is after LOCAL_OUT, but before POST_ROUTING. So your fix looks fine.
Quoted 6 lines
The comment in
skbuff.hsays "packet protocol from driver", from which I tend to conclude thatskb->protocolis only set by drivers when a packet enters the box. Too bad stuff like this isn't clearly spelled out, the FIXME for the dst field has been sitting there for probably more than a year too. Anyway, it wouldn't hurt if theskb->protocolfield always held the right value.
The IP stack knows it's IP anyway :) After that it has to hold the right value.
Regards
Patrick
Quoted 8 lines · Bart De Schuymer
Quoted 6 lines
A while ago there were a number of problems with bridging CIPE ethernet devices, which turned out to be the bridge code not initialising
skb->protocolfor locally originated STP frames.At the time I was told that initialising
skb->protocolfor locally originated packets is required, so that is how I fixed it then.Hi Lennert,
Hello,
Quoted 7 lines
skb->protocolis not set for locally generated packets when the packet is still in the IP stack. I don't know what happens with it after the IP stack is finished with the packet.
The comment inskbuff.hsays "packet protocol from driver", from which I tend to conclude thatskb->protocolis only set by drivers when a packet enters the box.
Too bad stuff like this isn't clearly spelled out,
This is what I thought back then too. Indeed, it's rather misleading.
Quoted 2 lines
the FIXME for the dst field has been sitting there for probably more than a year too.
Yes :(
Just one more thing: AFAIK it is possible to inject a raw IPv4 packet with an invalid IPv4 header. So maybe the better 'fix' would be to have different hooks for PF_INET and PF_INET6, and distinguish v4/v6 packets that way instead of peeking into the header. (The hook you're talking about is a PF_INET* and not a PF_BRIDGE hook, right?)
Then again, that would add yet another function onto the already rather deep call chains that we have in there.
Too bad I don't see any cleaner way of integrating the whole bridging thing into the stack. I wonder if any of the *BSDs found a cleaner way of doing this.
--L
Op vr, 31-12-2004 te 09:33 +0100, schreef Lennert Buytenhek:
Quoted 5 lines
Just one more thing: AFAIK it is possible to inject a raw IPv4 packet with an invalid IPv4 header. So maybe the better 'fix' would be to have different hooks for
PF_INETandPF_INET6, and distinguish v4/v6 packets that way instead of peeking into the header. (The hook you're talking about is a PF_INET* and not aPF_BRIDGEhook, right?)
That was my original plan, but it seems such a waste. Wouldn't injecting such an invalid IPv4 header also screw up iptables? Is there any reason why someone should be allowed to do this? I checked ip_tables.c::ipt_do_table() before using the IP version, and it looks at the IP header too without any precautions AFAICT.
Quoted 2 lines
Then again, that would add yet another function onto the already rather deep call chains that we have in there.
The netfilter scheme itself implies call chains.
Quoted 3 lines
Too bad I don't see any cleaner way of integrating the whole bridging thing into the stack. I wonder if any of the *BSDs found a cleaner way of doing this.
How about adding something like NF_STOP, which acts like NF_STOLEN but still executes okfn in nf_hook_slow()?
cheers,
Bart