[PATCH net] tipc: read le->link under the node lock in tipc_node_link_down()

[PATCH net] tipc: read le->link under the node lock in tipc_node_link_down()

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

tipc_node_link_down() caches the link pointer before taking n->lock:

	struct tipc_link *l = le->link;		/* unlocked */

	if (!l)
		return;
	tipc_node_write_lock(n);
	if (!tipc_link_is_establishing(l)) {	/* deref l */
	...
		tipc_link_reset(l);		/* write into l */
	if (delete) {
		kfree(l);
		le->link = NULL;

The delete=true caller frees that very object under n->lock, so the lock
does not protect the cached pointer against it:

 - CPU A, delete=false: tipc_rcv() on TIPC_LINK_DOWN_EVT, or the link
   supervision timer via tipc_node_timeout(), reads l unlocked and then
   dereferences it under n->lock;
 - CPU B, delete=true: netlink TIPC_NL_BEARER_DISABLE -> bearer_disable()
   -> tipc_node_delete_links() -> tipc_node_link_down(n, bearer_id, true)
   -> kfree(l).

The link is freed with plain kfree(), not kfree_rcu(), and for UDP bearers
disable_media() only schedules the asynchronous cleanup_bearer() work, so
its synchronize_net() runs after the links are already gone.  An in-flight
CPU A that has read l therefore dereferences freed memory once B frees it:
a use-after-free read in tipc_link_is_establishing(), and a use-after-free
write via tipc_link_reset() on the establishing branch.

  BUG: KASAN: slab-use-after-free in tipc_link_is_establishing+0x3f/0x50
  Read of size 4 at addr ffff888111f90868 by task swapper/3/0
   tipc_link_is_establishing+0x3f/0x50
   tipc_node_link_down+0x1a0/0x450
   tipc_node_timeout+0x63d/0xca0
  Allocated by task 7903:
   tipc_link_create+0x17e/0xf20
   tipc_node_check_dest+0x98b/0x1150
   tipc_disc_rcv+0x9f3/0x10c0
   tipc_udp_recv+0x42a/0x7e0
  Freed by task 7903:
   tipc_node_link_down+0x301/0x450
   tipc_node_delete_links+0xf5/0x240
   bearer_disable+0xc0/0x240
   __tipc_nl_bearer_disable+0x21f/0x380

Move the le->link read inside tipc_node_write_lock(), so it is serialised
against the kfree() in the delete path.  A racing teardown now either has
not run yet, and we see a valid link, or has already run, and we see NULL.

Fixes: 73f646cec354 ("tipc: delay ESTABLISH state event when link is established")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
 net/tipc/node.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/tipc/node.c b/net/tipc/node.c
index 8e4ef2630ae4..918e59362d07 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -1063,16 +1063,23 @@ static void tipc_node_link_down(struct tipc_node *n, int bearer_id, bool delete)
 {
 	struct tipc_link_entry *le = &n->links[bearer_id];
 	struct tipc_media_addr *maddr = NULL;
-	struct tipc_link *l = le->link;
 	int old_bearer_id = bearer_id;
 	struct sk_buff_head xmitq;
-
-	if (!l)
-		return;
+	struct tipc_link *l;
 
 	__skb_queue_head_init(&xmitq);
 
+	/* Read le->link under n->lock: the delete path below kfree()s it while
+	 * holding the same lock, so an unlocked read here could be raced by a
+	 * concurrent bearer teardown and leave us with a freed pointer.
+	 */
 	tipc_node_write_lock(n);
+	l = le->link;
+	if (!l) {
+		tipc_node_write_unlock(n);
+		return;
+	}
+
 	if (!tipc_link_is_establishing(l)) {
 		__tipc_node_link_down(n, &bearer_id, &xmitq, &maddr);
 	} else {
-- 
2.55.0