Re: [tipc-discussion] [PATCH net] tipc: move bc link creation back to tipc_node_create

[Jon M. <jm...@re...>]

On 6/6/22 11:24, Xin Long wrote:
> Shuang Li reported a NULL pointer dereference crash:
>
>    [] BUG: kernel NULL pointer dereference, address: 0000000000000068
>    [] RIP: 0010:tipc_link_is_up+0x5/0x10 [tipc]
>    [] Call Trace:
>    []  <IRQ>
>    []  tipc_bcast_rcv+0xa2/0x190 [tipc]
>    []  tipc_node_bc_rcv+0x8b/0x200 [tipc]
>    []  tipc_rcv+0x3af/0x5b0 [tipc]
>    []  tipc_udp_recv+0xc7/0x1e0 [tipc]
>
> It was caused by the 'l' passed into tipc_bcast_rcv() is NULL. When it
> creates a node in tipc_node_check_dest(), after inserting the new node
> into hashtable in tipc_node_create(), it creates the bc link. However,
> there is a gap between this insert and bc link creation, a bc packet
> may come in and get the node from the hashtable then try to dereference
> its bc link, which is NULL.
>
> This patch is to fix it by moving the bc link creation before inserting
> into the hashtable.
>
> Note that for a preliminary node becoming "real", the bc link creation
> should also be called before it's rehashed, as we don't create it for
> preliminary nodes.
>
> Fixes: 4cbf8ac2fe5a ("tipc: enable creating a "preliminary" node")
> Reported-by: Shuang Li <sh...@re...>
> Signed-off-by: Xin Long <luc...@gm...>
> ---
>   net/tipc/node.c | 41 ++++++++++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 19 deletions(-)
>
> diff --git a/net/tipc/node.c b/net/tipc/node.c
> index 6ef95ce565bd..b48d97cbbe29 100644
> --- a/net/tipc/node.c
> +++ b/net/tipc/node.c
> @@ -472,8 +472,8 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>   				   bool preliminary)
>   {
>   	struct tipc_net *tn = net_generic(net, tipc_net_id);
> +	struct tipc_link *l, *snd_l = tipc_bc_sndlink(net);
>   	struct tipc_node *n, *temp_node;
> -	struct tipc_link *l;
>   	unsigned long intv;
>   	int bearer_id;
>   	int i;
> @@ -488,6 +488,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>   			goto exit;
>   		/* A preliminary node becomes "real" now, refresh its data */
>   		tipc_node_write_lock(n);
> +		if (!tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
> +					 tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
> +					 n->capabilities, &n->bc_entry.inputq1,
> +					 &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
> +			pr_warn("Broadcast rcv link refresh failed, no memory\n");
> +			tipc_node_write_unlock_fast(n);
> +			tipc_node_put(n);
> +			n = NULL;
> +			goto exit;
> +		}
>   		n->preliminary = false;
>   		n->addr = addr;
>   		hlist_del_rcu(&n->hash);
> @@ -567,7 +577,16 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
>   	n->signature = INVALID_NODE_SIG;
>   	n->active_links[0] = INVALID_BEARER_ID;
>   	n->active_links[1] = INVALID_BEARER_ID;
> -	n->bc_entry.link = NULL;
> +	if (!preliminary &&
> +	    !tipc_link_bc_create(net, tipc_own_addr(net), addr, peer_id, U16_MAX,
> +				 tipc_link_min_win(snd_l), tipc_link_max_win(snd_l),
> +				 n->capabilities, &n->bc_entry.inputq1,
> +				 &n->bc_entry.namedq, snd_l, &n->bc_entry.link)) {
> +		pr_warn("Broadcast rcv link creation failed, no memory\n");
> +		kfree(n);
> +		n = NULL;
> +		goto exit;
> +	}
>   	tipc_node_get(n);
>   	timer_setup(&n->timer, tipc_node_timeout, 0);
>   	/* Start a slow timer anyway, crypto needs it */
> @@ -1155,7 +1174,7 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   			  bool *respond, bool *dupl_addr)
>   {
>   	struct tipc_node *n;
> -	struct tipc_link *l, *snd_l;
> +	struct tipc_link *l;
>   	struct tipc_link_entry *le;
>   	bool addr_match = false;
>   	bool sign_match = false;
> @@ -1175,22 +1194,6 @@ void tipc_node_check_dest(struct net *net, u32 addr,
>   		return;
>   
>   	tipc_node_write_lock(n);
> -	if (unlikely(!n->bc_entry.link)) {
> -		snd_l = tipc_bc_sndlink(net);
> -		if (!tipc_link_bc_create(net, tipc_own_addr(net),
> -					 addr, peer_id, U16_MAX,
> -					 tipc_link_min_win(snd_l),
> -					 tipc_link_max_win(snd_l),
> -					 n->capabilities,
> -					 &n->bc_entry.inputq1,
> -					 &n->bc_entry.namedq, snd_l,
> -					 &n->bc_entry.link)) {
> -			pr_warn("Broadcast rcv link creation failed, no mem\n");
> -			tipc_node_write_unlock_fast(n);
> -			tipc_node_put(n);
> -			return;
> -		}
> -	}
>   
>   	le = &n->links[b->identity];
>   
Acked-by: Jon Maloy <jm...@re...>