diff mbox series

[OpenWrt-Devel] kernel/generic: (try) fixing MAP-E patch misbehaving in 4.0

Message ID 7a0b483c-253b-b249-8b0f-93f74aabfb73@cgws.de
State Changes Requested
Delegated to: John Crispin
Headers show
Series [OpenWrt-Devel] kernel/generic: (try) fixing MAP-E patch misbehaving in 4.0 | expand

Commit Message

Axel Neumann Feb. 9, 2018, 12:20 p.m. UTC 
Hello,

the 666 kernel patches [1,2,3] break the possibility for using an ip4ip6
tunnel interface as a fall back interface accepting ip4-in-ip6 tunneled
packets from any remote address. This works out of the box with any
normal (non-666-patched) 4.4 (and earlier) kernel and can be configured
by setting up an 'ip -6 tunnel' with type 'any' or 'ip4ip6' and a remote
address of '::'.

The misbehavior comes with line 290 of [3] which discards all packets
that do not show the expected saddr, even if no single fmr rule was
defined and despite the validity of the saddr was already approved earlier.

Attached diff would re-enable this fall back capability without
affecting the behavior in case of any configured FMR rules.

It would be nice if the proposed or a similar fix could be applied asap
because currently I see no way of recovering the standard kernel
behavior which breaks certain desired bmx6 and bmx7 tunneling features.

Best regards
/axel


[1]
https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-3.18/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
[2]
https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.1/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
[3]
https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.4/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch#L290

Comments

John Crispin Feb. 12, 2018, 2:27 p.m. UTC | #1 
On 09/02/18 13:20, Axel Neumann wrote:
> Hello,
>
> the 666 kernel patches [1,2,3] break the possibility for using an ip4ip6
> tunnel interface as a fall back interface accepting ip4-in-ip6 tunneled
> packets from any remote address. This works out of the box with any
> normal (non-666-patched) 4.4 (and earlier) kernel and can be configured
> by setting up an 'ip -6 tunnel' with type 'any' or 'ip4ip6' and a remote
> address of '::'.
>
> The misbehavior comes with line 290 of [3] which discards all packets
> that do not show the expected saddr, even if no single fmr rule was
> defined and despite the validity of the saddr was already approved earlier.
>
> Attached diff would re-enable this fall back capability without
> affecting the behavior in case of any configured FMR rules.
>
> It would be nice if the proposed or a similar fix could be applied asap
> because currently I see no way of recovering the standard kernel
> behavior which breaks certain desired bmx6 and bmx7 tunneling features.
>
> Best regards
> /axel

Hi Axel,

the patch should be a unified one and you forgot to add your SoB line. 
best option is to generate the patch using git and also sending it out 
using git send-email.

     John

>
> [1]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-3.18/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> [2]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.1/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> [3]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.4/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch#L290
>
>
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
Daniel Golle May 23, 2018, 6:18 p.m. UTC | #2 
Hi Axel,
Hi Steven,

revisiting your patch, I try to understand what we need to fix here
or if the issue has already been resolved in the meantime.
See below:

On Fri, Feb 09, 2018 at 01:20:53PM +0100, Axel Neumann wrote:
> Hello,
> 
> the 666 kernel patches [1,2,3] break the possibility for using an ip4ip6
> tunnel interface as a fall back interface accepting ip4-in-ip6 tunneled
> packets from any remote address. This works out of the box with any
> normal (non-666-patched) 4.4 (and earlier) kernel and can be configured
> by setting up an 'ip -6 tunnel' with type 'any' or 'ip4ip6' and a remote
> address of '::'.
> 
> The misbehavior comes with line 290 of [3] which discards all packets
> that do not show the expected saddr, even if no single fmr rule was
> defined and despite the validity of the saddr was already approved earlier.
> 
> Attached diff would re-enable this fall back capability without
> affecting the behavior in case of any configured FMR rules.
> 
> It would be nice if the proposed or a similar fix could be applied asap
> because currently I see no way of recovering the standard kernel
> behavior which breaks certain desired bmx6 and bmx7 tunneling features.
> 
> Best regards
> /axel
> 
> 
> [1]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-3.18/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> [2]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.1/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> [3]
> https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.4/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch#L290
> 

The patch below is supposedly applied to net/ipv6/ip6_tunnel.c after
666-Add-support-for-MAP-E-FMRs-mesh-mode.patch has been applied, right?

> 1032c1032
> < 				if (fmr) {
> ---
> > 				if (fmr)
> 1036,1039c1036,1038
> < 					if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> < 						rcu_read_unlock();
> < 						goto discard;
> < 					}
> ---
> > 				if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> > 					rcu_read_unlock();
> > 					goto discard;

The original hunk in 666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
looks like this

---
@@ -832,6 +950,27 @@ static int __ip6_tnl_rcv(struct ip6_tnl
 	skb_reset_network_header(skb);
 	memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
 
+	if (tpi->proto == htons(ETH_P_IP) &&
+		!ipv6_addr_equal(&ipv6h->saddr, &tunnel->parms.raddr)) {
+			/* Packet didn't come from BR, so lookup FMR */
+			struct __ip6_tnl_fmr *fmr;
+			struct in6_addr expected = tunnel->parms.raddr;
+			for (fmr = tunnel->parms.fmrs; fmr; fmr = fmr->next)
+				if (ipv6_prefix_equal(&ipv6h->saddr,
+					&fmr->ip6_prefix, fmr->ip6_prefix_len))
+						break;
+
+			/* Check that IPv6 matches IPv4 source to prevent spoofing */
+			if (fmr)
+				ip4ip6_fmr_calc(&expected, ip_hdr(skb),
+						skb_tail_pointer(skb), fmr, false);
+
+			if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
+				rcu_read_unlock();
+				goto drop;
+			}
+	}
+
 	__skb_tunnel_rx(skb, tunnel->dev, tunnel->net);
 
 	err = dscp_ecn_decapsulate(tunnel, ipv6h, skb);
---

Reading this it is obvious that yout patch can be reverse-applied to
ip6_tunnel.c after 666-Add-support-for-MAP-E-FMRs-mesh-mode.patch has
been applied. As the MAP-E patch apparently hasn't been touched for
quite a while I assume that it must have looked the same at the time
when you sent the patch and you just used diff in a slightly
counter-intuitive way...

Just to make things crystal clear:
You are suggesting to change the patch above to rather be like:
---
@@ -832,6 +951,28 @@ static int __ip6_tnl_rcv(struct ip6_tnl
 	skb_reset_network_header(skb);
 	memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
 
+	if (tpi->proto == htons(ETH_P_IP) &&
+		!ipv6_addr_equal(&ipv6h->saddr, &tunnel->parms.raddr)) {
+			/* Packet didn't come from BR, so lookup FMR */
+			struct __ip6_tnl_fmr *fmr;
+			struct in6_addr expected = tunnel->parms.raddr;
+			for (fmr = tunnel->parms.fmrs; fmr; fmr = fmr->next)
+				if (ipv6_prefix_equal(&ipv6h->saddr,
+					&fmr->ip6_prefix, fmr->ip6_prefix_len))
+						break;
+
+			/* Check that IPv6 matches IPv4 source to prevent spoofing */
+			if (fmr) {
+				ip4ip6_fmr_calc(&expected, ip_hdr(skb),
+						skb_tail_pointer(skb), fmr, false);
+
+				if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
+					rcu_read_unlock();
+					goto drop;
+				}
+			}
+	}
+
 	__skb_tunnel_rx(skb, tunnel->dev, tunnel->net);
 
 	err = dscp_ecn_decapsulate(tunnel, ipv6h, skb);
---

Please confirm that this was your original intention, so we can move
forward with the discussion and this is an IPv6 show-stopper for
LibreMesh as well...


Cheers


Daniel
Daniel Golle July 30, 2018, 7:04 p.m. UTC | #3 
Hi Axel,

Can you make a pull-request towards gh.com/openwrt/openwrt (master)?
We can then backport this to 18.06, so it's a bit urgent.

Cheers

Daniel

On Thu, May 24, 2018 at 08:17:23PM +0200, Axel Neumann wrote:
> I meanwhile created another patch in a openwrt fork at github here 
> https://github.com/axn/openwrt/commit/8ea3fc4f808fe5e9aef50d082bd19095a3113750
> that should be more clean (
> there is a branch for for-master and for openwrt-18.06 branch).
>  But we want to do a few more tests before opening a pull request. Comments are welcome anyway. /axel
> 
> Am 23. Mai 2018 20:18:46 MESZ schrieb Daniel Golle <daniel@makrotopia.org>:
> >Hi Axel,
> >Hi Steven,
> >
> >revisiting your patch, I try to understand what we need to fix here
> >or if the issue has already been resolved in the meantime.
> >See below:
> >
> >On Fri, Feb 09, 2018 at 01:20:53PM +0100, Axel Neumann wrote:
> >> Hello,
> >> 
> >> the 666 kernel patches [1,2,3] break the possibility for using an
> >ip4ip6
> >> tunnel interface as a fall back interface accepting ip4-in-ip6
> >tunneled
> >> packets from any remote address. This works out of the box with any
> >> normal (non-666-patched) 4.4 (and earlier) kernel and can be
> >configured
> >> by setting up an 'ip -6 tunnel' with type 'any' or 'ip4ip6' and a
> >remote
> >> address of '::'.
> >> 
> >> The misbehavior comes with line 290 of [3] which discards all packets
> >> that do not show the expected saddr, even if no single fmr rule was
> >> defined and despite the validity of the saddr was already approved
> >earlier.
> >> 
> >> Attached diff would re-enable this fall back capability without
> >> affecting the behavior in case of any configured FMR rules.
> >> 
> >> It would be nice if the proposed or a similar fix could be applied
> >asap
> >> because currently I see no way of recovering the standard kernel
> >> behavior which breaks certain desired bmx6 and bmx7 tunneling
> >features.
> >> 
> >> Best regards
> >> /axel
> >> 
> >> 
> >> [1]
> >>
> >https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-3.18/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> >> [2]
> >>
> >https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.1/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> >> [3]
> >>
> >https://github.com/openwrt-mirror/openwrt/blob/master/target/linux/generic/patches-4.4/666-Add-support-for-MAP-E-FMRs-mesh-mode.patch#L290
> >> 
> >
> >The patch below is supposedly applied to net/ipv6/ip6_tunnel.c after
> >666-Add-support-for-MAP-E-FMRs-mesh-mode.patch has been applied, right?
> >
> >> 1032c1032
> >> < 				if (fmr) {
> >> ---
> >> > 				if (fmr)
> >> 1036,1039c1036,1038
> >> < 					if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> >> < 						rcu_read_unlock();
> >> < 						goto discard;
> >> < 					}
> >> ---
> >> > 				if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> >> > 					rcu_read_unlock();
> >> > 					goto discard;
> >
> >The original hunk in 666-Add-support-for-MAP-E-FMRs-mesh-mode.patch
> >looks like this
> >
> >---
> >@@ -832,6 +950,27 @@ static int __ip6_tnl_rcv(struct ip6_tnl
> > 	skb_reset_network_header(skb);
> > 	memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
> > 
> >+	if (tpi->proto == htons(ETH_P_IP) &&
> >+		!ipv6_addr_equal(&ipv6h->saddr, &tunnel->parms.raddr)) {
> >+			/* Packet didn't come from BR, so lookup FMR */
> >+			struct __ip6_tnl_fmr *fmr;
> >+			struct in6_addr expected = tunnel->parms.raddr;
> >+			for (fmr = tunnel->parms.fmrs; fmr; fmr = fmr->next)
> >+				if (ipv6_prefix_equal(&ipv6h->saddr,
> >+					&fmr->ip6_prefix, fmr->ip6_prefix_len))
> >+						break;
> >+
> >+			/* Check that IPv6 matches IPv4 source to prevent spoofing */
> >+			if (fmr)
> >+				ip4ip6_fmr_calc(&expected, ip_hdr(skb),
> >+						skb_tail_pointer(skb), fmr, false);
> >+
> >+			if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> >+				rcu_read_unlock();
> >+				goto drop;
> >+			}
> >+	}
> >+
> > 	__skb_tunnel_rx(skb, tunnel->dev, tunnel->net);
> > 
> > 	err = dscp_ecn_decapsulate(tunnel, ipv6h, skb);
> >---
> >
> >Reading this it is obvious that yout patch can be reverse-applied to
> >ip6_tunnel.c after 666-Add-support-for-MAP-E-FMRs-mesh-mode.patch has
> >been applied. As the MAP-E patch apparently hasn't been touched for
> >quite a while I assume that it must have looked the same at the time
> >when you sent the patch and you just used diff in a slightly
> >counter-intuitive way...
> >
> >Just to make things crystal clear:
> >You are suggesting to change the patch above to rather be like:
> >---
> >@@ -832,6 +951,28 @@ static int __ip6_tnl_rcv(struct ip6_tnl
> > 	skb_reset_network_header(skb);
> > 	memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
> > 
> >+	if (tpi->proto == htons(ETH_P_IP) &&
> >+		!ipv6_addr_equal(&ipv6h->saddr, &tunnel->parms.raddr)) {
> >+			/* Packet didn't come from BR, so lookup FMR */
> >+			struct __ip6_tnl_fmr *fmr;
> >+			struct in6_addr expected = tunnel->parms.raddr;
> >+			for (fmr = tunnel->parms.fmrs; fmr; fmr = fmr->next)
> >+				if (ipv6_prefix_equal(&ipv6h->saddr,
> >+					&fmr->ip6_prefix, fmr->ip6_prefix_len))
> >+						break;
> >+
> >+			/* Check that IPv6 matches IPv4 source to prevent spoofing */
> >+			if (fmr) {
> >+				ip4ip6_fmr_calc(&expected, ip_hdr(skb),
> >+						skb_tail_pointer(skb), fmr, false);
> >+
> >+				if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> >+					rcu_read_unlock();
> >+					goto drop;
> >+				}
> >+			}
> >+	}
> >+
> > 	__skb_tunnel_rx(skb, tunnel->dev, tunnel->net);
> > 
> > 	err = dscp_ecn_decapsulate(tunnel, ipv6h, skb);
> >---
> >
> >Please confirm that this was your original intention, so we can move
> >forward with the discussion and this is an IPv6 show-stopper for
> >LibreMesh as well...
> >
> >
> >Cheers
> >
> >
> >Daniel
> >
> >_______________________________________________
> >openwrt-devel mailing list
> >openwrt-devel@lists.openwrt.org
> >http://lists.infradead.org/mailman/listinfo/openwrt-devel
> 
> -- 
> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
diff mbox series

Patch

1032c1032
< 				if (fmr) {
---
> 				if (fmr)
1036,1039c1036,1038
< 					if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
< 						rcu_read_unlock();
< 						goto discard;
< 					}
---
> 				if (!ipv6_addr_equal(&ipv6h->saddr, &expected)) {
> 					rcu_read_unlock();
> 					goto discard;