netdev - Re: [PATCH net] net, ip_tunnel: fix interface lookup with no key

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Mar 2020 08:30:40 +0100
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     William Dauchy <w.dauchy@...teo.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] net, ip_tunnel: fix interface lookup with no key

Le 26/03/2020 à 19:56, William Dauchy a écrit :
> Hello Nicolas,
> 
> Thanks for your review.
> 
> On Thu, Mar 26, 2020 at 07:01:20PM +0100, Nicolas Dichtel wrote:
>> Hmm, removing this test may break some existing scenario. This flag is part of
>> the UAPI (for gre). Suppose that a tool configures a gre tunnel, leaves the key
>> uninitialized and set this flag. After this patch, the lookup may return
>> something else.
> 
> Indeed I was not sure about possible other impacts, as it seemed to not
> break iproute2 tooling, but it's true it could break other tools.
> But if we consider to remove the key test in that case, would it be
> acceptable to have something like:
> 
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index 74e1d964a615..8bdb9856d4c4 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -142,23 +142,32 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
>     cand = t;
>   }
> 
> - if (flags & TUNNEL_NO_KEY)
> -  goto skip_key_lookup;
> -
> - 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;
> + if (flags & TUNNEL_NO_KEY) {
> +  hlist_for_each_entry_rcu(t, head, hash_node) {
> +   if (t->parms.iph.saddr != 0 ||
> +       t->parms.iph.daddr != 0 ||
> +       !(t->dev->flags & IFF_UP))
> +    continue;
> 
>    if (t->parms.link == link)
>     return t;
>    else if (!cand)
>     cand = t;
> - }
> + } else {
> +
> +  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;
> +
> +   if (t->parms.link == link)
> +    return t;
> +   else if (!cand)
> +    cand = t;
> +  }
> 
> -skip_key_lookup:
>   if (cand)
>    return cand;
> 
Maybe less code duplication? Something like:

diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 74e1d964a615..cd4b84310d92 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -142,11 +142,8 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 			cand = t;
 	}

-	if (flags & TUNNEL_NO_KEY)
-		goto skip_key_lookup;
-
 	hlist_for_each_entry_rcu(t, head, hash_node) {
-		if (t->parms.i_key != key ||
+		if ((!(flags & TUNNEL_NO_KEY) && t->parms.i_key != key) ||
 		    t->parms.iph.saddr != 0 ||
 		    t->parms.iph.daddr != 0 ||
 		    !(t->dev->flags & IFF_UP))
@@ -158,7 +155,6 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
 			cand = t;
 	}

-skip_key_lookup:
 	if (cand)
 		return cand;