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]

Message-ID: <20200326185626.GA22732@dontpanic>
Date:   Thu, 26 Mar 2020 19:56:26 +0100
From:   William Dauchy <w.dauchy@...teo.com>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net] net, ip_tunnel: fix interface lookup with no key

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;



-- 
William