Forwarded: [PATCH] netrom: fix memory leak in nr_add_node()

[syzbot <syzbot+3f2d46b6e62b8dd546d3@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] netrom: fix memory leak in nr_add_node()
Author: kartikey406@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

When nr_add_node() creates a new neighbor but the route quality is too
low to be added (node already has 3 routes with better quality), the
newly allocated neighbor is never used but remains in the neighbor list
with refcount=1, causing a memory leak.

Also fix the same leak in the error path when nr_node allocation fails
after creating a new neighbor.

Fix by tracking whether a new neighbor was allocated and removing it
from the list if it was not used (count == 0) or on allocation failure.

Add debug printk to trace nr_rt_device_down cleanup.

Reported-by: syzbot+3f2d46b6e62b8dd546d3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3f2d46b6e62b8dd546d3
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 net/netrom/af_netrom.c |  3 +++
 net/netrom/nr_route.c  | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 5ed1a71ceec1..ecb47c8e2f0e 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -116,11 +116,14 @@ static int nr_device_event(struct notifier_block *this, unsigned long event, voi
 {
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
+	printk(KERN_ERR "nr_device_event: dev=%s event=%lu\n", dev->name, event);
+
 	if (!net_eq(dev_net(dev), &init_net))
 		return NOTIFY_DONE;
 
 	if (event != NETDEV_DOWN)
 		return NOTIFY_DONE;
+	printk(KERN_ERR "nr_device_event: calling nr_rt_device_down for %s\n", dev->name);
 
 	nr_kill_by_device(dev);
 	nr_rt_device_down(dev);
diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index b94cb2ffbaf8..1fac8c6e5c26 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -100,11 +100,16 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 {
 	struct nr_node  *nr_node;
 	struct nr_neigh *nr_neigh;
+	bool new_neigh = false;
 	int i, found;
 	struct net_device *odev;
 
+	printk(KERN_ERR "nr_add_node: entered, quality=%d\n", quality);
+
+
 	if ((odev=nr_dev_get(nr)) != NULL) {	/* Can't add routes to ourself */
 		dev_put(odev);
+		printk(KERN_ERR "nr_add_node: exit EINVAL (route to self)\n");
 		return -EINVAL;
 	}
 
@@ -112,6 +117,8 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 
 	nr_neigh = nr_neigh_get_dev(ax25, dev);
 
+	printk(KERN_ERR "nr_add_node: nr_node=%p, nr_neigh=%p\n", nr_node, nr_neigh);
+
 	/*
 	 * The L2 link to a neighbour has failed in the past
 	 * and now a frame comes from this neighbour. We assume
@@ -172,6 +179,8 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 			}
 		}
 
+		new_neigh = true;
+		printk(KERN_ERR "nr_add_node: created new neighbor\n");
 		spin_lock_bh(&nr_neigh_list_lock);
 		hlist_add_head(&nr_neigh->neigh_node, &nr_neigh_list);
 		nr_neigh_hold(nr_neigh);
@@ -182,9 +191,14 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 		nr_neigh->quality = quality;
 
 	if (nr_node == NULL) {
+		printk(KERN_ERR "nr_add_node: creating new node\n");
 		if ((nr_node = kmalloc(sizeof(*nr_node), GFP_ATOMIC)) == NULL) {
-			if (nr_neigh)
+			printk(KERN_ERR "nr_add_node: node alloc failed\n");
+			if (nr_neigh) {
+				if (new_neigh)
+					nr_remove_neigh(nr_neigh);
 				nr_neigh_put(nr_neigh);
+			}
 			return -ENOMEM;
 		}
 
@@ -209,6 +223,7 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 		spin_unlock_bh(&nr_node_list_lock);
 
 		nr_neigh_put(nr_neigh);
+		printk(KERN_ERR "nr_add_node: exit after new node, neigh count=%d\n", nr_neigh->count);
 		return 0;
 	}
 	nr_node_lock(nr_node);
@@ -225,9 +240,12 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 		}
 	}
 
+	printk(KERN_ERR "nr_add_node: found=%d, nr_node->count=%d\n", found, nr_node->count);
+
 	if (!found) {
 		/* We have space at the bottom, slot it in */
 		if (nr_node->count < 3) {
+			printk(KERN_ERR "nr_add_node: adding route (count < 3)\n");
 			nr_node->routes[2] = nr_node->routes[1];
 			nr_node->routes[1] = nr_node->routes[0];
 
@@ -242,6 +260,7 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 		} else {
 			/* It must be better than the worst */
 			if (quality > nr_node->routes[2].quality) {
+				printk(KERN_ERR "nr_add_node: replacing worst route\n");
 				nr_node->routes[2].neighbour->count--;
 				nr_neigh_put(nr_node->routes[2].neighbour);
 
@@ -255,6 +274,9 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 				nr_neigh_hold(nr_neigh);
 				nr_neigh->count++;
 			}
+			else {
+				printk(KERN_ERR "nr_add_node: quality too low, not adding\n");
+			}
 		}
 	}
 
@@ -279,6 +301,15 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
 		}
 	}
 
+	printk(KERN_ERR "nr_add_node: end, new_neigh=%d, count=%d\n", new_neigh, nr_neigh->count);
+
+	if (new_neigh && nr_neigh->count == 0) {
+		printk(KERN_ERR "nr_add_node: end, new_neigh=%d, count=%d\n", new_neigh, nr_neigh->count);
+		nr_remove_neigh(nr_neigh);
+	} else if (new_neigh) {
+		printk(KERN_ERR "nr_add_node: new_neigh used, count=%d\n", nr_neigh->count);
+	}
+
 	nr_neigh_put(nr_neigh);
 	nr_node_unlock(nr_node);
 	nr_node_put(nr_node);
@@ -511,14 +542,19 @@ void nr_rt_device_down(struct net_device *dev)
 	struct nr_node  *t;
 	int i;
 
+	printk(KERN_ERR "nr_rt_device_down: start for dev=%s (%p)\n", dev->name, dev);
+
 	spin_lock_bh(&nr_neigh_list_lock);
 	nr_neigh_for_each_safe(s, nodet, &nr_neigh_list) {
+		printk(KERN_ERR "nr_rt_device_down: checking neigh dev=%p vs %p\n", s->dev, dev);
 		if (s->dev == dev) {
+			printk(KERN_ERR "nr_rt_device_down: MATCH - removing neigh\n");
 			spin_lock_bh(&nr_node_list_lock);
 			nr_node_for_each_safe(t, node2t, &nr_node_list) {
 				nr_node_lock(t);
 				for (i = 0; i < t->count; i++) {
 					if (t->routes[i].neighbour == s) {
+						printk(KERN_ERR "nr_rt_device_down: removing route %d\n", i);
 						t->count--;
 
 						switch (i) {
@@ -543,6 +579,7 @@ void nr_rt_device_down(struct net_device *dev)
 			nr_remove_neigh_locked(s);
 		}
 	}
+	printk(KERN_ERR "nr_rt_device_down: done\n");
 	spin_unlock_bh(&nr_neigh_list_lock);
 }
 
-- 
2.43.0