[PATCH] ip_tunnel: fix ip_tunnel_lookup

[PATCH] ip_tunnel: fix ip_tunnel_lookup

[Dmitry Popov]

This patch fixes 3 similar bugs where incoming packets might be routed into
wrong non-wildcard tunnels:

1) Consider the following setup:
    ip address add 1.1.1.1/24 dev eth0
    ip address add 1.1.1.2/24 dev eth0
    ip tunnel add ipip1 remote 2.2.2.2 local 1.1.1.1 mode ipip dev eth0
    ip link set ipip1 up

Incoming ipip packets from 2.2.2.2 were routed into ipip1 even if it has dst =
1.1.1.2. Moreover even if there was wildcard tunnel like
   ip tunnel add ipip0 remote 2.2.2.2 local any mode ipip dev eth0
but it was created before explicit one (with local 1.1.1.1), incoming ipip
packets with src = 2.2.2.2 and dst = 1.1.1.2 were still routed into ipip1.

Same issue existed with all tunnels that use ip_tunnel_lookup (gre, vti)

2)  ip address add 1.1.1.1/24 dev eth0
    ip tunnel add ipip1 remote 2.2.146.85 local 1.1.1.1 mode ipip dev eth0
    ip link set ipip1 up

Incoming ipip packets with dst = 1.1.1.1 were routed into ipip1, no matter what
src address is. Any remote ip address which has ip_tunnel_hash = 0 raised this 
issue, 2.2.146.85 is just an example, there are more than 4 million of them.
And again, wildcard tunnel like
   ip tunnel add ipip0 remote any local 1.1.1.1 mode ipip dev eth0
wouldn't be ever matched if it was created before explicit tunnel like above.

Gre & vti tunnels had the same issue.

3)  ip address add 1.1.1.1/24 dev eth0
    ip tunnel add gre1 remote 2.2.146.84 local 1.1.1.1 key 1 mode gre dev eth0
    ip link set gre1 up

Any incoming gre packet with key = 1 were routed into gre1, no matter what 
src/dst addresses are. Any remote ip address which has ip_tunnel_hash = 0 raised
the issue, 2.2.146.84 is just an example, there are more than 4 million of them.
Wildcard tunnel like
   ip tunnel add gre2 remote any local any key 1 mode gre dev eth0
wouldn't be ever matched if it was created before explicit tunnel like above.

All this stuff happened because while looking for a wildcard tunnel we didn't
check that matched tunnel is a wildcard one. Fixed.

Signed-off-by: Dmitry Popov <ixaphire@qrator.net>
---
 net/ipv4/ip_tunnel.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 54b6731..6f9de61 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -169,6 +169,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 
 	hlist_for_each_entry_rcu(t, head, hash_node) {
 		if (remote != t->parms.iph.daddr ||
+		    t->parms.iph.saddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
 			continue;
 
@@ -185,10 +186,11 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 	head = &itn->tunnels[hash];
 
 	hlist_for_each_entry_rcu(t, head, hash_node) {
-		if ((local != t->parms.iph.saddr &&
-		     (local != t->parms.iph.daddr ||
-		      !ipv4_is_multicast(local))) ||
-		    !(t->dev->flags & IFF_UP))
+		if ((local != t->parms.iph.saddr || t->parms.iph.daddr != 0) &&
+		    (local != t->parms.iph.daddr || !ipv4_is_multicast(local)))
+			continue;
+
+		if (!(t->dev->flags & IFF_UP))
 			continue;
 
 		if (!ip_tunnel_key_match(&t->parms, flags, key))
@@ -205,6 +207,8 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 
 	hlist_for_each_entry_rcu(t, head, hash_node) {
 		if (t->parms.i_key != key ||
+		    t->parms.iph.saddr != 0 ||
+		    t->parms.iph.daddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
 			continue;
 

Re: [PATCH] ip_tunnel: fix ip_tunnel_lookup

[David Miller]

From: Dmitry Popov <ixaphire@qrator.net>
Date: Sat, 5 Jul 2014 02:26:37 +0400

> @@ -205,6 +207,8 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
>  
>  	hlist_for_each_entry_rcu(t, head, hash_node) {
>  		if (t->parms.i_key != key ||
> +		    t->parms.iph.saddr != 0 ||
> +		    t->parms.iph.daddr != 0 ||
>  		    !(t->dev->flags & IFF_UP))
>  			continue;
>  

I don't really understand the logic of these tests.

Usually the canonical way to test these kinds of things is:

	if (parms->saddr && parms->saddr != saddr)
		goto no_match;

But you are signalling a non-match any time the address is not a
wildcard.

Why?

Re: [PATCH] ip_tunnel: fix ip_tunnel_lookup

[Dmitry Popov]

On Tue, 08 Jul 2014 15:12:10 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Dmitry Popov <ixaphire@qrator.net>
> Date: Sat, 5 Jul 2014 02:26:37 +0400
> 
> > @@ -205,6 +207,8 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> >  
> >  	hlist_for_each_entry_rcu(t, head, hash_node) {
> >  		if (t->parms.i_key != key ||
> > +		    t->parms.iph.saddr != 0 ||
> > +		    t->parms.iph.daddr != 0 ||
> >  		    !(t->dev->flags & IFF_UP))
> >  			continue;
> >  
> 
> I don't really understand the logic of these tests.
> 
> Usually the canonical way to test these kinds of things is:
> 
> 	if (parms->saddr && parms->saddr != saddr)
> 		goto no_match;
> 
> But you are signalling a non-match any time the address is not a
> wildcard.
> 
> Why?

Because that's exactly what I want: to skip any non-wildcard tunnels.

How I see ip_tunnel_lookup logic:
1) try to find exact match (and if found return this tunnel):
tunnel.saddr == iph.daddr && tunnel.daddr == iph.saddr && key_matched()
2) try to find matched (local) wildcard tunnel:
tunnel.saddr == any && tunnel.daddr == iph.saddr && key_matched()
3) try to find matched (remote) wildcard tunnel:
tunnel.saddr == iph.daddr && tunnel.daddr == any && key_matched()
(there is also a test for multicast tunnel, but let's skip it for simplicity)
4) try to find matched (full) wildcard tunnel:
tunnel.saddr == any && tunnel.daddr == any && key_matched()
5) if nothing found return default tunnel.

According to this logic, in 4th loop (the one you quoted) we have to test that
tunnel.daddr == any && tunnel.saddr == any. In my opinion those two new lines
are the best way to achieve it.

Re: [PATCH] ip_tunnel: fix ip_tunnel_lookup

[David Miller]

From: Dmitry Popov <ixaphire@qrator.net>
Date: Wed, 9 Jul 2014 04:47:59 +0400

> How I see ip_tunnel_lookup logic:
> 1) try to find exact match (and if found return this tunnel):
> tunnel.saddr == iph.daddr && tunnel.daddr == iph.saddr && key_matched()
> 2) try to find matched (local) wildcard tunnel:
> tunnel.saddr == any && tunnel.daddr == iph.saddr && key_matched()
> 3) try to find matched (remote) wildcard tunnel:
> tunnel.saddr == iph.daddr && tunnel.daddr == any && key_matched()
> (there is also a test for multicast tunnel, but let's skip it for simplicity)
> 4) try to find matched (full) wildcard tunnel:
> tunnel.saddr == any && tunnel.daddr == any && key_matched()
> 5) if nothing found return default tunnel.
> 
> According to this logic, in 4th loop (the one you quoted) we have to test that
> tunnel.daddr == any && tunnel.saddr == any. In my opinion those two new lines
> are the best way to achieve it.

Now it makes sense, thanks for explaining.

Applied and queued up for -stable, thanks again.