[V2] ax25: Fix refcount leaks caused by ax25_cb_del()

Commit Message

Duoming Zhou March 9, 2022, 11 p.m. UTC

The previous commit d01ffb9eee4a ("ax25: add refcount in ax25_dev to
avoid UAF bugs") and commit feef318c855a ("ax25: fix UAF bugs of
net_device caused by rebinding operation") increase the refcounts of
ax25_dev and net_device in ax25_bind() and decrease the matching refcounts
in ax25_kill_by_device() in order to prevent UAF bugs. But there are
reference count leaks.

If we use ax25_bind() to increase the refcounts of ax25_dev and
net_device, then, use ax25_cb_del() invoked by ax25_destroy_socket()
to delete ax25_cb in hlist before calling ax25_kill_by_device(), the
decrements of refcounts in ax25_kill_by_device() will not be executed,
because ax25_cb is deleted from the hlist.

This patch adds two flags in ax25_dev in order to prevent reference
count leaks. If we bind successfully, then, use ax25_cb_del() to
delete ax25_cb, the two "test_bit" condition checks in ax25_kill_by_device()
could pass and the refcounts could be decreased properly.

Fixes: d01ffb9eee4a ("ax25: add refcount in ax25_dev to avoid UAF bugs")
Fixes: feef318c855a ("ax25: fix UAF bugs of net_device caused by rebinding operation")
Reported-by: Thomas Osterried <[e-mail]>
Signed-off-by: Duoming Zhou <[e-mail]>
---
Changes in V2:
  - Change memory leak to refcount leak.

 include/net/ax25.h  |  7 +++++--
 net/ax25/af_ax25.c  | 10 ++++++----
 net/ax25/ax25_dev.c |  3 ++-
 3 files changed, 13 insertions(+), 7 deletions(-)

--
2.17.1

Patch

diff --git a/include/net/ax25.h b/include/net/ax25.h
index 8221af1811d..50b3eacada0 100644
--- a/include/net/ax25.h
+++ b/include/net/ax25.h
@@ -157,7 +157,9 @@  enum {
 #define AX25_DEF_PACLEN		256			/* Paclen=256 */
 #define	AX25_DEF_PROTOCOL	AX25_PROTO_STD_SIMPLEX	/* Standard AX.25 */
 #define AX25_DEF_DS_TIMEOUT	180000			/* DAMA timeout 3 minutes */
-
+#define AX25_DEV_INIT    0
+#define AX25_DEV_KILL    1
+#define AX25_DEV_BIND    2
 typedef struct ax25_uid_assoc {
 	struct hlist_node	uid_node;
 	refcount_t		refcount;
@@ -240,8 +242,9 @@  typedef struct ax25_dev {
 	ax25_dama_info		dama;
 #endif
 	refcount_t		refcount;
+	unsigned long   kill_flag;
+	unsigned long   bind_flag;
 } ax25_dev;
-
 typedef struct ax25_cb {
 	struct hlist_node	ax25_node;
 	ax25_address		source_addr, dest_addr;
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 6bd09718077..5519448378d 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -86,6 +86,7 @@  static void ax25_kill_by_device(struct net_device *dev)
 again:
 	ax25_for_each(s, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
+			set_bit(AX25_DEV_KILL, &ax25_dev->kill_flag);
 			sk = s->sk;
 			if (!sk) {
 				spin_unlock_bh(&ax25_list_lock);
@@ -114,9 +115,12 @@  static void ax25_kill_by_device(struct net_device *dev)
 			goto again;
 		}
 	}
+	if(!test_bit(AX25_DEV_KILL, &ax25_dev->kill_flag) && test_bit(AX25_DEV_BIND, &ax25_dev->bind_flag)) {
+		dev_put_track(ax25_dev->dev, &ax25_dev->dev_tracker);
+		ax25_dev_put(ax25_dev);
+	}
 	spin_unlock_bh(&ax25_list_lock);
 }
-
 /*
  *	Handle device status changes.
  */
@@ -1132,13 +1136,11 @@  static int ax25_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 done:
 	ax25_cb_add(ax25);
 	sock_reset_flag(sk, SOCK_ZAPPED);
-
+	set_bit(AX25_DEV_BIND, &ax25_dev->bind_flag);
 out:
 	release_sock(sk);
-
 	return err;
 }
-
 /*
  *	FIXME: nonblock behaviour looks like it may have a bug.
  */
diff --git a/net/ax25/ax25_dev.c b/net/ax25/ax25_dev.c
index d2a244e1c26..7c40914e5c9 100644
--- a/net/ax25/ax25_dev.c
+++ b/net/ax25/ax25_dev.c
@@ -77,7 +77,8 @@  void ax25_dev_device_up(struct net_device *dev)
 	ax25_dev->values[AX25_VALUES_PACLEN]	= AX25_DEF_PACLEN;
 	ax25_dev->values[AX25_VALUES_PROTOCOL]  = AX25_DEF_PROTOCOL;
 	ax25_dev->values[AX25_VALUES_DS_TIMEOUT]= AX25_DEF_DS_TIMEOUT;
-
+	ax25_dev->kill_flag = AX25_DEV_INIT;
+	ax25_dev->bind_flag = AX25_DEV_INIT;
 #if defined(CONFIG_AX25_DAMA_SLAVE) || defined(CONFIG_AX25_DAMA_MASTER)
 	ax25_ds_setup_timer(ax25_dev);
 #endif