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

[Tung Quang Nguyen]

>Subject: [PATCH net v2] 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.
>
>The following trace was captured on 7.2.0-rc5-00284-gaf39eb111ce6:
>
>  BUG: KASAN: slab-use-after-free in tipc_link_is_establishing
>(net/tipc/link.c:285)
>  Read of size 4 at addr ffff88802e2aa068 by task swapper/2/0
>   tipc_link_is_establishing (net/tipc/link.c:285)
>   tipc_node_link_down (net/tipc/node.c:1076)
>   tipc_node_timeout (net/tipc/node.c:843)
>  Allocated by task 9549:
>   tipc_link_create (net/tipc/link.c:490)
>   tipc_node_check_dest (net/tipc/node.c:1279)
>   tipc_disc_rcv (net/tipc/discover.c:252)
>   tipc_udp_recv (net/tipc/udp_media.c:389)
>  Freed by task 9549:
>   tipc_node_link_down (net/tipc/node.c:1084)
>   tipc_node_delete_links (net/tipc/node.c:1320)
>   bearer_disable (net/tipc/bearer.c:414)
>   __tipc_nl_bearer_disable (net/tipc/bearer.c:992)
>
>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.
>

Reviewed-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>