[PATCH net] tipc: don't count unjoined publications as receivers

[PATCH net] tipc: don't count unjoined publications as receivers

[shaos]

Hello,

A local process with no capabilities can block sends from a TIPC
communication group by publishing the matching service range from a
socket that never joins the group. I reproduced it with a root sender
and a separate unprivileged group member on the same node. A third,
unprivileged process binds the same type and instance as an ordinary
TIPC service. The sender's first nonblocking send returns -EAGAIN. Its
subsequent blocking send remains asleep; after 12 seconds,
poll(POLLOUT) is still clear.

The publisher has CapEff=0 and NoNewPrivs=1. It does not need
CAP_NET_ADMIN or CAP_NET_RAW. The guest init enables the TIPC bearer
before dropping privileges. The test uses one local node; I have not
tested a remote-node trigger. This demonstrates a local availability
issue for the group service only. There is no memory corruption,
information disclosure, or privilege escalation.

tipc_nametbl_build_group() and TIPC_PUBLISHED events create
MBR_PUBLISHED entries for matching service publications.
tipc_group_is_receiver() currently accepts this state. Its
zero-initialized window is added to small_win, so
tipc_group_bc_cong() blocks a sender indefinitely when the ordinary
socket does not complete the group JOIN handshake. A real group
member is present in the reproducer, so this is not an empty-group
send.

This is distinct from the group broadcast hang caused by deleting
the last unacked member: this trigger needs no ACK loss or member
departure and occurs before the first broadcast.

HEAD: commit c03c1a14507dfaf8b856c2608f9ac216021fc222 ("Linux 7.3")
git tree:       upstream Linux, local branch master
source version: Linux 7.3.0 (v7.3)
compiler:       gcc 11.4.0, GNU ld 2.38
QEMU:           6.2.0

Raw reproduction files:

console-output.txt
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/console-output.txt
patched-console-output.txt
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/patched-console-output.txt
kernel-config.txt
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/kernel-config.txt
poc.c
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/poc.c
init
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/init
build-initramfs.sh
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/build-initramfs.sh
run-qemu.sh
https://gist.githubusercontent.com/shaos26/243e46a190cef51fbe6a1e8cf1bcdede/raw/1cb4d487be1eace60d6d68ab384f85057b296b41/run-qemu.sh

With these files saved in a Linux tree at the HEAD above:

    cp kernel-config.txt .config
    make olddefconfig
    make -j"$(nproc)" bzImage
    ./build-initramfs.sh
    ./run-qemu.sh arch/x86/boot/bzImage phantom-group

The unmodified kernel reproduced:

    POLL_BEFORE_FIRST_SEND=4
    NONBLOCK_FIRST_SEND ret=-1 errno=11
    POC_RESULT POSITIVE_SEND_BLOCKED observe_ms=12000 pollout=0

The patched kernel was built from the same HEAD and config. The same
test reported:

    POLL_BEFORE_FIRST_SEND=4
    NONBLOCK_FIRST_SEND ret=1 errno=0
    POC_RESULT CONTROL_SEND_RETURNED ret=1 errno=0

The patched run also preserves POLLOUT for a legitimate group member.
The patch passed checkpatch.pl --no-tree --strict and git apply --check;
the patched kernel and static PoC both built successfully.

Exclude MBR_PUBLISHED entries from receiver accounting. When the JOIN
handshake changes a member to MBR_JOINED, open the group even if that
member was not on the small-window list.

Fixes: b7d42635517f ("tipc: introduce flow control for group broadcast messages")
Signed-off-by: shaos <shawnq1@163.com>
---
diff --git a/net/tipc/group.c b/net/tipc/group.c
index 74f6d3dac0784..61fc57d96f365 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -105,9 +105,12 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
 static void tipc_group_open(struct tipc_member *m, bool *wakeup)
 {
 	*wakeup = false;
-	if (list_empty(&m->small_win))
-		return;
-	list_del_init(&m->small_win);
+	if (list_empty(&m->small_win)) {
+		if (m->state != MBR_JOINED || *m->group->open)
+			return;
+	} else {
+		list_del_init(&m->small_win);
+	}
 	*m->group->open = true;
 	*wakeup = true;
 }
@@ -145,7 +148,8 @@ u16 tipc_group_bc_snd_nxt(struct tipc_group *grp)
 
 static bool tipc_group_is_receiver(struct tipc_member *m)
 {
-	return m && m->state != MBR_JOINING && m->state != MBR_LEAVING;
+	return m && m->state != MBR_JOINING && m->state != MBR_PUBLISHED &&
+	       m->state != MBR_LEAVING;
 }
 
 static bool tipc_group_is_sender(struct tipc_member *m)

Re: [PATCH net] tipc: don't count unjoined publications as receivers

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] tipc: don't count unjoined publications as receivers

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch takes MBR_PUBLISHED out of tipc_group_is_receiver(),
  which removes the gate that held a new joiner's sends until each…
- [High] Changing tipc_group_is_receiver() also changes what
  tipc_group_find_dest() and tipc_group_cong() return, which affects…
- [Low] The new branch in tipc_group_open() opens the group whenever the
  member is MBR_JOINED, is not on small_win, and the group is closed.
- [Low] The changelog reads like a cover letter or bug report and is not
  a commit log.
- [Low] tipc_group_create_member() still adds every member's node to
  grp->dests regardless of state (tipc_nlist_add(&grp->dests, m->node),…

--

tipc: don't count unjoined publications as receivers

This commit removes MBR_PUBLISHED members from tipc_group_is_receiver(),
so publications from sockets that never join the group no longer hold
group senders on the small window list. It also lets tipc_group_open()
reopen the group when a member reaches MBR_JOINED, even if that member
was not on small_win.

> Hello,
>
> A local process with no capabilities can block sends from a TIPC

[ ... ]

> HEAD: commit c03c1a14507dfaf8b856c2608f9ac216021fc222 ("Linux 7.3")
> git tree:       upstream Linux, local branch master
> source version: Linux 7.3.0 (v7.3)
> compiler:       gcc 11.4.0, GNU ld 2.38
> QEMU:           6.2.0
>
> Raw reproduction files:

[ ... ]

[Severity: Low]
This isn't a bug, but the changelog reads like a cover letter or a bug
report. The greeting, gist URLs, toolchain and QEMU versions, local
branch name, HEAD reference and build/run instructions would all end up
in git history. Could they move below the --- line?

The changelog also leaves out several behaviour changes in the patch:
senders are no longer gated on the JOIN handshake, unicast to a member
still in the handshake now returns -EHOSTUNREACH, and anycast behaves
differently.

> diff --git a/net/tipc/group.c b/net/tipc/group.c
> index 0c6a762000b02..c55daebc6cb38 100644
> --- a/net/tipc/group.c
> +++ b/net/tipc/group.c
> @@ -105,9 +105,12 @@ static void tipc_group_proto_xmit(struct tipc_group *grp, struct tipc_member *m,
>  static void tipc_group_open(struct tipc_member *m, bool *wakeup)
>  {
>  	*wakeup = false;
> -	if (list_empty(&m->small_win))
> -		return;
> -	list_del_init(&m->small_win);
> +	if (list_empty(&m->small_win)) {
> +		if (m->state != MBR_JOINED || *m->group->open)
> +			return;
> +	} else {
> +		list_del_init(&m->small_win);
> +	}

[Severity: Low]
The new branch is aimed at the JOIN handshake transitions. However,
tipc_group_open() is also called from the GRP_ADV_MSG, GRP_RECLAIM_MSG
and GRP_LEAVE_MSG cases in tipc_group_proto_rcv(), for example:

	case GRP_ADV_MSG:
		if (!m)
			return;
		m->window += msg_adv_win(hdr);
		tipc_group_open(m, usr_wakeup);
		return;

The group may be closed for an unrelated reason, such as pending
replicast acks (grp->bc_ackers != 0) or congestion on another member.
In that case, won't an ADV or RECLAIM from any JOINED member that is not
on small_win now set *grp->open = true and *usr_wakeup = true?

tipc_poll() reports EPOLLOUT based only on tsk->group_is_open. That
would give a spurious POLLOUT followed by -EAGAIN on a nonblocking
send, and spurious wakeups of blocked senders. The existing small_win
branch already reopens without checking those conditions, but this
change extends that to members that were never congested.

The two departure paths also behave differently now. GRP_LEAVE_MSG
calls tipc_group_open() while the member is still MBR_JOINED:

		tipc_group_open(m, usr_wakeup);
		tipc_group_decr_active(grp, m);
		m->state = MBR_LEAVING;

so the new branch can fire there. The TIPC_WITHDRAWN path in
tipc_group_member_evt() sets MBR_LEAVING before calling
tipc_group_open(), so it cannot.

Should the new open be limited to the actual transition into
MBR_JOINED?

>  	*m->group->open = true;
>  	*wakeup = true;
>  }
> @@ -145,7 +148,8 @@ u16 tipc_group_bc_snd_nxt(struct tipc_group *grp)
>  
>  static bool tipc_group_is_receiver(struct tipc_member *m)
>  {
> -	return m && m->state != MBR_JOINING && m->state != MBR_LEAVING;
> +	return m && m->state != MBR_JOINING && m->state != MBR_PUBLISHED &&
> +	       m->state != MBR_LEAVING;

[Severity: High]
Does excluding MBR_PUBLISHED here also remove the gate that stopped a
new joiner from sending before each existing member had processed its
JOIN?

Before this patch, tipc_group_join() did:

	rbtree_postorder_for_each_entry_safe(m, tmp, tree, tree_node) {
		tipc_group_proto_xmit(grp, m, GRP_JOIN_MSG, &xmitq);
		tipc_group_update_member(m, 0);
	}

This put every PUBLISHED member (window 0) on grp->small_win, so
tipc_group_bc_cong() blocked the sender until the peer's JOIN arrived.

With this change, tipc_group_update_member() returns early for those
members and small_win stays empty. tipc_group_bc_cong() now returns
false right after setsockopt(TIPC_GROUP_JOIN).

Take socket A joining while member B already exists. A's GRP_JOIN_MSG
reaches B synchronously on the local node. tipc_group_proto_rcv() then
creates A as MBR_JOINING with bc_rcv_nxt set to the syncpoint. A's
TIPC_PUBLISHED topology event reaches B later, through the topsrv
workqueue:

tipc_topsrv_queue_evt()->queue_work()->tipc_topsrv_kern_evt()

If A broadcasts before that worker runs, B sees this in
tipc_group_filter_msg():

	m = tipc_group_find_member(grp, node, port);
	if (!tipc_group_is_sender(m))
		goto drop;

B drops the message without advancing bc_rcv_nxt. When the PUBLISHED
event arrives, tipc_group_member_evt() moves A to MBR_JOINED but leaves
bc_rcv_nxt unchanged.

Every later message from A then hits this check. That includes
unicasts, because tipc_send_group_msg() stamps bc_snd_nxt via
msg_set_grp_bc_seqno():

		if (more(msg_grp_bc_seqno(hdr), m->bc_rcv_nxt))
			break;

The message stays in m->deferredq. B never reads these messages, so it
never sends GRP_ADV_MSG, and A's window for B eventually runs out.

Can this leave A's broadcasts and unicasts to B blocked permanently?
That is the symptom this patch sets out to fix, but here it is
reachable by an ordinary join followed by an immediate send.

There is also a side effect: tipc_group_update_bc_members() now skips
members that A still sees as PUBLISHED. No window is charged and no ack
is expected for them, even though the broadcast reaches their node
through grp->dests.

[Severity: High]
tipc_group_find_dest() and tipc_group_cong() both use
tipc_group_is_receiver(). Doesn't this change unicast and anycast
behaviour as well?

For an MBR_PUBLISHED member, tipc_group_cong() now takes this path:

	m = tipc_group_find_dest(grp, dnode, dport);
	if (!tipc_group_is_receiver(m)) {
		*mbr = NULL;
		return false;
	}

Previously it returned true with *mbr set.

For unicast, tipc_send_group_unicast() to a member still in the JOIN
handshake now fails immediately instead of waiting for the JOIN:

	if (unlikely(!mb))
		return -EHOSTUNREACH;

For anycast, tipc_send_group_anycast() was written for the old
behaviour. The inner loop breaks on the first cong == false, and then:

		/* Start over if destination was not in member list */
		if (unlikely(!mbr))
			continue;

This uses up one of only three outer lookups (while (++lookups < 4)),
and after the third one the send fails:

	if (unlikely(lookups >= 4))
		return -EHOSTUNREACH;

tipc_nametbl_lookup_group() hands out candidates round-robin from
sr->all_publ. It filters only by scope and the excluded port, not by
group membership.

Could an unprivileged process bind three or more ordinary (non-group)
sockets to the group's type/instance and make group anycast fail with
-EHOSTUNREACH, even when a joined, uncongested member is reachable?

Before the patch, these phantom entries reported cong == true with mbr
set, and the inner loop moved past them to a real member. So the
problem described in the changelog seems to remain reachable through
anycast, with the symptom changing from blocking to -EHOSTUNREACH.
tipc_send_group_mcast() also still counts phantom publications in
dstcnt.

Would non-member publications need to be filtered out, rather than just
classified differently?

[Severity: Low]
tipc_group_create_member() still adds every member's node to grp->dests,
whatever its state:

	tipc_nlist_add(&grp->dests, m->node);

MBR_PUBLISHED members, though, are no longer receivers. Suppose the
only peers matching the group are unjoined publications.
tipc_send_group_bcast() then passes this check:

	if (!dsts->local && !dsts->remote)
		return -EHOSTUNREACH;

tipc_group_bc_cong() returns false because small_win is empty. The
broadcast goes out with zero accounted receivers, and the non-group
socket drops it in tipc_sk_filter_rcv().

Meanwhile tsk->group_is_open starts out false and only becomes true
through a JOIN or a GRP_ACK, so tipc_poll() never reports EPOLLOUT:

		if (tsk->group_is_open && !tsk->cong_link_cnt)
			revents |= EPOLLOUT;

Is it intended that sendmsg() succeeds while poll() says the socket is
not writable? Also, should a send to a group with no joined members
return success rather than -EHOSTUNREACH?

>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107082422.900347-1-shawnq1%40163.com