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

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: read le->link under the node lock in
>tipc_node_link_down()
>
>From: Jun Yang <junvyyang@xxxxxxxxxxx>
>
>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
>

Please update the commit message with decoded stack trace (using linux/scripts/decode_stacktrace.sh).

>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@xxxxxxxxxx
>Reported-by: TencentOS Corvus AI <corvus@xxxxxxxxxxx>
>Signed-off-by: Jun Yang <junvyyang@xxxxxxxxxxx>
>---
> 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];

Above node access also needs to be moved to below ' tipc_node_write_lock()'.

> 	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);

Please use 'tipc_node_write_unlock_fast()' instead.

>+		return;
>+	}
>+
> 	if (!tipc_link_is_establishing(l)) {
> 		__tipc_node_link_down(n, &bearer_id, &xmitq, &maddr);
> 	} else {
>--
>2.55.0
>