RE: [PATCH net v8 1/3] tipc: fix NULL deref in tipc_named_node_up() on empty publication list

[Tung Quang Nguyen]

>Subject: [PATCH net v8 1/3] tipc: fix NULL deref in tipc_named_node_up() on
>empty publication list
>
>[The defer-to-workqueue approach is by Tung Nguyen. He posted it  on the
>thread and asked us to test it, as the replacement for the  item-less bulk
>approach. Since the RFC only exists as an inline  diff in the thread, it is folded
>into this series so the fix is  self-contained.]
>
>named_distribute() stamps the last_bulk flag on the tail skb of the publication
>list. When the list is empty no skb is enqueued and the tail access dereferences
>NULL. tipc_named_node_up() hits this on an empty cluster_scope, which
>happens with a node-id configuration where cluster_scope is populated only
>later by tipc_net_finalize(). It is reachable by an unprivileged user over a UDP
>bearer in a user+net namespace. The reported crash:
>
> 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
>
>When cluster_scope is empty at node-up, defer the bulk distribution to a
>workqueue and wait for tipc_net_finalize() to publish the node-state name, so
>named_distribute() always runs on a non-empty list. On allocation failure,
>purge the partially built queue and bring the link down so the bulk distribution
>restarts when the link comes up again.
>
>Fixes: cad2929dc432 ("tipc: update a binding service via broadcast")
>Reported-by: Xiang Mei <xmei5@xxxxxxx>
>Reported-by: kernel test robot <lkp@xxxxxxxxx>
>Closes: https://lore.kernel.org/oe-kbuild-all/202607180730.TwVgASDI-
>lkp@xxxxxxxxx/
>Signed-off-by: Tung Nguyen <tung.quang.nguyen@xxxxxxxx>
>Tested-by: Weiming Shi <bestswngs@xxxxxxxxx>
>Signed-off-by: Weiming Shi <bestswngs@xxxxxxxxx>
>---
sashiko reports many critical/high issues: https://sashiko.dev/#/patchset/20260718092544.785289-1-bestswngs%40gmail.com

I address all in below patch. Could you please test it ?

---
 net/tipc/core.c       |  2 ++
 net/tipc/core.h       |  4 +++
 net/tipc/name_distr.c | 67 +++++++++++++++++++++++++++++++++++++------
 net/tipc/name_distr.h |  3 +-
 net/tipc/net.c        | 15 +++++++++-
 net/tipc/node.c       | 61 +++++++++++++++++++++++++++++++++++++--
 6 files changed, 139 insertions(+), 13 deletions(-)

diff --git a/net/tipc/core.c b/net/tipc/core.c
index 315975c3be81..52544b805dcc 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -61,6 +61,8 @@ static int __net_init tipc_init_net(struct net *net)
        tn->trial_addr = 0;
        tn->addr_trial_end = 0;
        tn->capabilities = TIPC_NODE_CAPABILITIES;
+       atomic_set(&tn->finalized, 0);
+       atomic_set(&tn->work_rescheduled, 0);
        INIT_WORK(&tn->work, tipc_net_finalize_work);
        memset(tn->node_id, 0, sizeof(tn->node_id));
        memset(tn->node_id_string, 0, sizeof(tn->node_id_string));
diff --git a/net/tipc/core.h b/net/tipc/core.h
index 9ce5f9ff6cc0..52fdb9189cc3 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -145,6 +145,10 @@ struct tipc_net {
        struct work_struct work;
        /* The numbers of work queues in schedule */
        atomic_t wq_count;
+       /* flag to indicate work has finished */
+       atomic_t finalized;
+       /* flag to reschedule work */
+       atomic_t work_rescheduled;
 };

 static inline struct tipc_net *tipc_net(struct net *net)
diff --git a/net/tipc/name_distr.c b/net/tipc/name_distr.c
index ba4f4906e13b..3e32445cfbd0 100644
--- a/net/tipc/name_distr.c
+++ b/net/tipc/name_distr.c
@@ -147,8 +147,8 @@ struct sk_buff *tipc_named_withdraw(struct net *net, struct publication *p)
  * @pls: linked list of publication items to be packed into buffer chain
  * @seqno: sequence number for this message
  */
-static void named_distribute(struct net *net, struct sk_buff_head *list,
-                            u32 dnode, struct list_head *pls, u16 seqno)
+static int named_distribute(struct net *net, struct sk_buff_head *list,
+                           u32 dnode, struct list_head *pls, u16 seqno)
 {
        struct publication *publ;
        struct sk_buff *skb = NULL;
@@ -164,8 +164,9 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
                        skb = named_prepare_buf(net, PUBLICATION, msg_rem,
                                                dnode);
                        if (!skb) {
+                               __skb_queue_purge(list);
                                pr_warn("Bulk publication failure\n");
-                               return;
+                               return 1;
                        }
                        hdr = buf_msg(skb);
                        msg_set_bc_ack_invalid(hdr, true);
@@ -195,15 +196,16 @@ static void named_distribute(struct net *net, struct sk_buff_head *list,
        hdr = buf_msg(skb_peek_tail(list));
        msg_set_last_bulk(hdr);
        msg_set_named_seqno(hdr, seqno);
+
+       return 0;
 }

 /**
- * tipc_named_node_up - tell specified node about all publications by this node
+ * tipc_named_distribute - distribute all publications to specified node
  * @net: the associated network namespace
  * @dnode: destination node
- * @capabilities: peer node's capabilities
  */
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
+static int tipc_named_distribute(struct net *net, u32 dnode)
 {
        struct name_table *nt = tipc_name_table(net);
        struct tipc_net *tn = tipc_net(net);
@@ -212,15 +214,62 @@ void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)

        __skb_queue_head_init(&head);
        spin_lock_bh(&tn->nametbl_lock);
-       if (!(capabilities & TIPC_NAMED_BCAST))
-               nt->rc_dests++;
        seqno = nt->snd_nxt;
        spin_unlock_bh(&tn->nametbl_lock);

        read_lock_bh(&nt->cluster_scope_lock);
-       named_distribute(net, &head, dnode, &nt->cluster_scope, seqno);
+       /* 1. tipc_net_finalize_work() is not scheduled because of namespace
+        *    teardown.
+        * 2. Or tipc_net_finalize() ---> tipc_nametbl_publish() has failed
+        *    to insert node self address publication to nt->cluster_scope.
+        * 3. Or tipc_net_finalize() ---> tipc_nametbl_publish() has not
+        *    executed yet.
+        */
+       if (unlikely(list_empty(&nt->cluster_scope))) {
+               read_unlock_bh(&nt->cluster_scope_lock);
+               return 1;
+       }
+
+       if (named_distribute(net, &head, dnode, &nt->cluster_scope, seqno)) {
+               read_unlock_bh(&nt->cluster_scope_lock);
+               return -ENOBUFS;
+       }
        tipc_node_xmit(net, &head, dnode, 0);
        read_unlock_bh(&nt->cluster_scope_lock);
+
+       return 0;
+}
+
+/**
+ * tipc_named_node_up - tell specified node about all publications by this node
+ * @net: the associated network namespace
+ * @dnode: destination node
+ * @capabilities: peer node's capabilities
+ */
+int tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities)
+{
+       struct name_table *nt = tipc_name_table(net);
+       struct tipc_net *tn = tipc_net(net);
+
+       spin_lock_bh(&tn->nametbl_lock);
+       if (!(capabilities & TIPC_NAMED_BCAST))
+               nt->rc_dests++;
+       spin_unlock_bh(&tn->nametbl_lock);
+
+       return tipc_named_distribute(net, dnode);
+}
+
+/**
+ * tipc_named_dist_cluster_scope - distribute all publications to specified node
+ * @net: the associated network namespace
+ * @dnode: destination node
+ */
+int tipc_named_dist_cluster_scope(struct net *net, u32 dnode)
+{
+       struct tipc_net *tn = tipc_net(net);
+
+       wait_var_event(&tn->finalized, atomic_read(&tn->finalized));
+       return tipc_named_distribute(net, dnode);
 }

 /**
diff --git a/net/tipc/name_distr.h b/net/tipc/name_distr.h
index c677f6f082df..cadf4e8c3e66 100644
--- a/net/tipc/name_distr.h
+++ b/net/tipc/name_distr.h
@@ -69,7 +69,8 @@ struct distr_item {

 struct sk_buff *tipc_named_publish(struct net *net, struct publication *publ);
 struct sk_buff *tipc_named_withdraw(struct net *net, struct publication *publ);
-void tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities);
+int tipc_named_node_up(struct net *net, u32 dnode, u16 capabilities);
+int tipc_named_dist_cluster_scope(struct net *net, u32 dnode);
 void tipc_named_rcv(struct net *net, struct sk_buff_head *namedq,
                    u16 *rcv_nxt, bool *open);
 void tipc_named_reinit(struct net *net);
diff --git a/net/tipc/net.c b/net/tipc/net.c
index 7e65d0b0c4a8..78418515277e 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -132,13 +132,26 @@ static void tipc_net_finalize(struct net *net, u32 addr)
        tipc_uaddr(&ua, TIPC_SERVICE_RANGE, TIPC_CLUSTER_SCOPE,
                   TIPC_NODE_STATE, addr, addr);

+       if (atomic_read(&tn->work_rescheduled))
+               goto publish;
+
        if (cmpxchg(&tn->node_addr, 0, addr))
                return;
        tipc_set_node_addr(net, addr);
        tipc_named_reinit(net);
        tipc_sk_reinit(net);
        tipc_mon_reinit_self(net);
-       tipc_nametbl_publish(net, &ua, &sk, addr);
+
+publish:
+       if (!tipc_nametbl_publish(net, &ua, &sk, addr)) {
+               tn->trial_addr = addr;
+               atomic_set(&tn->work_rescheduled, 1);
+               schedule_work(&tn->work);
+               return;
+       }
+       atomic_set(&tn->work_rescheduled, 0);
+       atomic_set(&tn->finalized, 1);
+       wake_up_var(&tn->finalized);
 }

 void tipc_net_finalize_work(struct work_struct *work)
diff --git a/net/tipc/node.c b/net/tipc/node.c
index 8e4ef2630ae4..afed72894722 100644
--- a/net/tipc/node.c
+++ b/net/tipc/node.c
@@ -111,6 +111,8 @@ struct tipc_bclink_entry {
  * @peer_net: peer's net namespace
  * @peer_hash_mix: hash for this peer (FIXME)
  * @crypto_rx: RX crypto handler
+ * @work: work item for bulk distribution of cluster scope publications
+ * @work_scheduled: flag to indicate the work has been scheduled
  */
 struct tipc_node {
        u32 addr;
@@ -145,6 +147,8 @@ struct tipc_node {
 #ifdef CONFIG_TIPC_CRYPTO
        struct tipc_crypto *crypto_rx;
 #endif
+       struct work_struct work;
+       atomic_t work_scheduled;
 };

 /* Node FSM states and events:
@@ -393,6 +397,27 @@ static void tipc_node_write_unlock_fast(struct tipc_node *n)
        write_unlock_bh(&n->lock);
 }

+static void tipc_node_down(struct tipc_node *n)
+{
+       u32 bearer_id, bearer_cnt;
+
+       tipc_node_read_lock(n);
+       bearer_cnt = n->link_cnt;
+       tipc_node_read_unlock(n);
+       for (bearer_id = 0; bearer_id < bearer_cnt; bearer_id++)
+               tipc_node_link_down(n, bearer_id, false);
+}
+
+static void tipc_node_dist_bulk(struct work_struct *work)
+{
+       struct tipc_node *node = container_of(work, struct tipc_node, work);
+
+       if (tipc_named_dist_cluster_scope(node->net, node->addr) < 0)
+               tipc_node_down(node);
+
+       tipc_node_put(node);
+}
+
 static void tipc_node_write_unlock(struct tipc_node *n)
        __releases(n->lock)
 {
@@ -424,8 +449,23 @@ static void tipc_node_write_unlock(struct tipc_node *n)
        if (flags & TIPC_NOTIFY_NODE_DOWN)
                tipc_publ_notify(net, publ_list, node, n->capabilities);

-       if (flags & TIPC_NOTIFY_NODE_UP)
-               tipc_named_node_up(net, node, n->capabilities);
+       if (flags & TIPC_NOTIFY_NODE_UP) {
+               int rc = 0;
+
+               rc = tipc_named_node_up(net, node, n->capabilities);
+               /* Defer bulk distribution to work queue */
+               if (rc > 0) {
+                       atomic_set(&n->work_scheduled, 1);
+                       tipc_node_get(n);
+                       if (!schedule_work(&n->work))
+                               tipc_node_put(n);
+               } else if (rc < 0) {
+                       /* Bring the node down to start over bulk distribution
+                        * when the first link is up again.
+                        */
+                       tipc_node_down(n);
+               }
+       }

        if (flags & TIPC_NOTIFY_LINK_UP) {
                tipc_mon_peer_up(net, node, bearer_id);
@@ -564,6 +604,8 @@ struct tipc_node *tipc_node_create(struct net *net, u32 addr, u8 *peer_id,
        INIT_LIST_HEAD(&n->list);
        INIT_LIST_HEAD(&n->publ_list);
        INIT_LIST_HEAD(&n->conn_sks);
+       INIT_WORK(&n->work, tipc_node_dist_bulk);
+       atomic_set(&n->work_scheduled, 0);
        skb_queue_head_init(&n->bc_entry.namedq);
        skb_queue_head_init(&n->bc_entry.inputq1);
        __skb_queue_head_init(&n->bc_entry.arrvq);
@@ -635,10 +677,25 @@ static void tipc_node_delete_from_list(struct tipc_node *node)

 static void tipc_node_delete(struct tipc_node *node)
 {
+       struct tipc_net *tn = tipc_net(node->net);
+
        trace_tipc_node_delete(node, true, " ");
        tipc_node_delete_from_list(node);

        timer_delete_sync(&node->timer);
+
+       /* Wake up node work queue if tipc_net_finalize_work() is not
+        * scheduled yet.
+        */
+       if (atomic_read(&node->work_scheduled)) {
+               if (!atomic_read(&tn->finalized)) {
+                       atomic_set(&tn->finalized, 1);
+                       wake_up_var(&tn->finalized);
+               }
+
+               cancel_work_sync(&node->work);
+       }
+
        tipc_node_put(node);
 }

-- 
2.43.0