[PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[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 <bandan.das@stratus.com>
---
 net/ipv4/ip_output.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index e427620..5fef4a9 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -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);
+		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);
-- 
1.6.6.1

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le lundi 30 août 2010 à 16:09 -0400, Bandan Das a écrit :
> 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 <bandan.das@stratus.com>
> ---
>  net/ipv4/ip_output.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index e427620..5fef4a9 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -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);
> +		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 ?

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 30 août 2010 à 16:09 -0400, Bandan Das a écrit :
> > 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 <bandan.das@stratus.com>
> > ---
> >  net/ipv4/ip_output.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> > index e427620..5fef4a9 100644
> > --- a/net/ipv4/ip_output.c
> > +++ b/net/ipv4/ip_output.c
> > @@ -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);
> > +		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 ?
> 
> 
Thanks Eric for looking at this. Indeed, setting local_df to 1 seems to be enough! I will
respin and post a different patch.

-- 
Bandan Das

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > 
> > 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.
> 

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

Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Mon Jul 5 21:29:28 2010 +0000

    bridge: Clear IPCB before possible entry into IP stack
    
    The bridge protocol lives dangerously by having incestuous relations
    with the IP stack.  In this instance an abomination has been created
    where a bogus IPCB area from a bridged packet leads to a crash in
    the IP stack because it's interpreted as IP options.
    
    This patch papers over the problem by clearing the IPCB area in that
    particular spot.  To fix this properly we'd also need to parse any
    IP options if present but I'm way too lazy for that.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: David S. Miller <davem@davemloft.net>


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 :

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 2c911c0..9fdf1b1 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -598,9 +598,6 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 
 	pskb_trim_rcsum(skb, len);
 
-	/* BUG: Should really parse the IP options here. */
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
 		return NF_DROP;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 75c90ed..b27163a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -18,6 +18,7 @@
 #include <linux/netpoll.h>
 #include <linux/u64_stats_sync.h>
 #include <net/route.h>
+#include <linux/ipv6.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)
@@ -244,6 +245,8 @@ struct net_bridge
 };
 
 struct br_input_skb_cb {
+	struct inet6_skb_parm pad;
+
 	struct net_device *brdev;
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	int igmp;

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > 
> > 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.
> 

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

Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Mon Jul 5 21:29:28 2010 +0000

    bridge: Clear IPCB before possible entry into IP stack
    
    The bridge protocol lives dangerously by having incestuous relations
    with the IP stack.  In this instance an abomination has been created
    where a bogus IPCB area from a bridged packet leads to a crash in
    the IP stack because it's interpreted as IP options.
    
    This patch papers over the problem by clearing the IPCB area in that
    particular spot.  To fix this properly we'd also need to parse any
    IP options if present but I'm way too lazy for that.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
    Signed-off-by: David S. Miller <davem@davemloft.net>


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 :

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 2c911c0..9fdf1b1 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -598,9 +598,6 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 
 	pskb_trim_rcsum(skb, len);
 
-	/* BUG: Should really parse the IP options here. */
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
 		return NF_DROP;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 75c90ed..b27163a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -18,6 +18,7 @@
 #include <linux/netpoll.h>
 #include <linux/u64_stats_sync.h>
 #include <net/route.h>
+#include <linux/ipv6.h>
 
 #define BR_HASH_BITS 8
 #define BR_HASH_SIZE (1 << BR_HASH_BITS)
@@ -244,6 +245,8 @@ struct net_bridge
 };
 
 struct br_input_skb_cb {
+	struct inet6_skb_parm pad;
+
 	struct net_device *brdev;
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	int igmp;

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Tue, Aug 31, 2010 at 07:20:31AM +0200, Eric Dumazet wrote:
> Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > > 
> > > 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.
> > 
> 
> 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).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Tue, Aug 31, 2010 at 07:20:31AM +0200, Eric Dumazet wrote:
> Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > > 
> > > 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.
> > 
> 
> 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).

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le mardi 31 août 2010 à 16:24 +0800, Herbert Xu a écrit :
> On Tue, Aug 31, 2010 at 07:20:31AM +0200, Eric Dumazet wrote:
> > Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > > > 
> > > > 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.
> > > 
> > 
> > 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.

We can correct every bug we find once in a while (you did in commit
17762060c255 for a particular bug), or just make bridge not touch IPCB.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le mardi 31 août 2010 à 16:24 +0800, Herbert Xu a écrit :
> On Tue, Aug 31, 2010 at 07:20:31AM +0200, Eric Dumazet wrote:
> > Le lundi 30 août 2010 à 19:21 -0400, Bandan Das a écrit :
> > > > 
> > > > 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.
> > > 
> > 
> > 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.

We can correct every bug we find once in a while (you did in commit
17762060c255 for a particular bug), or just make bridge not touch IPCB.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
>
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
>
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le mardi 31 août 2010 à 20:36 +0800, Herbert Xu a écrit :
> On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
> >
> > 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.

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

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Eric Dumazet]

Le mardi 31 août 2010 à 20:36 +0800, Herbert Xu a écrit :
> On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
> >
> > 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.

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

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 31 août 2010 à 20:36 +0800, Herbert Xu a écrit :
> > On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
> > >
> > > 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.
> 
> 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
> 

That flow is correct. Sorry about the stack trace, it's now pasted 
below. But I am still wondering : Why not make sure in ip_fragment()
that we do not have a corrupt CB ? Today, it's the bridge, tomorrow it will be 
someone else that would be doing this.


08-31 09:38:23 [root@kvm ~]# Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff8146dd13
08-31 09:38:32 
08-31 09:38:32 Pid: 3209, comm: qemu-kvm Not tainted 2.6.32-63.el6.bdas.x86_64 #1
08-31 09:38:32 Call Trace:
08-31 09:38:32 <IRQ> 
08-31 09:38:32 Message from  [<ffffffff814c7fa5>] panic+0x78/0x137
08-31 09:38:32 syslogd@kvm at A [<ffffffff8146dd13>] ? icmp_send+0x743/0x780
08-31 09:38:32 ug 31 09:38:59 . [<ffffffff8106b30b>] __stack_chk_fail+0x1b/0x30
08-31 09:38:32 ..
08-31 09:38:32  kernel:Ker [<ffffffff8146dd13>] icmp_send+0x743/0x780
08-31 09:38:32 nel panic - not  [<ffffffffa03b9fe0>] ? br_nf_dev_queue_xmit+0x0/0x90 [bridge]
08-31 09:38:32 syncing: stack-p [<ffffffffa03b4400>] ? br_dev_queue_push_xmit+0x0/0xa0 [bridge]
08-31 09:38:32 rotector: Kernel [<ffffffff81436944>] ? nf_hook_slow+0x74/0x100
08-31 09:38:32  stack is corrup [<ffffffffa03ba060>] ? br_nf_dev_queue_xmit+0x80/0x90 [bridge]
08-31 09:38:32 ted in: ffffffff [<ffffffffa03baa90>] ? br_nf_post_routing+0x1d0/0x280 [bridge]
08-31 09:38:32 8146dd13
08-31 09:38:32 
08-31 09:38:32 M [<ffffffff8143688c>] ? nf_iterate+0x6c/0xb0
08-31 09:38:32 essage from sysl [<ffffffffa03b4400>] ? br_dev_queue_push_xmit+0x0/0xa0 [bridge]
08-31 09:38:32 ogd@kvm at Aug 3 [<ffffffff81436944>] ? nf_hook_slow+0x74/0x100
08-31 09:38:32 1 09:38:59 ... [<ffffffffa03b4400>] ? br_dev_queue_push_xmit+0x0/0xa0 [bridge]
08-31 09:38:32 
08-31 09:38:32  kernel:
08-31 09:38:32  [<ffffffffa03b44a0>] ? br_forward_finish+0x0/0x60 [bridge]
08-31 09:38:32  [<ffffffffa03b44e3>] ? br_forward_finish+0x43/0x60 [bridge]
08-31 09:38:32  [<ffffffffa03ba638>] ? br_nf_forward_finish+0x128/0x140 [bridge]
08-31 09:38:32  [<ffffffffa03bbb00>] ? br_nf_forward_ip+0x310/0x3c0 [bridge]
08-31 09:38:32  [<ffffffff8143688c>] ? nf_iterate+0x6c/0xb0
08-31 09:38:32  [<ffffffffa03b44a0>] ? br_forward_finish+0x0/0x60 [bridge]
08-31 09:38:32  [<ffffffff81436944>] ? nf_hook_slow+0x74/0x100
08-31 09:38:32  [<ffffffffa03b44a0>] ? br_forward_finish+0x0/0x60 [bridge]
08-31 09:38:32  [<ffffffffa03b4572>] ? __br_forward+0x72/0xc0 [bridge]
08-31 09:38:32  [<ffffffffa03b461d>] ? br_forward+0x5d/0x70 [bridge]
08-31 09:38:32  [<ffffffffa03b5359>] ? br_handle_frame_finish+0x129/0x260 [bridge]
08-31 09:38:32  [<ffffffffa03baf88>] ? br_nf_pre_routing_finish+0x228/0x340 [bridge]
08-31 09:38:32  [<ffffffff81436944>] ? nf_hook_slow+0x74/0x100
08-31 09:38:32  [<ffffffffa03bad60>] ? br_nf_pre_routing_finish+0x0/0x340 [bridge]
08-31 09:38:32  [<ffffffffa03bb4ef>] ? br_nf_pre_routing+0x44f/0x750 [bridge]
08-31 09:38:32  [<ffffffff8143688c>] ? nf_iterate+0x6c/0xb0
08-31 09:38:32  [<ffffffffa03b5230>] ? br_handle_frame_finish+0x0/0x260 [bridge]
08-31 09:38:32  [<ffffffff81436944>] ? nf_hook_slow+0x74/0x100
08-31 09:38:32  [<ffffffffa03b5230>] ? br_handle_frame_finish+0x0/0x260 [bridge]
08-31 09:38:32  [<ffffffffa03b561c>] ? br_handle_frame+0x18c/0x250 [bridge]
08-31 09:38:32  [<ffffffff8140f5e3>] ? netif_receive_skb+0x1c3/0x670
08-31 09:38:32  [<ffffffff81094962>] ? enqueue_hrtimer+0x82/0xd0
08-31 09:38:32  [<ffffffff8140fb13>] ? process_backlog+0x83/0xe0
08-31 09:38:32  [<ffffffff81410333>] ? net_rx_action+0x103/0x210
08-31 09:38:32  [<ffffffff81073597>] ? __do_softirq+0xb7/0x1e0
08-31 09:38:32  [<ffffffff810142cc>] ? call_softirq+0x1c/0x30
08-31 09:38:32  <EOI>  [<ffffffff81015f35>] ? do_softirq+0x65/0xa0
08-31 09:38:32  [<ffffffff814143a8>] ? netif_rx_ni+0x28/0x30
08-31 09:38:32  [<ffffffffa01bf445>] ? tun_chr_aio_write+0x295/0x580 [tun]
08-31 09:38:32  [<ffffffffa01bf1b0>] ? tun_chr_aio_write+0x0/0x580 [tun]
08-31 09:38:33  [<ffffffff8116c63b>] ? do_sync_readv_writev+0xfb/0x140
08-31 09:38:33  [<ffffffff810916d0>] ? autoremove_wake_function+0x0/0x40
08-31 09:38:33  [<ffffffff8120c51f>] ? selinux_file_permission+0xbf/0x150
08-31 09:38:33  [<ffffffff811ff936>] ? security_file_permission+0x16/0x20
08-31 09:38:33  [<ffffffff8116d6ff>] ? do_readv_writev+0xcf/0x1f0
08-31 09:38:33  [<ffffffff8117f94a>] ? do_vfs_ioctl+0x3aa/0x580
08-31 09:38:33  [<ffffffff8116d866>] ? vfs_writev+0x46/0x60
08-31 09:38:33  [<ffffffff8116d991>] ? sys_writev+0x51/0xb0
08-31 09:38:33  [<ffffffff81013172>] ? system_call_fastpath+0x16/0x1b
-- 


Bandan

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, Herbert Xu <herbert@gondor.hengli.com.au> wrote:
> On Tue, Aug 31, 2010 at 11:17:51AM +0200, Eric Dumazet wrote:
> >
> > 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.

Ok. So, I am not sure if re-exporting ip_compile_options is a 
good idea nor am I sure if replicating its behavior in a different 
function is.  It was removed from the list of exported symbols way
back in 2005. 

commit 0742fd53a3774781255bd1e471e7aa2e4a82d5f7
Author: Adrian Bunk <bunk@stusta.de>
Date:   Tue Aug 9 19:35:47 2005 -0700

    [IPV4]: possible cleanups
    
    This patch contains the following possible cleanups:
    - make needlessly global code static
    - #if 0 the following unused global function:
      - xfrm4_state.c: xfrm4_state_fini
    - remove the following unneeded EXPORT_SYMBOL's:
      - ip_output.c: ip_finish_output
      - ip_output.c: sysctl_ip_default_ttl
      - fib_frontend.c: ip_dev_find
      - inetpeer.c: inet_peer_idlock
      - ip_options.c: ip_options_compile
      - ip_options.c: ip_options_undo
      - net/core/request_sock.c: sysctl_max_syn_backlog

But, nevertheless, I moved the call to ip_options_compile to 
br_nf_dev_queue_xmit(). Does something like this look ok ?
(Previously sent patch : http://www.spinics.net/lists/kernel/msg1077537.html)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 2c911c0..de44271 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -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 &&
-           !skb_is_gso(skb))
+           !skb_is_gso(skb)) {
+               iph = ip_hdr(skb);
+               opt = &(IPCB(skb)->opt);
+               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);
+                       memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+               }
+       }
                return ip_fragment(skb, br_dev_queue_push_xmit);
        else
                return br_dev_queue_push_xmit(skb);
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ba9836c..72fe82c 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -466,7 +466,7 @@ error:
        }
        return -EINVAL;
 }
-
+EXPORT_SYMBOL(ip_options_compile);
 
 /*
  *     Undo all the changes done by ip_options_compile().

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 31 Aug 2010 07:20:31 +0200

> 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 :

This will not help Brandan's case.

His packets are coming straight from TUN/TAP.  They did not live in
the IP stack at all before hitting the bridge and then heading to
ip_fragment().

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.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 31 Aug 2010 07:20:31 +0200

> 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 :

This will not help Brandan's case.

His packets are coming straight from TUN/TAP.  They did not live in
the IP stack at all before hitting the bridge and then heading to
ip_fragment().

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.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 01, 2010 at 02:46:58PM -0700, David Miller wrote:
.
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 01, 2010 at 02:46:58PM -0700, David Miller wrote:
.
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 2 Sep 2010 07:30:56 +0800

> On Wed, Sep 01, 2010 at 02:46:58PM -0700, David Miller wrote:
> .
>> 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 <brandan.das@stratus.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/bridge/br_netfilter.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 5ed00bd..137f232 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -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
-- 
1.7.2.2

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 2 Sep 2010 07:30:56 +0800

> On Wed, Sep 01, 2010 at 02:46:58PM -0700, David Miller wrote:
> .
>> 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 <brandan.das@stratus.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/bridge/br_netfilter.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 5ed00bd..137f232 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -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
-- 
1.7.2.2

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, David Miller <davem@davemloft.net> wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 2 Sep 2010 07:30:56 +0800
> 
> > On Wed, Sep 01, 2010 at 02:46:58PM -0700, David Miller wrote:
> > .
> >> 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 <brandan.das@stratus.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/bridge/br_netfilter.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index 5ed00bd..137f232 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -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
> -- 
> 1.7.2.2
Sounds good, except for one thing :)
It should be:  Reported-by: Bandan Das <bandan.das@stratus.com> (without the "r")

Bandan

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Bandan Das <bandan.das@stratus.com>
Date: Wed, 1 Sep 2010 22:05:45 -0400

> Sounds good, except for one thing :)
> It should be:  Reported-by: Bandan Das <bandan.das@stratus.com> (without the "r")

Fixed, thanks!

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 01, 2010 at 12:57:43PM -0400, Bandan Das wrote:
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index 2c911c0..de44271 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -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 &&
> -           !skb_is_gso(skb))
> +           !skb_is_gso(skb)) {
> +               iph = ip_hdr(skb);
> +               opt = &(IPCB(skb)->opt);
> +               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);
> +                       memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> +               }
> +       }

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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 01, 2010 at 12:57:43PM -0400, Bandan Das wrote:
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index 2c911c0..de44271 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -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 &&
> -           !skb_is_gso(skb))
> +           !skb_is_gso(skb)) {
> +               iph = ip_hdr(skb);
> +               opt = &(IPCB(skb)->opt);
> +               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);
> +                       memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
> +               }
> +       }

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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, Herbert Xu <herbert@gondor.hengli.com.au> wrote:
> 
> 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.


diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 137f232..0a1f929 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -209,6 +209,72 @@ static inline void nf_bridge_update_protocol(struct sk_buff *skb)
 		skb->protocol = htons(ETH_P_PPP_SES);
 }
 
+/* When handing a packet over to the IP layer
+ * check whether we have a skb that is in the
+ * expected format
+ */
+
+int br_parse_ip_options(struct sk_buff *skb)
+{
+	struct ip_options *opt;
+	struct iphdr *iph;
+	struct net_device *dev = skb->dev;
+	u32 len;
+
+	iph = ip_hdr(skb);
+	opt = &(IPCB(skb)->opt);
+
+	/* Basic sanity checks */
+	if (iph->ihl < 5 || iph->version != 4)
+		goto inhdr_error;
+
+	if (!pskb_may_pull(skb, iph->ihl*4))
+		goto inhdr_error;
+
+	iph = ip_hdr(skb);
+	if(unlikely(ip_fast_csum((u8 *)iph, iph->ihl)))
+		goto inhdr_error;
+
+	len = ntohs(iph->tot_len);
+	if(skb->len < len) {
+		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INTRUNCATEDPKTS);
+		goto drop;
+	} else if (len < (iph->ihl*4))
+		goto inhdr_error;
+
+	if (pskb_trim_rcsum(skb, len)){
+		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS);
+		goto drop;
+	}
+
+	/* Zero out the CB buffer if no options present */
+	if (iph->ihl == 5){
+		memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+		return 0;
+	}
+
+	opt->optlen = iph->ihl*4 - sizeof(struct iphdr);
+	if(ip_options_compile(dev_net(dev), opt, skb))
+		goto inhdr_error;
+
+	/* Check correct handling of SRR option */
+	if (unlikely(opt->srr)) {
+		struct in_device *in_dev = __in_dev_get_rcu(dev);
+		if (in_dev && !IN_DEV_SOURCE_ROUTE(in_dev))
+			goto drop;
+
+		if(ip_options_rcv_srr(skb))
+			goto drop;
+	}
+
+	return 0;
+
+ inhdr_error:
+	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INHDRERRORS);
+ drop:
+	return -1;
+}
+
 /* Fill in the header for fragmented IP packets handled by
  * the IPv4 connection tracking code.
  */
@@ -549,7 +615,6 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 {
 	struct net_bridge_port *p;
 	struct net_bridge *br;
-	struct iphdr *iph;
 	__u32 len = nf_bridge_encap_header_len(skb);
 
 	if (unlikely(!pskb_may_pull(skb, len)))
@@ -578,28 +643,9 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 
 	nf_bridge_pull_encap_header_rcsum(skb);
 
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	iph = ip_hdr(skb);
-	if (iph->ihl < 5 || iph->version != 4)
-		goto inhdr_error;
-
-	if (!pskb_may_pull(skb, 4 * iph->ihl))
-		goto inhdr_error;
-
-	iph = ip_hdr(skb);
-	if (ip_fast_csum((__u8 *) iph, iph->ihl) != 0)
-		goto inhdr_error;
-
-	len = ntohs(iph->tot_len);
-	if (skb->len < len || len < 4 * iph->ihl)
-		goto inhdr_error;
-
-	pskb_trim_rcsum(skb, len);
-
-	/* BUG: Should really parse the IP options here. */
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
+	if (br_parse_ip_options(skb))
+		/* Drop invalid packet */
+		goto out;
 
 	nf_bridge_put(skb->nf_bridge);
 	if (!nf_bridge_alloc(skb))
@@ -614,8 +660,6 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
 
 	return NF_STOLEN;
 
-inhdr_error:
-//      IP_INC_STATS_BH(IpInHdrErrors);
 out:
 	return NF_DROP;
 }
@@ -759,14 +803,19 @@ 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)
 {
+	int ret;
+
 	if (skb->nfct != NULL && skb->protocol == htons(ETH_P_IP) &&
 	    skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&
 	    !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);
+		if(br_parse_ip_options(skb))
+			/* Drop invalid packet */
+			return NF_DROP;
+		ret = ip_fragment(skb, br_dev_queue_push_xmit);
 	} else
-		return br_dev_queue_push_xmit(skb);
+		ret = br_dev_queue_push_xmit(skb);
+
+	return ret;
 }
 #else
 static int br_nf_dev_queue_xmit(struct sk_buff *skb)
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index ba9836c..1906fa3 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -466,7 +466,7 @@ error:
 	}
 	return -EINVAL;
 }
-
+EXPORT_SYMBOL(ip_options_compile);
 
 /*
  *	Undo all the changes done by ip_options_compile().
@@ -646,3 +646,4 @@ int ip_options_rcv_srr(struct sk_buff *skb)
 	}
 	return 0;
 }
+EXPORT_SYMBOL(ip_options_rcv_srr);


--
Bandan

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 15, 2010 at 01:32:09PM -0400, Bandan Das wrote:
>
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Herbert Xu]

On Wed, Sep 15, 2010 at 01:32:09PM -0400, Bandan Das wrote:
>
> 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.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 17 Sep 2010 14:51:17 +0800

> On Wed, Sep 15, 2010 at 01:32:09PM -0400, Bandan Das wrote:
>>
>> 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.

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.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 17 Sep 2010 14:51:17 +0800

> On Wed, Sep 15, 2010 at 01:32:09PM -0400, Bandan Das wrote:
>>
>> 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.

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.

Re: [PATCH net-next-2.6] net/ipv4: push IP options to CB in ip_fragment

[Bandan Das]

On  0, David Miller <davem@davemloft.net> wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 17 Sep 2010 14:51:17 +0800
> 
> > On Wed, Sep 15, 2010 at 01:32:09PM -0400, Bandan Das wrote:
> >>
> >> 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.
> 
> 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.

Submitted. And also incorporated the coding style fixes you mentioned.

Thanks
Bandan