Re: net: heap out-of-bounds in fib6_clean_node/rt6_fill_node/fib6_age/fib6_prune_clone

[David Ahern <dsa@cumulusnetworks.com>]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

On 3/7/17 11:13 AM, Dmitry Vyukov wrote:
>> on this warning:
>>
>> /* dst.next really should not be set at this point */
>> if (rt->dst.next && rt->dst.next->ops->family != AF_INET6) {
>>         pr_warn("fib6_add: adding rt with bad next -- family %d dst
>> flags %x\n",
>>                 rt->dst.next->ops->family, rt->dst.next->flags);
>>
>>         WARN_ON(1);
>> }
>>
>> You should have seen the pr_warn in the log preceding the WARN_ON dump.
> 
> Right. They all have the same "IPv6: fib6_add: adding rt with bad next
> -- family 2 dst flags 6"

remove the previous changes and try the attached.

[-- Attachment #2: ipv6-debug.patch --]
[-- Type: text/plain, Size: 5112 bytes --]

diff --git a/include/net/dst.h b/include/net/dst.h
index 049af33da3b6..d164eb8ceab8 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -58,6 +58,7 @@ struct dst_entry {
 #define DST_XFRM_TUNNEL		0x0080
 #define DST_XFRM_QUEUE		0x0100
 #define DST_METADATA		0x0200
+#define DST_IN_FIB		0x0400
 
 	short			error;
 
diff --git a/include/net/dst_ops.h b/include/net/dst_ops.h
index c84b3287e38b..cd0df8f76420 100644
--- a/include/net/dst_ops.h
+++ b/include/net/dst_ops.h
@@ -15,6 +15,7 @@ struct dst_ops {
 	unsigned short		family;
 	unsigned int		gc_thresh;
 
+	void			(*dump)(struct dst_entry *);
 	int			(*gc)(struct dst_ops *ops);
 	struct dst_entry *	(*check)(struct dst_entry *, __u32 cookie);
 	unsigned int		(*default_advmss)(const struct dst_entry *);
diff --git a/net/core/dst.c b/net/core/dst.c
index 960e503b5a52..c98447fe8510 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -232,6 +232,9 @@ void __dst_free(struct dst_entry *dst)
 {
 	spin_lock_bh(&dst_garbage.lock);
 	___dst_free(dst);
+if (dst->flags & DST_IN_FIB)
+	pr_warn("dst %p is marked as in fib\n", dst);
+//WARN_ON(dst->flags & DST_IN_FIB);
 	dst->next = dst_garbage.list;
 	dst_garbage.list = dst;
 	if (dst_garbage.timer_inc > DST_GC_INC) {
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index e4266746e4a2..d4539d9a463e 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -155,6 +155,7 @@ static void node_free(struct fib6_node *fn)
 
 static void rt6_rcu_free(struct rt6_info *rt)
 {
+WARN_ON(rt->dst.flags & DST_IN_FIB);
 	call_rcu(&rt->dst.rcu_head, dst_rcu_free);
 }
 
@@ -878,6 +879,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 			return err;
 
 		rt->dst.rt6_next = iter;
+		rt->dst.flags |= DST_IN_FIB;
 		*ins = rt;
 		rt->rt6i_node = fn;
 		atomic_inc(&rt->rt6i_ref);
@@ -907,6 +909,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 		*ins = rt;
 		rt->rt6i_node = fn;
 		rt->dst.rt6_next = iter->dst.rt6_next;
+		rt->dst.flags |= DST_IN_FIB;
 		atomic_inc(&rt->rt6i_ref);
 		if (!info->skip_notify)
 			inet6_rt_notify(RTM_NEWROUTE, rt, info, NLM_F_REPLACE);
@@ -916,6 +919,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 		}
 		nsiblings = iter->rt6i_nsiblings;
 		fib6_purge_rt(iter, fn, info->nl_net);
+		iter->dst.flags &= ~DST_IN_FIB;
 		rt6_release(iter);
 
 		if (nsiblings) {
@@ -926,6 +930,7 @@ static int fib6_add_rt2node(struct fib6_node *fn, struct rt6_info *rt,
 				if (rt6_qualify_for_ecmp(iter)) {
 					*ins = iter->dst.rt6_next;
 					fib6_purge_rt(iter, fn, info->nl_net);
+					iter->dst.flags &= ~DST_IN_FIB;
 					rt6_release(iter);
 					nsiblings--;
 				} else {
@@ -974,6 +979,21 @@ int fib6_add(struct fib6_node *root, struct rt6_info *rt,
 			 !atomic_read(&rt->dst.__refcnt)))
 		return -EINVAL;
 
+if (rt->dst.ops->family != AF_INET6) {
+	pr_warn("fib6_add: adding rt with family is %d dst flags %x\n",
+		rt->dst.ops->family, rt->dst.flags);
+
+	WARN_ON(1);
+}
+/* dst.next really should not be set at this point */
+if (rt->dst.next && rt->dst.next->ops->family != AF_INET6) {
+	pr_warn("fib6_add: adding rt with bad next -- family %d dst flags %x\n",
+		rt->dst.next->ops->family, rt->dst.next->flags);
+
+	if (rt->dst.ops->dump)
+		rt->dst.ops->dump(&rt->dst);
+}
+
 	if (info->nlh) {
 		if (!(info->nlh->nlmsg_flags & NLM_F_CREATE))
 			allow_create = 0;
@@ -1444,6 +1464,7 @@ static void fib6_del_route(struct fib6_node *fn, struct rt6_info **rtp,
 	read_unlock(&net->ipv6.fib6_walker_lock);
 
 	rt->dst.rt6_next = NULL;
+	rt->dst.flags &= ~DST_IN_FIB;
 
 	/* If it was last route, expunge its radix tree node */
 	if (!fn->leaf) {
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 229bfcc451ef..e83b5ef7fbcd 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -238,10 +238,22 @@ static void ip6_confirm_neigh(const struct dst_entry *dst, const void *daddr)
 	__ipv6_confirm_neigh(dev, daddr);
 }
 
+static void ip6_dst_dump(struct dst_entry *dst)
+{
+	struct rt6_info *rt = (struct rt6_info *) dst;
+
+	pr_warn("rt %p: dev %s gw %pI6c dst %pI6c/%d src %pI6c prefsrc %pI6c flags %x rt6i_nsiblings %u\n",
+		rt, rt->rt6i_idev ? rt->rt6i_idev->dev->name : "<unknown>",
+		&rt->rt6i_gateway, &rt->rt6i_dst.addr, rt->rt6i_dst.plen,
+		&rt->rt6i_src.addr, &rt->rt6i_prefsrc.addr,
+		rt->rt6i_flags, rt->rt6i_nsiblings);
+}
+
 static struct dst_ops ip6_dst_ops_template = {
 	.family			=	AF_INET6,
 	.gc			=	ip6_dst_gc,
 	.gc_thresh		=	1024,
+	.dump			=	ip6_dst_dump,
 	.check			=	ip6_dst_check,
 	.default_advmss		=	ip6_default_advmss,
 	.mtu			=	ip6_mtu,
@@ -1135,6 +1147,8 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 
 		dst_hold(&uncached_rt->dst);
 
+		uncached_rt->dst.flags &= ~DST_IN_FIB;
+
 		trace_fib6_table_lookup(net, uncached_rt, table->tb6_id, fl6);
 		return uncached_rt;
 
@@ -1160,6 +1174,7 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 			dst_release(&rt->dst);
 		}
 
+		pcpu_rt->dst.flags &= ~DST_IN_FIB;
 		trace_fib6_table_lookup(net, pcpu_rt, table->tb6_id, fl6);
 		return pcpu_rt;