linux-kernel - [PATCH 3.2 050/147] ipv6: add rcu grace period before freeing fib6

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: <lsq.1510009387.890419163@decadent.org.uk>
Date:   Mon, 06 Nov 2017 23:03:07 +0000
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org, "Eric Dumazet" <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        "Martin KaFai Lau" <kafai@...com>, "Wei Wang" <weiwan@...gle.com>
Subject: [PATCH 3.2 050/147] ipv6: add rcu grace period before freeing
 fib6_node

3.2.95-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Wei Wang <weiwan@...gle.com>

commit c5cff8561d2d0006e972bd114afd51f082fee77c upstream.

We currently keep rt->rt6i_node pointing to the fib6_node for the route.
And some functions make use of this pointer to dereference the fib6_node
from rt structure, e.g. rt6_check(). However, as there is neither
refcount nor rcu taken when dereferencing rt->rt6i_node, it could
potentially cause crashes as rt->rt6i_node could be set to NULL by other
CPUs when doing a route deletion.
This patch introduces an rcu grace period before freeing fib6_node and
makes sure the functions that dereference it takes rcu_read_lock().

Note: there is no "Fixes" tag because this bug was there in a very
early stage.

Signed-off-by: Wei Wang <weiwan@...gle.com>
Acked-by: Eric Dumazet <edumazet@...gle.com>
Acked-by: Martin KaFai Lau <kafai@...com>
Signed-off-by: David S. Miller <davem@...emloft.net>
[bwh: Backported to 3.2: adjust context]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 include/net/ip6_fib.h | 30 +++++++++++++++++++++++++++++-
 net/ipv6/ip6_fib.c    | 20 ++++++++++++++++----
 net/ipv6/route.c      | 14 +++++++++++---
 3 files changed, 56 insertions(+), 8 deletions(-)

--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -63,6 +63,7 @@ struct fib6_node {
 	__u16			fn_flags;
 	__u32			fn_sernum;
 	struct rt6_info		*rr_ptr;
+	struct rcu_head		rcu;
 };
 
 #ifndef CONFIG_IPV6_SUBTREES
@@ -126,9 +127,36 @@ static inline struct inet6_dev *ip6_dst_
 	return ((struct rt6_info *)dst)->rt6i_idev;
 }
 
+/* Function to safely get fn->sernum for passed in rt
+ * and store result in passed in cookie.
+ * Return true if we can get cookie safely
+ * Return false if not
+ */
+static inline bool rt6_get_cookie_safe(const struct rt6_info *rt,
+				       u32 *cookie)
+{
+	struct fib6_node *fn;
+	bool status = false;
+
+	rcu_read_lock();
+	fn = rcu_dereference(rt->rt6i_node);
+
+	if (fn) {
+		*cookie = fn->fn_sernum;
+		status = true;
+	}
+
+	rcu_read_unlock();
+	return status;
+}
+
 static inline u32 rt6_get_cookie(const struct rt6_info *rt)
 {
-	return rt->rt6i_node ? rt->rt6i_node->fn_sernum : 0;
+	u32 cookie = 0;
+
+	rt6_get_cookie_safe(rt, &cookie);
+
+	return cookie;
 }
 
 struct fib6_walker_t {
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -153,11 +153,23 @@ static __inline__ struct fib6_node * nod
 	return fn;
 }
 
-static __inline__ void node_free(struct fib6_node * fn)
+static void node_free_immediate(struct fib6_node *fn)
 {
 	kmem_cache_free(fib6_node_kmem, fn);
 }
 
+static void node_free_rcu(struct rcu_head *head)
+{
+	struct fib6_node *fn = container_of(head, struct fib6_node, rcu);
+
+	kmem_cache_free(fib6_node_kmem, fn);
+}
+
+static void node_free(struct fib6_node *fn)
+{
+	call_rcu(&fn->rcu, node_free_rcu);
+}
+
 static __inline__ void rt6_release(struct rt6_info *rt)
 {
 	if (atomic_dec_and_test(&rt->rt6i_ref))
@@ -529,9 +541,9 @@ insert_above:
 
 		if (in == NULL || ln == NULL) {
 			if (in)
-				node_free(in);
+				node_free_immediate(in);
 			if (ln)
-				node_free(ln);
+				node_free_immediate(ln);
 			return NULL;
 		}
 
@@ -743,7 +755,7 @@ int fib6_add(struct fib6_node *root, str
 				   root, and then (in st_failure) stale node
 				   in main tree.
 				 */
-				node_free(sfn);
+				node_free_immediate(sfn);
 				goto st_failure;
 			}
 
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -964,10 +964,11 @@ struct dst_entry *ip6_blackhole_route(st
 static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 {
 	struct rt6_info *rt;
+	u32 rt_cookie;
 
 	rt = (struct rt6_info *) dst;
 
-	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) {
+	if (rt6_get_cookie_safe(rt, &rt_cookie) && rt_cookie == cookie) {
 		if (rt->rt6i_peer_genid != rt6_peer_genid()) {
 			if (!rt->rt6i_peer)
 				rt6_bind_peer(rt, 0);
@@ -1007,8 +1008,15 @@ static void ip6_link_failure(struct sk_b
 		if (rt->rt6i_flags&RTF_CACHE) {
 			dst_set_expires(&rt->dst, 0);
 			rt->rt6i_flags |= RTF_EXPIRES;
-		} else if (rt->rt6i_node && (rt->rt6i_flags & RTF_DEFAULT))
-			rt->rt6i_node->fn_sernum = -1;
+		} else {
+			struct fib6_node *fn;
+
+			rcu_read_lock();
+			fn = rcu_dereference(rt->rt6i_node);
+			if (fn && (rt->rt6i_flags & RTF_DEFAULT))
+				fn->fn_sernum = -1;
+			rcu_read_unlock();
+		}
 	}
 }