RE: [PATCH net v3 2/2] tipc: fix NULL deref in tipc_named_node_up() on empty publication list

[Tung Quang Nguyen]

>named_distribute() ends by stamping the last_bulk flag on the tail skb via
>buf_msg(skb_peek_tail(list)). When the publication list is empty no skb is
>enqueued, skb_peek_tail() returns NULL, and buf_msg(NULL) is dereferenced.
>
>tipc_named_node_up() runs this on &nt->cluster_scope. With a node-id
>configuration cluster_scope is populated only later by tipc_net_finalize(), so a
>peer link that comes up first reaches named_distribute() with an empty list. It
>is reachable by an unprivileged user (TIPC genl ops use
>GENL_UNS_ADMIN_PERM) over a UDP bearer in a user+net namespace:
>
> KASAN: null-ptr-deref in range [0x00000000000000d8-0x00000000000000df]
> RIP: 0010:tipc_named_node_up (net/tipc/name_distr.c:196)
>  tipc_named_node_up (net/tipc/name_distr.c:196 net/tipc/name_distr.c:221)
>  tipc_node_write_unlock (net/tipc/node.c:428)
>  tipc_rcv (net/tipc/node.c:2185)
>  tipc_udp_recv (net/tipc/udp_media.c:392)  Kernel panic - not syncing: Fatal
>exception in interrupt
>
>The peer holds back this node's later name updates until it sees a bulk with the
>last_bulk flag, so simply skipping the empty bulk would stall it.
>Emit an item-less bulk when the list is empty, and break out of the build loop
>on allocation failure instead of returning, so the last_bulk flag is applied to the
>last queued skb.
>
>Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
>Reported-by: Xiang Mei <xmei5@xxxxxxx>
>Assisted-by: Claude:claude-opus-4-8
>Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
>---
> net/tipc/name_distr.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c index
>ba4f4906e13b..dbcfa965de34 100644
>--- a/net/tipc/name_distr.c
>+++ b/net/tipc/name_distr.c
>@@ -165,7 +165,7 @@ static void named_distribute(struct net *net, struct
>sk_buff_head *list,
> 						dnode);
> 			if (!skb) {
> 				pr_warn("Bulk publication failure\n");
>-				return;
>+				break;
> 			}
> 			hdr = buf_msg(skb);
> 			msg_set_bc_ack_invalid(hdr, true);
>@@ -192,6 +192,20 @@ static void named_distribute(struct net *net, struct
>sk_buff_head *list,
> 		skb_trim(skb, INT_H_SIZE + (msg_dsz - msg_rem));
> 		__skb_queue_tail(list, skb);
> 	}
>+
>+	if (skb_queue_empty(list)) {
>+		skb = named_prepare_buf(net, PUBLICATION, 0, dnode);
>+		if (!skb) {
>+			pr_warn("Bulk publication failure\n");
>+			return;
>+		}

This approach is wrong because:
1. When 'list' is empty, it is caused by memory allocation failure before. So, it is likely that 'skb' could be NULL again because of memory allocation failure.
2. Even if 'skb' is not NULL, allocation of non-data (zero-in-size) message will break the receiving peer when it handles this message.

>+		hdr = buf_msg(skb);
>+		msg_set_bc_ack_invalid(hdr, true);
>+		msg_set_bulk(hdr);
>+		msg_set_non_legacy(hdr);
>+		__skb_queue_tail(list, skb);
>+	}
>+
> 	hdr = buf_msg(skb_peek_tail(list));
> 	msg_set_last_bulk(hdr);
> 	msg_set_named_seqno(hdr, seqno);
>--
>2.43.0