Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[Florian Westphal]

David Newall <davidn@davidnewall.com> wrote:

[ remove lkml and cc nf-devel ]

> I tried to persevere with the commit: I recalculated checksum, which
> left routes and times improperly updated in options.  Then I tried
> calling ip_forward_options, which looks like it would correctly
> update RR and TS (not to mention checksum)m but that bombed because
> skb_rtable returned NULL.

Yes.  bridge<->netfilter wiring is pure duct tape.
The glue code will set up a fake rtable for the skb after the
prerouting hook. [ see br_nf_pre_routing_finish() ].

> I see three ways to progress:
> 
> 1. Possibly call ip_forward_option, but that requires somebody who
> understands this code to help;
> 2. Just recalculate the checksum, leaving crap in the options; or
> 3. Revert the commit.

I think none of these are an option.

I fail to understand why a bridge should honor/modifiy IP options.

For the 'local delivery' case the ip stack will take care of
option parsing, for forwarding it should be sufficient to do
sanity tests (for netfilters sake).

>From a quick glance, it should be sufficient to edit
br_parse_ip_options() and remove everything after

memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));

A 2nd step would be to move a copy of ip_options_compile()
into br_netfilter.c and trim it down to only validate the
ipv4 header without modifying it.

If there is a good reason to mangle options on a bridge i'd
prefer a comment explaining them...

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[Bart De Schuymer]

Florian Westphal schreef op 19/05/2014 19:09:
> David Newall <davidn@davidnewall.com> wrote:
>
> [ remove lkml and cc nf-devel ]
>
>> I tried to persevere with the commit: I recalculated checksum, which
>> left routes and times improperly updated in options.  Then I tried
>> calling ip_forward_options, which looks like it would correctly
>> update RR and TS (not to mention checksum)m but that bombed because
>> skb_rtable returned NULL.
>
> Yes.  bridge<->netfilter wiring is pure duct tape.
> The glue code will set up a fake rtable for the skb after the
> prerouting hook. [ see br_nf_pre_routing_finish() ].
>
>> I see three ways to progress:
>>
>> 1. Possibly call ip_forward_option, but that requires somebody who
>> understands this code to help;
>> 2. Just recalculate the checksum, leaving crap in the options; or
>> 3. Revert the commit.
>
> I think none of these are an option.
>
> I fail to understand why a bridge should honor/modifiy IP options.
>
> For the 'local delivery' case the ip stack will take care of
> option parsing, for forwarding it should be sufficient to do
> sanity tests (for netfilters sake).
>
>>From a quick glance, it should be sufficient to edit
> br_parse_ip_options() and remove everything after
>
> memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
>
> A 2nd step would be to move a copy of ip_options_compile()
> into br_netfilter.c and trim it down to only validate the
> ipv4 header without modifying it.

Perhaps it's possible to call ip_options_compile with a skb == NULL, 
like ip_options.c::ip_options_get_finish does. That way we don't need to 
duplicate code.
An alternative would be to make sure that the data pointed to by IPCB 
and BR_INPUT_SKB_CB don't overlap. If this were the case, we could 
indeed just revert the commit that was referred to.


cheers,
Bart

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

Hi Bart,

Thanks for thinking about this.

On 20/05/14 06:19, Bart De Schuymer wrote:
> Perhaps it's possible to call ip_options_compile with a skb == NULL, 
> like ip_options.c::ip_options_get_finish does. That way we don't need 
> to duplicate code.

I think not.  My reading of the discussion behind the commit is that skb 
cb area could contain something that was confused for IP options.  To 
solve that, and to allow for proper response when the packet's DF flag 
was set, the cb area was cleared and ip_options_compile() was called.  
Calling that function does only part of the job, leaving slots for 
addresses (and possibly timestamps) to be filled in by a later function; 
possibly ip_forward_options().  I did try calling that; it failed; 
skb_rtable() returned NULL.

I have also read enough comments deriding the "incestuous" relationship 
between bridge and IP to convince me that the relationship should be 
severed.  A bridge is such a simple concept which, when it starts 
looking into the payload, ceases to be a bridge.

I have experience in this code measured in hours; not a lot.  I welcome 
correction if I misunderstand things.

> An alternative would be to make sure that the data pointed to by IPCB 
> and BR_INPUT_SKB_CB don't overlap. If this were the case, we could 
> indeed just revert the commit that was referred to.

They are identical spaces, but you imply a good point: the cb area is 
possibly being used, simultaneously, for two, incompatible purposes.  
Yet another argument for divorcing bridge of ip logic.

Regards,

David

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[Bart De Schuymer]

David Newall schreef op 21/05/2014 9:49:
>> An alternative would be to make sure that the data pointed to by IPCB
>> and BR_INPUT_SKB_CB don't overlap. If this were the case, we could
>> indeed just revert the commit that was referred to.
>
> They are identical spaces, but you imply a good point: the cb area is
> possibly being used, simultaneously, for two, incompatible purposes. Yet
> another argument for divorcing bridge of ip logic.

There's no reason why they should overlap in the cb: it's 48 bytes big, 
so big enough to hold both struct br_input_skb_cb and struct 
inet_skb_parm. The original problem was introduced when BR_INPUT_SKB_CB 
was introduced (around Feb 27, 2010), so fixing BR_INPUT_SKB_CB seems 
most appropriate to me.
As for your other remark: as I've said before, if you don't like 
bridge-netfilter then don't compile it into your kernel.

Bart

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: Bart De Schuymer <bdschuym@pandora.be>
Date: Wed, 21 May 2014 20:51:14 +0200

> David Newall schreef op 21/05/2014 9:49:
>>> An alternative would be to make sure that the data pointed to by IPCB
>>> and BR_INPUT_SKB_CB don't overlap. If this were the case, we could
>>> indeed just revert the commit that was referred to.
>>
>> They are identical spaces, but you imply a good point: the cb area is
>> possibly being used, simultaneously, for two, incompatible
>> purposes. Yet
>> another argument for divorcing bridge of ip logic.
> 
> There's no reason why they should overlap in the cb: it's 48 bytes
> big, so big enough to hold both struct br_input_skb_cb and struct
> inet_skb_parm. The original problem was introduced when
> BR_INPUT_SKB_CB was introduced (around Feb 27, 2010), so fixing
> BR_INPUT_SKB_CB seems most appropriate to me.

So you are suggesting the patch below will fix everything?

> As for your other remark: as I've said before, if you don't like
> bridge-netfilter then don't compile it into your kernel.

That's never a good argument, please stop making it.

%99.999999999 of users get their kernels from distributions and
they are all going to enable basically every feature available.

We never should have added bridging netfilter to the tree in the
first place, I wish I had better judgment back then.

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 06811d7..2300def 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 <net/ip.h>
 #include <linux/if_vlan.h>
 
 #define BR_HASH_BITS 8
@@ -297,6 +298,7 @@ struct net_bridge
 };
 
 struct br_input_skb_cb {
+	struct inet_skb_parm ip;
 	struct net_device *brdev;
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 	int igmp;

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[Bart De Schuymer]

David Miller schreef op 21/05/2014 22:18:
> From: Bart De Schuymer <bdschuym@pandora.be>
>> There's no reason why they should overlap in the cb: it's 48 bytes
>> big, so big enough to hold both struct br_input_skb_cb and struct
>> inet_skb_parm. The original problem was introduced when
>> BR_INPUT_SKB_CB was introduced (around Feb 27, 2010), so fixing
>> BR_INPUT_SKB_CB seems most appropriate to me.
>
> So you are suggesting the patch below will fix everything?

Assuming:
- David Newall's worries about IPCB are incorrect
- you also revert the commit mentioned by David 
(462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge :
Sanitize skb before it enters the IP stack))

Then I give it a good chance the regression will be gone with your patch.

> We never should have added bridging netfilter to the tree in the
> first place, I wish I had better judgment back then.

Feel free to deprecate it. This is my last spare-time involvement.

Please apply following patch:

diff --git a/MAINTAINERS b/MAINTAINERS
index f5de16e..2369bae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3163,10 +3163,9 @@ S:	Maintained
  F:	drivers/scsi/eata_pio.*

  EBTABLES
-M:	Bart De Schuymer <bart.de.schuymer@pandora.be>
  L:	netfilter-devel@vger.kernel.org
  W:	http://ebtables.sourceforge.net/
-S:	Maintained
+S:	Orphan
  F:	include/linux/netfilter_bridge/ebt_*.h
  F:	include/uapi/linux/netfilter_bridge/ebt_*.h
  F:	net/bridge/netfilter/ebt*.c

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: Bart De Schuymer <bdschuym@pandora.be>
Date: Thu, 22 May 2014 20:57:13 +0200

> Please apply following patch:

This patch was corrupted and wouldn't apply cleanly.

But I applied it by hand for you, because clearly (as has been the
case for some time) you don't care.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 22/05/14 05:48, David Miller wrote:
> From: Bart De Schuymer<bdschuym@pandora.be>
> Date: Wed, 21 May 2014 20:51:14 +0200
> > There's no reason why they should overlap in the cb: it's 48 bytes
> > big, so big enough to hold both struct br_input_skb_cb and struct
> > inet_skb_parm. The original problem was introduced when
> > BR_INPUT_SKB_CB was introduced (around Feb 27, 2010), so fixing
> > BR_INPUT_SKB_CB seems most appropriate to me.
>
> So you are suggesting the patch below will fix everything?


First, of course I was wrong about ip overflowing the cb area.  Even I 
thought that was unlikely.  I've reread the code, much more carefully, 
and spotted where I went wrong.

I've added the change that David Miller provided, to those which I am 
proposing, and minimally tested them by pinging through a bridge with RR 
set.  No surprise: it works.

The patch now reverts the commit and mitigates the original problem by 
ensuring bridge's use of cb does not overlap ip's.

--- linux-source-3.13.0/net/bridge/br_netfilter.c.orig	2014-05-17 00:12:23.418906498 +0930
+++ linux-source-3.13.0/net/bridge/br_netfilter.c	2014-05-17 01:04:43.540972961 +0930
@@ -253,73 +253,6 @@ static inline void nf_bridge_update_prot
  		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
- */
-
-static int br_parse_ip_options(struct sk_buff *skb)
-{
-	struct ip_options *opt;
-	const struct iphdr *iph;
-	struct net_device *dev = skb->dev;
-	u32 len;
-
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	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;
-	}
-
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-	if (iph->ihl == 5)
-		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.
   */
@@ -679,6 +612,7 @@ static unsigned int br_nf_pre_routing(co
  {
  	struct net_bridge_port *p;
  	struct net_bridge *br;
+	const struct iphdr *iph;
  	__u32 len = nf_bridge_encap_header_len(skb);
  
  	if (unlikely(!pskb_may_pull(skb, len)))
@@ -704,10 +638,30 @@ static unsigned int br_nf_pre_routing(co
  		return NF_ACCEPT;
  
  	nf_bridge_pull_encap_header_rcsum(skb);
+
+	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+		return NF_DROP;
  
-	if (br_parse_ip_options(skb))
+	iph = ip_hdr(skb);
+	if (iph->ihl < 5 || iph->version != 4)
+		return NF_DROP;
+
+	if (!pskb_may_pull(skb, 4 * iph->ihl))
  		return NF_DROP;
  
+	iph = ip_hdr(skb);
+	if (ip_fast_csum((__u8 *) iph, iph->ihl) != 0)
+		return NF_DROP;
+
+	len = ntohs(iph->tot_len);
+	if (skb->len < len || len < 4 * iph->ihl)
+		return NF_DROP;
+
+	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;
@@ -806,9 +760,6 @@ static unsigned int br_nf_forward_ip(con
  		nf_bridge->mask |= BRNF_PKT_TYPE;
  	}
  
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
-		return NF_DROP;
-
  	/* The physdev module checks on this */
  	nf_bridge->mask |= BRNF_BRIDGED;
  	nf_bridge->physoutdev = skb->dev;
@@ -862,19 +813,14 @@ static unsigned int br_nf_forward_arp(co
  #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4)
  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)) {
-		if (br_parse_ip_options(skb))
-			/* Drop invalid packet */
-			return NF_DROP;
-		ret = ip_fragment(skb, br_dev_queue_push_xmit);
+		/* 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
-		ret = br_dev_queue_push_xmit(skb);
-
-	return ret;
+		return br_dev_queue_push_xmit(skb);
  }
  #else
  static int br_nf_dev_queue_xmit(struct sk_buff *skb)
--- linux-source-3.13.0/net/ipv4/ip_options.c.orig	2014-05-16 18:11:10.260370554 +0930
+++ linux-source-3.13.0/net/ipv4/ip_options.c	2014-05-17 01:01:56.738277137 +0930
@@ -475,7 +475,6 @@ error:
  	}
  	return -EINVAL;
  }
-EXPORT_SYMBOL(ip_options_compile);
  
  /*
   *	Undo all the changes done by ip_options_compile().
@@ -658,4 +657,3 @@ int ip_options_rcv_srr(struct sk_buff *s
  	}
  	return 0;
  }
-EXPORT_SYMBOL(ip_options_rcv_srr);
--- /usr/src/linux-source-3.13.0/net/bridge/br_private.h.orig	2014-05-24 13:51:09.269709831 +0930
+++ /usr/src/linux-source-3.13.0/net/bridge/br_private.h	2014-05-24 13:53:20.243551927 +0930
@@ -18,6 +18,7 @@
  #include <linux/netpoll.h>
  #include <linux/u64_stats_sync.h>
  #include <net/route.h>
+#include <net/ip.h>
  #include <linux/if_vlan.h>
  
  #define BR_HASH_BITS 8
@@ -304,6 +305,7 @@ struct net_bridge
  };
  
  struct br_input_skb_cb {
+	struct inet_skb_parm ip;	/* we don't interfere with ip's use of cb area */
  	struct net_device *brdev;
  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
  	int igmp;

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: David Newall <davidn@davidnewall.com>
Date: Sat, 24 May 2014 15:26:24 +0930

> The patch now reverts the commit and mitigates the original problem by
> ensuring bridge's use of cb does not overlap ip's.

This patch was substantially corrupted by your email client.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 25/05/14 03:13, David Miller wrote:
> This patch was substantially corrupted by your email client.

We should be sending these things as mime attachments.  Having to put 
patches inline is brittle, absurd and a waste of everyone's time.  Is 
there actually anybody here who doesn't have a mime-compatible MUA?

Trying again...

--- linux-source-3.13.0/net/bridge/br_netfilter.c.orig	2014-05-17 00:12:23.418906498 +0930
+++ linux-source-3.13.0/net/bridge/br_netfilter.c	2014-05-17 01:04:43.540972961 +0930
@@ -253,73 +253,6 @@ static inline void nf_bridge_update_prot
  		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
- */
-
-static int br_parse_ip_options(struct sk_buff *skb)
-{
-	struct ip_options *opt;
-	const struct iphdr *iph;
-	struct net_device *dev = skb->dev;
-	u32 len;
-
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	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;
-	}
-
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-	if (iph->ihl == 5)
-		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.
   */
@@ -679,6 +612,7 @@ static unsigned int br_nf_pre_routing(co
  {
  	struct net_bridge_port *p;
  	struct net_bridge *br;
+	const struct iphdr *iph;
  	__u32 len = nf_bridge_encap_header_len(skb);
  
  	if (unlikely(!pskb_may_pull(skb, len)))
@@ -704,10 +638,30 @@ static unsigned int br_nf_pre_routing(co
  		return NF_ACCEPT;
  
  	nf_bridge_pull_encap_header_rcsum(skb);
+
+	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+		return NF_DROP;
  
-	if (br_parse_ip_options(skb))
+	iph = ip_hdr(skb);
+	if (iph->ihl < 5 || iph->version != 4)
+		return NF_DROP;
+
+	if (!pskb_may_pull(skb, 4 * iph->ihl))
  		return NF_DROP;
  
+	iph = ip_hdr(skb);
+	if (ip_fast_csum((__u8 *) iph, iph->ihl) != 0)
+		return NF_DROP;
+
+	len = ntohs(iph->tot_len);
+	if (skb->len < len || len < 4 * iph->ihl)
+		return NF_DROP;
+
+	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;
@@ -806,9 +760,6 @@ static unsigned int br_nf_forward_ip(con
  		nf_bridge->mask |= BRNF_PKT_TYPE;
  	}
  
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
-		return NF_DROP;
-
  	/* The physdev module checks on this */
  	nf_bridge->mask |= BRNF_BRIDGED;
  	nf_bridge->physoutdev = skb->dev;
@@ -862,19 +813,14 @@ static unsigned int br_nf_forward_arp(co
  #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4)
  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)) {
-		if (br_parse_ip_options(skb))
-			/* Drop invalid packet */
-			return NF_DROP;
-		ret = ip_fragment(skb, br_dev_queue_push_xmit);
+		/* 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
-		ret = br_dev_queue_push_xmit(skb);
-
-	return ret;
+		return br_dev_queue_push_xmit(skb);
  }
  #else
  static int br_nf_dev_queue_xmit(struct sk_buff *skb)
--- linux-source-3.13.0/net/ipv4/ip_options.c.orig	2014-05-16 18:11:10.260370554 +0930
+++ linux-source-3.13.0/net/ipv4/ip_options.c	2014-05-17 01:01:56.738277137 +0930
@@ -475,7 +475,6 @@ error:
  	}
  	return -EINVAL;
  }
-EXPORT_SYMBOL(ip_options_compile);
  
  /*
   *	Undo all the changes done by ip_options_compile().
@@ -658,4 +657,3 @@ int ip_options_rcv_srr(struct sk_buff *s
  	}
  	return 0;
  }
-EXPORT_SYMBOL(ip_options_rcv_srr);
--- /usr/src/linux-source-3.13.0/net/bridge/br_private.h.orig	2014-05-24 13:51:09.269709831 +0930
+++ /usr/src/linux-source-3.13.0/net/bridge/br_private.h	2014-05-24 13:53:20.243551927 +0930
@@ -18,6 +18,7 @@
  #include <linux/netpoll.h>
  #include <linux/u64_stats_sync.h>
  #include <net/route.h>
+#include <net/ip.h>
  #include <linux/if_vlan.h>
  
  #define BR_HASH_BITS 8
@@ -304,6 +305,7 @@ struct net_bridge
  };
  
  struct br_input_skb_cb {
+	struct inet_skb_parm ip;	/* we don't interfere with ip's use of cb area */
  	struct net_device *brdev;
  #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
  	int igmp;

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: David Newall <davidn@davidnewall.com>
Date: Sun, 25 May 2014 12:02:03 +0930

> On 25/05/14 03:13, David Miller wrote:
>> This patch was substantially corrupted by your email client.
> 
> We should be sending these things as mime attachments.  Having to put
> patches inline is brittle, absurd and a waste of everyone's time.  Is
> there actually anybody here who doesn't have a mime-compatible MUA?

It makes replying and commenting inline easy.

It's not our problem that so many email clients make sending
plain unmolested ASCII text difficult.  But at least we've gone
out of our way to document how to do so in the kernel tree.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 25/05/14 12:32, David Miller wrote:
> From: David Newall<davidn@davidnewall.com>
> Date: Sun, 25 May 2014 12:02:03 +0930
>
>> On 25/05/14 03:13, David Miller wrote:
>>> This patch was substantially corrupted by your email client.
>> >We should be sending these things as mime attachments.
> It makes replying and commenting inline easy.

Patches are intrinsically corrupted by commenting on them inline, and 
that doesn't matter.  What does matter is when a patch that people will 
need to test is corrupted, and sending them as mime attachments is the 
best answer I know of.  It's trivial to copy and paste from an 
attachment to the body so that you can comment; far easier than copying 
and pasting a patch verbatim (i.e. without corrupting it.)


> It's not our problem that so many email clients make sending
> plain unmolested ASCII text difficult.

It wasn't the email client; it was the xfce-terminal copy that corrupted 
it.  It's proven to corrupt this patch in two different ways; the other, 
which I caught before send, was because of unreliable scrollback.

In fact it is our problem when we insist that patches be sent in a way 
which we know is brittle and error-prone; our problem and our fault.  
Just imagine if you could have back all of the time you've wasted 
looking at included code, only to discover that it had been corrupted in 
some way or another; and then multiply that by everybody else who's 
wasted time the same way.  The argument that it makes it easy to comment 
is unconvincing to me because the alternative is so easy.

I apologise for this noise as I don't believe this is something which 
will change any time soon; it will change, just not soon.  I'm quite 
willing to drop the issue.

RE: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Laight]

From: David Newall
> On 25/05/14 03:13, David Miller wrote:
> > This patch was substantially corrupted by your email client.
> 
> We should be sending these things as mime attachments.  Having to put
> patches inline is brittle, absurd and a waste of everyone's time.  Is
> there actually anybody here who doesn't have a mime-compatible MUA?

Yes - anyone using the email client from the world's largest desktop
computer software company.

It doesn't have any method for displaying text attachments.
It has a scheme for executing attachments, for which it will use
an interpreter based on the filename extension.
(Yes - this is why it is very good at propagating viruses.)

FWIW it can send valid patches quite easily - just copy/paste from wordpad.
(Possibly after hacking the registry to allow lines longer than 72 characters.

	David

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: David Newall <davidn@davidnewall.com>
Date: Sun, 25 May 2014 12:02:03 +0930

> +	pskb_trim_rcsum(skb, len);

You really need to check the return value as this can perform allocations,
GFP_ATOMIC ones in fact.

Also, why are we not bumping the statistics any more?  I didn't see a
discussion of that in this thread.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 30/05/14 08:04, David Miller wrote:
> You really need to check the return value as this can perform allocations,
> GFP_ATOMIC ones in fact.
>
> Also, why are we not bumping the statistics any more?  I didn't see a
> discussion of that in this thread.

I was only restoring the code as it was before the commit.  Maybe this, 
(instead of the previous patch of br_netfilter.c,) to keep the (added) 
check on pskb_may_pull's return value and incremented statistics?

--- br_netfilter.c	2014-05-30 18:01:40.221868365 +0930
+++ br_netfilter.c.orig	2014-05-30 18:17:39.697425383 +0930
@@ -253,73 +253,6 @@ static inline void nf_bridge_update_prot
  		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
- */
-
-static int br_parse_ip_options(struct sk_buff *skb)
-{
-	struct ip_options *opt;
-	const struct iphdr *iph;
-	struct net_device *dev = skb->dev;
-	u32 len;
-
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	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;
-	}
-
-	memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));
-	if (iph->ihl == 5)
-		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.
   */
@@ -679,6 +612,8 @@ static unsigned int br_nf_pre_routing(co
  {
  	struct net_bridge_port *p;
  	struct net_bridge *br;
+	const struct iphdr *iph;
+	struct net_device *dev = skb->dev;
  	__u32 len = nf_bridge_encap_header_len(skb);
  
  	if (unlikely(!pskb_may_pull(skb, len)))
@@ -704,9 +639,35 @@ static unsigned int br_nf_pre_routing(co
  		return NF_ACCEPT;
  
  	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) {
+		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INTRUNCATEDPKTS);
+		return NF_DROP;
+	} else if (len < (iph->ihl*4))
+		goto inhdr_error;
  
-	if (br_parse_ip_options(skb))
+	if (pskb_trim_rcsum(skb, len)) {
+		IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INDISCARDS);
  		return NF_DROP;
+	}
+
+	/* 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))
@@ -720,6 +681,10 @@ static unsigned int br_nf_pre_routing(co
  		br_nf_pre_routing_finish);
  
  	return NF_STOLEN;
+
+inhdr_error:
+	IP_INC_STATS_BH(dev_net(dev), IPSTATS_MIB_INHDRERRORS);
+	return NF_DROP;
  }
  
  
@@ -806,9 +771,6 @@ static unsigned int br_nf_forward_ip(con
  		nf_bridge->mask |= BRNF_PKT_TYPE;
  	}
  
-	if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
-		return NF_DROP;
-
  	/* The physdev module checks on this */
  	nf_bridge->mask |= BRNF_BRIDGED;
  	nf_bridge->physoutdev = skb->dev;
@@ -862,19 +824,14 @@ static unsigned int br_nf_forward_arp(co
  #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4)
  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)) {
-		if (br_parse_ip_options(skb))
-			/* Drop invalid packet */
-			return NF_DROP;
-		ret = ip_fragment(skb, br_dev_queue_push_xmit);
+		/* 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
-		ret = br_dev_queue_push_xmit(skb);
-
-	return ret;
+		return br_dev_queue_push_xmit(skb);
  }
  #else
  static int br_nf_dev_queue_xmit(struct sk_buff *skb)

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: David Newall <davidn@davidnewall.com>
Date: Fri, 30 May 2014 18:47:58 +0930

> On 30/05/14 08:04, David Miller wrote:
>> You really need to check the return value as this can perform
>> allocations,
>> GFP_ATOMIC ones in fact.
>>
>> Also, why are we not bumping the statistics any more?  I didn't see a
>> discussion of that in this thread.
> 
> I was only restoring the code as it was before the commit.  Maybe
> this, (instead of the previous patch of br_netfilter.c,) to keep the
> (added) check on pskb_may_pull's return value and incremented
> statistics?

I don't see why you don't simply keep br_parse_ip_options() around
and adjust it as you need, you're just mostly duplicating it's
contents into br_nf_pre_routing().

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 31/05/14 10:16, David Miller wrote:
> I don't see why you don't simply keep br_parse_ip_options() around
> and adjust it as you need, you're just mostly duplicating it's
> contents into br_nf_pre_routing().

More accurately, I'm *restoring* br_parse_ip_options()'s contents to 
br_nf_pre_routing().  The reasons why are twofold: I'm undoing a change 
which turns out to have been a mistake; and leaving it largely as-is, 
just removing the call to ip_options_compile(), would be confusing in 
that the name (br_pase_ip_options()) gives an expectation of function 
that would be untrue.

I can see an argument in favour of leaving br_parse_options() around, 
being that it is called from three places, and thus restoring the code 
removes checks which are currently being performed.  They weren't being 
performed before and it's not clear that they are needed, but if you say 
that it would be better, I'll leave it around and just remove the call 
to ip_options_compile(). Just say the word.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Miller]

From: David Newall <davidn@davidnewall.com>
Date: Sat, 31 May 2014 15:43:16 +0930

> On 31/05/14 10:16, David Miller wrote:
>> I don't see why you don't simply keep br_parse_ip_options() around
>> and adjust it as you need, you're just mostly duplicating it's
>> contents into br_nf_pre_routing().
> 
> More accurately, I'm *restoring* br_parse_ip_options()'s contents to
> br_nf_pre_routing().  The reasons why are twofold: I'm undoing a
> change which turns out to have been a mistake; and leaving it largely
> as-is, just removing the call to ip_options_compile(), would be
> confusing in that the name (br_pase_ip_options()) gives an expectation
> of function that would be untrue.
> 
> I can see an argument in favour of leaving br_parse_options() around,
> being that it is called from three places, and thus restoring the code
> removes checks which are currently being performed.  They weren't
> being performed before and it's not clear that they are needed, but if
> you say that it would be better, I'll leave it around and just remove
> the call to ip_options_compile(). Just say the word.

You can rename the function to something more suitable.

Because then it's just a handful of line changes rather than a huge
bunch of hunks which are harder to audit.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 22/05/14 04:21, Bart De Schuymer wrote:
> There's no reason why they should overlap in the cb: it's 48 bytes 
> big, so big enough to hold both struct br_input_skb_cb and struct 
> inet_skb_parm.

No reason, aside from the math, I think.  Those 48 bytes appear to be 
used for 16 bytes of ip_options plus up to 40 bytes of options data, so 
we're using pretend-space; of which we'd need more to squeeze 
br_input_skb_cb in at the same time.

I hate opening a second can of worms, but, if I read this right, IPCB is 
quite, quite broken.


> As for your other remark: as I've said before, if you don't like 
> bridge-netfilter then don't compile it into your kernel.

That's not very helpful.  I could say, with just as much merit, that it 
should be marked deprecated (so that it's not compiled into distribution 
kernels) and you can compile it into yours.

What I dislike is that bridge-netfilter is faulty.

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[Bart De Schuymer]

David Newall schreef op 22/05/2014 5:50:
> On 22/05/14 04:21, Bart De Schuymer wrote:
>> As for your other remark: as I've said before, if you don't like
>> bridge-netfilter then don't compile it into your kernel.
>
> That's not very helpful.  I could say, with just as much merit, that it
> should be marked deprecated (so that it's not compiled into distribution
> kernels) and you can compile it into yours.
>
> What I dislike is that bridge-netfilter is faulty.

I can see this may be frustrating to many. Now and then I actually got 
some positive feedback :-) Anyway, since I didn't have much sleep last 
night I'll be brief.
I'm fine with deprecating ebtables/bridge-nf. The code is over a decade 
old and development hasn't really picked up speed after I diverted my 
spare time to other things.
 From now on I'm no longer wasting my spare time on involvement in 
bridge-nf/ebtables. Anyone that wants to step up to take over the git 
repository (also contains arptables userspace app) can please contact me 
by private mail.

Best regards,
Bart

Re: Revert 462fb2af9788a82a534f8184abfde31574e1cfa0 (bridge : Sanitize skb before it enters the IP stack)

[David Newall]

On 20/05/14 02:39, Florian Westphal wrote:
>  From a quick glance, it should be sufficient to edit
> br_parse_ip_options() and remove everything after
>
> memset(IPCB(skb), 0, sizeof(struct inet_skb_parm));

Yes.  That's the way it used to be, and how it would return with the 
change I'm proposing.  The br_parse_ip_option function would be removed 
and its remaining code moved back from whence it came.


> A 2nd step would be to move a copy of ip_options_compile()
> into br_netfilter.c and trim it down to only validate the
> ipv4 header without modifying it.

The bridge sounds like the wrong place to validate an IPv4 header, 
unless it also validates every type of header; and that can't be right.  
That we need to zero the cb area seems like a big clue that IP's 
treatment of the area is lame.  I think that's where the problem lies, 
and that the right thing to do is to yank out the crap from bridge that 
papers over IP's weakness.