RE: [PATCH net v2] tipc: fix out-of-bounds read in broadcast Gap ACK blocks

[Tung Quang Nguyen]

>Subject: [PATCH net v2] tipc: fix out-of-bounds read in broadcast Gap ACK
>blocks
>
>A broadcast PROTOCOL/STATE_MSG can carry a Gap ACK blocks record in its
>data area. tipc_get_gap_ack_blks() only verifies that the record's len field is
>self-consistent with its ugack_cnt/bgack_cnt counts (sz == struct_size(p, gacks,
>ugack_cnt + bgack_cnt)); it does not check that the record actually fits in the
>message data area, msg_data_sz().
>
>The unicast caller tipc_link_proto_rcv() bounds it ("if (glen > dlen) break;"), but
>the broadcast caller tipc_bcast_sync_rcv() discards the returned size, so
>tipc_link_advance_transmq() copies the record off the receive skb with an
>attacker-controlled count:
>
>	this_ga = kmemdup(ga, struct_size(ga, gacks, ga->bgack_cnt),
>			  GFP_ATOMIC);
>
>A TIPC neighbour that negotiated TIPC_GAP_ACK_BLOCK triggers it with one
>ordinary broadcast STATE_MSG (msg_bc_ack_invalid() clear), sized so its data
>area is short, carrying a Gap ACK record with len = 0x400, bgack_cnt = 0xff and
>ugack_cnt = 0. len then equals struct_size(p, gacks, 255), so the consistency
>check passes and ga is non-NULL; kmemdup() reads struct_size(ga, gacks, 255)
>= 1024 bytes out of the much smaller skb:
>
>  BUG: KASAN: slab-out-of-bounds in kmemdup_noprof+0x48/0x60
>  Read of size 1024 at addr ffff0000c7030d38 by task poc864/69
>  Call trace:
>   kmemdup_noprof+0x48/0x60
>   tipc_link_advance_transmq+0x86c/0xb80
>   tipc_link_bc_ack_rcv+0x19c/0x1e0
>   tipc_bcast_sync_rcv+0x1c4/0x2c4
>   tipc_rcv+0x85c/0x1340
>   tipc_l2_rcv_msg+0xac/0x104
>  The buggy address belongs to the object at ffff0000c7030d00
>   which belongs to the cache skbuff_small_head of size 704
>  The buggy address is located 56 bytes inside of
>   allocated 704-byte region [ffff0000c7030d00, ffff0000c7030fc0)
>
>The copied-out bytes are subsequently consumed as gap/ack values, but the
>read is already out of bounds at the kmemdup() regardless of how they are
>used.
>
>The unicast STATE path drops such a message: "if (glen > dlen) break;"
>skips the rest of STATE_MSG handling and the skb is freed. Make the broadcast
>path drop it too. tipc_bcast_sync_rcv() now bounds the record against
>msg_data_sz() and, when it does not fit, reports it back through
>tipc_node_bc_sync_rcv() to tipc_rcv() so the skb is discarded rather than
>processed. ga is not cleared on this path: ga == NULL already means "legacy
>peer without Selective ACK", a distinct legitimate state.
>
>Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
>Cc: stable@xxxxxxxxxxxxxxx
>Assisted-by: Bynario AI
>Signed-off-by: Samuel Page <sam@xxxxxxxx>
>---
>v2, per review of v1 [1]:
> - v1 cleared 'ga' on an oversized Gap ACK record, which let the malformed
>   STATE message be processed as a legacy (no Selective ACK) one rather than
>   dropped.  v2 drops it instead, matching the unicast STATE path:
>   tipc_bcast_sync_rcv() reports the bad record through a bool output
>   parameter, propagated by tipc_node_bc_sync_rcv() to tipc_rcv(), which
>   discards the skb.
> - v1 touched only net/tipc/bcast.c; v2 also touches net/tipc/{bcast.h,node.c}.
>
>[1] https://lore.kernel.org/netdev/20260623134137.3641275-1-sam@xxxxxxxx/
>
>For reference, an earlier thread proposed validating inside
>tipc_get_gap_ack_blks():
>
>https://lore.kernel.org/netdev/1316452e465e9a96fce44ec15130a14f3872149f.
>1775809727.git.caoruide123@xxxxxxxxx/
>
> net/tipc/bcast.c | 22 ++++++++++++++--------  net/tipc/bcast.h |  2 +-
>net/tipc/node.c  | 13 ++++++++++---
> 3 files changed, 25 insertions(+), 12 deletions(-)
>
>diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
>76a1585d3f6b..08637c3c9db0 100644
>--- a/net/tipc/bcast.c
>+++ b/net/tipc/bcast.c
>@@ -497,11 +497,12 @@ void tipc_bcast_ack_rcv(struct net *net, struct
>tipc_link *l,
>  */
> int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
> 			struct tipc_msg *hdr,
>-			struct sk_buff_head *retrq)
>+			struct sk_buff_head *retrq, bool *valid)
> {
> 	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> 	struct tipc_gap_ack_blks *ga;
> 	struct sk_buff_head xmitq;
>+	u16 glen;

Move this variable declaration to the bottom to follow reverse xmas tree style.

> 	int rc = 0;
>
> 	__skb_queue_head_init(&xmitq);
>@@ -510,13 +511,18 @@ int tipc_bcast_sync_rcv(struct net *net, struct
>tipc_link *l,
> 	if (msg_type(hdr) != STATE_MSG) {
> 		tipc_link_bc_init_rcv(l, hdr);
> 	} else if (!msg_bc_ack_invalid(hdr)) {
>-		tipc_get_gap_ack_blks(&ga, l, hdr, false);
>-		if (!sysctl_tipc_bc_retruni)
>-			retrq = &xmitq;
>-		rc = tipc_link_bc_ack_rcv(l, msg_bcast_ack(hdr),
>-					  msg_bc_gap(hdr), ga, &xmitq,
>-					  retrq);
>-		rc |= tipc_link_bc_sync_rcv(l, hdr, &xmitq);
>+		glen = tipc_get_gap_ack_blks(&ga, l, hdr, false);
>+		if (glen > msg_data_sz(hdr)) {
>+			/* Malformed Gap ACK blocks; caller drops the msg */
>+			*valid = false;
>+		} else {
>+			if (!sysctl_tipc_bc_retruni)
>+				retrq = &xmitq;
>+			rc = tipc_link_bc_ack_rcv(l, msg_bcast_ack(hdr),
>+						  msg_bc_gap(hdr), ga, &xmitq,
>+						  retrq);
>+			rc |= tipc_link_bc_sync_rcv(l, hdr, &xmitq);
>+		}
> 	}
> 	tipc_bcast_unlock(net);
>
>diff --git a/net/tipc/bcast.h b/net/tipc/bcast.h index
>2d9352dc7b0e..55d17b5413e1 100644
>--- a/net/tipc/bcast.h
>+++ b/net/tipc/bcast.h
>@@ -97,7 +97,7 @@ void tipc_bcast_ack_rcv(struct net *net, struct tipc_link *l,
> 			struct tipc_msg *hdr);
> int tipc_bcast_sync_rcv(struct net *net, struct tipc_link *l,
> 			struct tipc_msg *hdr,
>-			struct sk_buff_head *retrq);
>+			struct sk_buff_head *retrq, bool *valid);
> int tipc_nl_add_bc_link(struct net *net, struct tipc_nl_msg *msg,
> 			struct tipc_link *bcl);
> int tipc_nl_bc_link_set(struct net *net, struct nlattr *attrs[]); diff --git
>a/net/tipc/node.c b/net/tipc/node.c index 97aa970a0d83..2887f94ee28f
>100644
>--- a/net/tipc/node.c
>+++ b/net/tipc/node.c
>@@ -1831,12 +1831,13 @@ static void tipc_node_mcast_rcv(struct tipc_node
>*n)  }
>
> static void tipc_node_bc_sync_rcv(struct tipc_node *n, struct tipc_msg *hdr,
>-				  int bearer_id, struct sk_buff_head *xmitq)
>+				  int bearer_id, struct sk_buff_head *xmitq,
>+				  bool *valid)
> {
> 	struct tipc_link *ucl;
> 	int rc;
>
>-	rc = tipc_bcast_sync_rcv(n->net, n->bc_entry.link, hdr, xmitq);
>+	rc = tipc_bcast_sync_rcv(n->net, n->bc_entry.link, hdr, xmitq, valid);

'valid' needs to be checked after this call. Then, return immediately if it is false.

>
> 	if (rc & TIPC_LINK_DOWN_EVT) {
> 		tipc_node_reset_links(n);
>@@ -2140,12 +2141,18 @@ void tipc_rcv(struct net *net, struct sk_buff *skb,
>struct tipc_bearer *b)
>
> 	/* Ensure broadcast reception is in synch with peer's send state */
> 	if (unlikely(usr == LINK_PROTOCOL)) {
>+		bool valid = true;
>+
> 		if (unlikely(skb_linearize(skb))) {
> 			tipc_node_put(n);
> 			goto discard;
> 		}
> 		hdr = buf_msg(skb);
>-		tipc_node_bc_sync_rcv(n, hdr, bearer_id, &xmitq);
>+		tipc_node_bc_sync_rcv(n, hdr, bearer_id, &xmitq, &valid);
>+		if (!valid) {
>+			tipc_node_put(n);
>+			goto discard;
>+		}
> 	} else if (unlikely(tipc_link_acked(n->bc_entry.link) != bc_ack)) {
> 		tipc_bcast_ack_rcv(net, n->bc_entry.link, hdr);
> 	}
>
>base-commit: a986fde914d88af47eb78fd29c5d1af7952c3500
>--
>2.54.0