net/ipv4: push IP options to CB in ip_fragment

The bridge layer can overwrite the IPCB using the BR_INPUT_SKB_CB macro. In br_nf_dev_queue_xmit,
if we recieve a packet greater in size than the bridge device MTU, we call ip_fragment which in turn will lead to icmp_send calling ip_options_echo if the DF flag is set. ip_options_echo will then incorrectly try to parse the IPCB as IP options resulting in a buffer overflow.
This change refills the CB area back with IP
options before ip_fragment calls icmp_send. If we fail parsing, we zero out the IPCB area to guarantee that the stack does not get corrupted.

Test setup that can exhibit this behavior:
<Remote Machine>--<Host Machine>--<Bridge>--<Tun/Tap>--<KVM Guest>
1500 mtu 576 mtu 576 mtu 576 mtu 1500 mtu

Example of crash :

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff8146dff3
Pid: 3173, comm: qemu-kvm Not tainted 2.6.32-63.el6.x86_64 #1
Call Trace:
Message from <IRQ>  syslogd@kvm at  [<ffffffff814c8285>] panic+0x78/0x137
Aug 30 14:09:50  [<ffffffff8146dff3>] ? icmp_send+0x743/0x780

Signed-off-by Bandan Das <[email protected]>

---
 net/ipv4/ip_output.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

net/ipv4/ip_output.c +4 -0

index e427620..5fef4a9 100644
@@ -445,6 +445,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	struct iphdr *iph;
 	int ptr;
 	struct net_device *dev;
+	struct ip_options *opt;
 	struct sk_buff *skb2;
 	unsigned int mtu, hlen, left, len, ll_rs;
 	int offset;

@@ -462,6 +463,16 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 
 	if (unlikely((iph->frag_off & htons(IP_DF)) && !skb->local_df)) {
 		IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
+
+		/* Refill CB with IP options */
+		opt = &(IPCB(skb)->opt);

Quoted 47 lines · Bandan Das

The bridge layer can overwrite the IPCB using the BR_INPUT_SKB_CB macro. In br_nf_dev_queue_xmit,
if we recieve a packet greater in size than the bridge device MTU, we call ip_fragment which in turn will lead to icmp_send calling ip_options_echo if the DF flag is set. ip_options_echo will then incorrectly try to parse the IPCB as IP options resulting in a buffer overflow.
This change refills the CB area back with IP
options before ip_fragment calls icmp_send. If we fail parsing, we zero out the IPCB area to guarantee that the stack does not get corrupted.

Test setup that can exhibit this behavior:
<Remote Machine>--<Host Machine>--<Bridge>--<Tun/Tap>--<KVM Guest>
1500 mtu 576 mtu 576 mtu 576 mtu 1500 mtu

Example of crash :

Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff8146dff3
Pid: 3173, comm: qemu-kvm Not tainted 2.6.32-63.el6.x86_64 #1
Call Trace:
Message from <IRQ>  syslogd@kvm at  [<ffffffff814c8285>] panic+0x78/0x137
Aug 30 14:09:50  [<ffffffff8146dff3>] ? icmp_send+0x743/0x780

Signed-off-by Bandan Das <[email protected]>

---
 net/ipv4/ip_output.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

net/ipv4/ip_output.c +4 -0

index e427620..5fef4a9 100644
@@ -445,6 +445,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	struct iphdr *iph;
 	int ptr;
 	struct net_device *dev;
+	struct ip_options *opt;
 	struct sk_buff *skb2;
 	unsigned int mtu, hlen, left, len, ll_rs;
 	int offset;

@@ -462,6 +463,16 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 
 	if (unlikely((iph->frag_off & htons(IP_DF)) && !skb->local_df)) {
 		IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
+
+		/* Refill CB with IP options */
+		opt = &(IPCB(skb)->opt);

net/ipv4/ip_output.c int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *)) · line 478

Quoted 10 lines

net/ipv4/ip_output.c +7 -0

+		opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
+		if (ip_options_compile(dev_net(dev), opt, skb)) {
+			IP_INC_STATS(dev_net(dev), IPSTATS_MIB_INHDRERRORS);
+			/* If we fail, zero out IPCB and continue */
+			memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+		}
+
 		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
 			  htonl(ip_skb_dst_mtu(skb)));
 		kfree_skb(skb);

I wonder if we want this.

Maybe setting skb->local_df = 1 in bridge should be enough ?

Quoted 6 lines

I wonder if we want this.

Maybe setting skb->local_df = 1 in bridge should be enough ?

Thanks Eric for looking at this. Indeed, setting local_df to 1 seems to be enough! I will respin and post a different patch.

Quoted 6 lines · Bandan Das

Quoted 6 lines

I wonder if we want this.

Maybe setting skb->local_df = 1 in bridge should be enough ?

Quoted 3 lines

Thanks Eric for looking at this. Indeed, setting local_df to 1 seems to be enough! I will respin and post a different patch.

Reading this stuff again, I wonder if we should not revert commit 17762060c25590bfddd and use a different trick

Quoted 7 lines · Eric Dumazet

Quoted 6 lines · Bandan Das

Quoted 6 lines

I wonder if we want this.

Maybe setting skb->local_df = 1 in bridge should be enough ?

Quoted 6 lines

Quoted 3 lines

Thanks Eric for looking at this. Indeed, setting local_df to 1 seems to be enough! I will respin and post a different patch.

Reading this stuff again, I wonder if we should not revert commit 17762060c25590bfddd and use a different trick

I don't quite understand the problem. Does the packet actually have IP options? If not why is it trying to look at IP options?

If it does have options, then we should parse options in the bridge driver before passing it to the IP stack (or just drop the packet).

Once again, the IP stack -> bridge -> IP stack flow bites us, because bridge likes to dirty IPCB.

Quoted 3 lines · Eric Dumazet

Once again, the IP stack -> bridge -> IP stack flow bites us, because bridge likes to dirty IPCB.

OK, so we're talking about a locally transmitted packet, with IP options leaving the IP stack, entering bridging, and then reentering the IP stack?

In that case the packet should no longer be treated as an IP packet when it enters the bridge. So if it did have options and we want to support that in bridging then we need to parse IP options there as my comment suggested.

Quoted 5 lines · Herbert Xu

Quoted 3 lines · Eric Dumazet

Once again, the IP stack -> bridge -> IP stack flow bites us, because bridge likes to dirty IPCB.

Quoted 8 lines

OK, so we're talking about a locally transmitted packet, with IP options leaving the IP stack, entering bridging, and then reentering the IP stack?

In that case the packet should no longer be treated as an IP packet when it enters the bridge. So if it did have options and we want to support that in bridging then we need to parse IP options there as my comment suggested.

Bandan did not provide a full stack trace,
but I believe the problem was :

br_nf_dev_queue_xmit() -> ip_fragment -> icmp_send() -> ip_options_echo() : crash, because ip_options_echo take bridge CB as IPCB data.

http://www.spinics.net/lists/netdev/msg139370.html

Quoted 3 lines · Eric Dumazet

Once again, the IP stack -> bridge -> IP stack flow bites us, because bridge likes to dirty IPCB.

OK, so we're talking about a locally transmitted packet, with IP options leaving the IP stack, entering bridging, and then reentering the IP stack?

In that case the packet should no longer be treated as an IP packet when it enters the bridge. So if it did have options and we want to support that in bridging then we need to parse IP options there as my comment suggested.

We could have a padding in front of struct br_input_skb_cb to make sure we dont overwrite IP (4|6) CB in bridge ?

Something like this untested patch :

Therefore I'm inclined to agree with Herbert that we need to parse the options explicitly before invoke ip_fragment(). We must call it with an SKB in the state it expects, and that means with options parsing already performed.

.

Quoted 4 lines

Therefore I'm inclined to agree with Herbert that we need to parse the options explicitly before invoke ip_fragment(). We must call it with an SKB in the state it expects, and that means with options parsing already performed.

FWIW the packet probably doesn't even have IP options. What is happening here is that we've found yet another entry point from the bridge driver into the IP stack so we need to duplicate my original patch here.

From: Herbert Xu <[email protected]>
Date: Thu, 2 Sep 2010 07:30:56 +0800

Quoted 11 lines · David Miller

.

Quoted 4 lines

Therefore I'm inclined to agree with Herbert that we need to parse the options explicitly before invoke ip_fragment(). We must call it with an SKB in the state it expects, and that means with options parsing already performed.

FWIW the packet probably doesn't even have IP options. What is happening here is that we've found yet another entry point from the bridge driver into the IP stack so we need to duplicate my original patch here.

With that in mind I'm going to commit the following and queue it up to -stable too.

Thanks.

--------------------
bridge: Clear INET control block of SKBs passed into ip_fragment().

In a similar vain to commit 17762060c25590bfddd68cc1131f28ec720f405f ("bridge: Clear IPCB before possible entry into IP stack")

Any time we call into the IP stack we have to make sure the state there is as expected by the ipv4 code.

With help from Eric Dumazet and Herbert Xu.

Reported-by Brandan Das <[email protected]>

Signed-off-by David S. Miller <[email protected]>

---
 net/bridge/br_netfilter.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

net/bridge/br_netfilter.c +4 -2

index 5ed00bd..137f232 100644
@@ -761,9 +761,11 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
 	if (skb->nfct != NULL && skb->protocol == htons(ETH_P_IP) &&
 	    skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&
-	    !skb_is_gso(skb))
+	    !skb_is_gso(skb)) {
+		/* BUG: Should really parse the IP options here. */
+		memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
 		return ip_fragment(skb, br_dev_queue_push_xmit);
-	else
+	} else
 		return br_dev_queue_push_xmit(skb);
 }
 #else

Signature · 1 line

1.7.2.2

Sounds good, except for one thing :)
It should be: Reported-by: Bandan Das <[email protected]> (without the "r")

net/bridge/br_netfilter.c +4 -0

index 2c911c0..de44271 100644
@@ -759,9 +759,21 @@ static unsigned int br_nf_forward_arp(unsigned int hook, struct sk_buff *skb,
 #if defined(CONFIG_NF_CONNTRACK_IPV4) || defined(CONFIG_NF_CONNTRACK_IPV4_MODULE)
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 {
+       struct ip_options *opt;
+       struct iphdr *iph;
+       struct net_device *dev = skb->dev;
+
        if (skb->nfct != NULL && skb->protocol == htons(ETH_P_IP) &&
            skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&

1. Only parse options if ihl > 5.
2. Please audit the IP stack to ensure that this does not mangle the packet. We should not write to the packet here.
3. Please check whether SRR is handled correctly (see ip_rcv_options).

This should go into a helper function as this isn't the only entry point from the bridge into the IP stack.

Also it may be worth considering whether we should replace ip_fragment here with something that only refragments a frag_list since the only time we want to fragment here is if we reassembled an IP datagram due to netfilter.

Sorry for the late response. Here's a patch that I put up based on Herbert's suggestions. I ofcourse don't see the problem anymore after
commit 87f94b4e91dc042620c527f3c30c37e5127ef757 but a generic helper such as this can be used anytime the bridge code is sending a packet over to the IP layer. Compile tested only but based on responses, will test it before submitting a final change. Also added it at two places where I know we do send a packet over to the IP layer. I will add it at other places later as I come across them.

Quoted 8 lines

Sorry for the late response. Here's a patch that I put up based on Herbert's suggestions. I ofcourse don't see the problem anymore after
commit 87f94b4e91dc042620c527f3c30c37e5127ef757 but a generic helper such as this can be used anytime the bridge code is sending a packet over to the IP layer. Compile tested only but based on responses, will test it before submitting a final change. Also added it at two places where I know we do send a packet over to the IP layer. I will add it at other places later as I come across them.

Looks fine to me.

From: Herbert Xu <[email protected]>
Date: Fri, 17 Sep 2010 14:51:17 +0800

Quoted 10 lines · Bandan Das

Quoted 8 lines

Sorry for the late response. Here's a patch that I put up based on Herbert's suggestions. I ofcourse don't see the problem anymore after
commit 87f94b4e91dc042620c527f3c30c37e5127ef757 but a generic helper such as this can be used anytime the bridge code is sending a packet over to the IP layer. Compile tested only but based on responses, will test it before submitting a final change. Also added it at two places where I know we do send a packet over to the IP layer. I will add it at other places later as I come across them.

Quoted 1 line

Looks fine to me.

Bandan, please submit this formally with proper commit message, signoff, also the new function you added needs a minor coding style fix, there needs to be a space after "if" and the openning left parenthesis.

Thanks.