[PATCHv2,1/1] bridge: forward IPv6 fragmented packets when passing netfilter

Currently IPv6 fragmented packets are not forwarded on an ethernet bridge
with netfilter ip6_tables loaded. e.g. steps to reproduce

1) create a simple bridge like this

        modprobe br_netfilter
        brctl addbr br0
        brctl addif br0 eth0
        brctl addif br0 eth2
        ifconfig eth0 up
        ifconfig eth2 up
        ifconfig br0 up

2) place a host with an IPv6 address on each side of the bridge

        set IPv6 address on host A:
        ip -6 addr add fd01:2345:6789:1::1/64 dev eth0

        set IPv6 address on host B:
        ip -6 addr add fd01:2345:6789:1::2/64 dev eth0

3) run a simple ping command on host A with packets > MTU

        ping6 -s 4000 fd01:2345:6789:1::2

4) wait some time and run e.g. "ip6tables -t nat -nvL" on the bridge

IPv6 fragmented packets traverse the bridge cleanly until "ip6tables -t nat -nvL"
is run. As soon as it is run (and netfilter modules are loaded) IPv6 fragmented
packets do not traverse the bridge any more (you see no more responses in ping's
output).

Patch exports ip6_fragment() in include/net/ipv6.h and net/ipv6/ip6_output.c
to use it in net/bridge/br_netfilter.c's br_nf_dev_queue_xmit() for IPv6 packets
that need to be fragmented.

In net/bridge/br_netfilter.c br_nf_pre_routing_finish_ipv6() is changed to keep
track of fragment size and br_nf_dev_queue_xmit() checks for IPv6 packets that
need to be fragmented. br_parse_ip6_options() is introduced to do some validity
checks on the IPv6 packet before calling ip6_fragment() and is closely aligned
to IPv4 code as an example.
br_nf_dev_queue_xmit() is changed to contain the relevant code depending on
CONFIG_NF_DEFRAG_IPV4 or CONFIG_NF_DEFRAG_IPV6 being used both or each for
their own.

ip6_fragment() in net/ipv6/ip6_output.c yields a NULL pointer dereference
happening when handling packets coming from br_nf_dev_queue_xmit().  When
calling IP6_INC_STATS(), ip6_dst_idev(skb_dst(skb)) did crash the kernel
like this:

BUG: unable to handle kernel NULL pointer dereference at 000000000000037a
IP: [<ffffffff814cba2a>] ip6_fragment+0x99a/0x1290
PGD 3bc3f067 PUD 3bc12067 PMD 0
Oops: 0000 [#1] SMP
...

As suggested by Pablo Neira Ayuso net/bridge/br_private.h was changed to
prevent this issue by using a union to hold either fake_rtable or fake_rt6_info.

ip6_fragment() is further changed to use nf_bridge_mtu_reduction(skb) as it is
done in the IPv4 code.

After applying this patch, in the same setup as stated above fragmented IPv6
packets traverse the bridge even after running "ip6tables -t nat -nvL" / net-
filter modules loaded. This was tested using overlong IPv6 ICMP ping packets
as described above.
Some tests were performed crafting invalid headers (e.g. ipv6 version field
set to 5) to test checks in br_parse_ip6_options(), however these packets do
not seem to reach changed code parts and seem to be dropped at an earlier
stage (more testing needed with invalid / fuzzed packets).

Signed-off-by: Bernhard Thaler <bernhard.thaler@wvnet.at>
---

Changes since v1:
* removed changes to IP6_INC_STATS() calls in net/ipv6/ip6_output.c 
* changed net/bridge/br_private.h to use union for fake_rtable and fake_rt6_info
  as suggested by Pablo Neira Ayuso

 include/net/ipv6.h        |    1 +
 net/bridge/br_netfilter.c |   87 ++++++++++++++++++++++++++++++++++++++++-----
 net/bridge/br_private.h   |    6 +++-
 net/ipv6/ip6_output.c     |    1 +
 4 files changed, 85 insertions(+), 10 deletions(-)

Bernhard Thaler <bernhard.thaler@wvnet.at> wrote:
> +/* Equivalent to br_parse_ip_options for IPv6 */
> +
> +static int br_parse_ip6_options(struct sk_buff *skb)
> +{

Its not parsing options (yes, the ipv4 counterpart
is also	a misnomer, lets at least not repeat it?).

> +	const struct ipv6hdr *ip6h;
> +	struct net_device *dev = skb->dev;
> +	struct inet6_dev *idev = in6_dev_get(skb->dev);
> +	u32 len;
> +	u8 ip6h_len = sizeof(struct ipv6hdr);
> +
> +	if (!pskb_may_pull(skb, ip6h_len))
> +		goto inhdr_error;
> +
> +	ip6h = ipv6_hdr(skb);
> +
> +	/* Basic sanity checks
> +	 * check version
> +	 * check minimum header length (40 bytes)
> +	 * check total length
> +	 */
> +	if (ip6h->version != 6)
> +		goto inhdr_error;
> +
> +	if (!pskb_may_pull(skb, ip6h_len))
> +		goto inhdr_error;

This maypull call is not needed.

> +	len = ntohs(ip6h->payload_len) + ip6h_len;
> +
> +	if (skb->len < len) {
> +		IP6_INC_STATS_BH(dev_net(dev), idev,
> +				 IPSTATS_MIB_INTRUNCATEDPKTS);
> +		goto drop;
> +	} else if (len < ip6h_len) {

How can this evaluate to true?

I think this should be refactored with what we have in
br_nf_pre_routing_ipv6(); in fact br_nf_pre_routing_ipv6 says that there
is no ipv6 nat which isn't true anymore; I think we should at the very
least zero out skb->cb[] there as well.

Perhaps factor out some common br_ip6_validate() helper that does
whats in br_nf_pre_routing_ipv6.

> +		if (br_parse_ip6_options(skb))
> +			/* Drop invalid packet */
> +			return NF_DROP;

If we call br_parse_ip6_options() (or eqivalent) in PREROUTING, do we need to call
it again here?  It seems to me we validated skb already (Also true for ipv4 counterpart)?

The IP6CB reset is needed of course.

So, to summarize:

- Not sure we need br_parse_ip6_options; we only call it from
  br_nf_dev_queue_xmit(); as long as we validated things in prerouting
  earlier we should be fine.
- The existing validation calls in br_nf_pre_routing_ipv6() should at
  least also zero IP6CB.

I agree with the other changes (even though its ugly; don't have any better
solution either).

Cheers,
Florian
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html