tipc: validate Gap ACK blocks in STATE message

PATCH In review Ren Wei 3 participants 4 messages Apr 10 – Apr 13 1 message not in the archive

Subsystems (3)

Networking Network drivers TIPC network layer

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author

2026-04-10 15:53 UTC #1 lore raw

PATCH 1/1 tipc: validate Gap ACK blocks in STATE message

From: Ruide Cao <[email protected]>

tipc_get_gap_ack_blks() reads len, ugack_cnt and bgack_cnt directly from msg_data(hdr) before verifying that a STATE message actually contains the fixed Gap ACK block header in its logical data area.

A peer that negotiates TIPC_GAP_ACK_BLOCK can send a short STATE message with a declared TIPC payload shorter than struct tipc_gap_ack_blks and still append a few physical bytes after the header. The helper then trusts those bytes as Gap ACK metadata, and the forged bgack_cnt/len values can drive the broadcast receive path into kmemdup() beyond the skb boundary.

Fix this by rejecting Gap ACK parsing unless the logical STATE payload is large enough to cover the fixed header, and by rejecting declared Gap ACK lengths that are smaller than the fixed header or larger than the logical payload. Return 0 for invalid lengths so malformed Gap ACK data is not treated as a valid payload offset, and drop unicast STATE messages that advertise Gap ACK support but still yield an invalid Gap ACK length. This keeps malformed Gap ACK data ignored without misaligning monitor payload parsing.

Fixes d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")

Cc [email protected]

Reported-by Yifan Wu <[email protected]>

Reported-by Juefei Pu <[email protected]>

Co-developed-by Yuan Tan <[email protected]>

Signed-off-by Yuan Tan <[email protected]>

Suggested-by Xin Liu <[email protected]>

Tested-by Ren Wei <[email protected]>

Signed-off-by Ruide Cao <[email protected]>

Signed-off-by Ren Wei <[email protected]>

---
 net/tipc/link.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

net/tipc/link.c +14 -2

index 49dfc098d89b..44678d98939a 100644
@@ -1415,12 +1415,22 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
 			  struct tipc_msg *hdr, bool uc)
 {
 	struct tipc_gap_ack_blks *p;
-	u16 sz = 0;
+	u16 sz = 0, dlen = msg_data_sz(hdr);
 
 	/* Does peer support the Gap ACK blocks feature? */
 	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+		u16 min_sz = struct_size(p, gacks, 0);
+
+		if (dlen < min_sz)
+			goto ignore;
+
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
+		if (sz < min_sz || sz > dlen) {
+			sz = 0;
+			goto ignore;
+		}
+
 		/* Sanity check */
 		if (sz == struct_size(p, gacks, size_add(p->ugack_cnt, p->bgack_cnt))) {
 			/* Good, check if the desired type exists */

@@ -1434,6 +1444,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
 			}
 		}
 	}
+
+ignore:
 	/* Other cases: ignore! */
 	p = NULL;

@@ -2270,7 +2282,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct sk_buff *skb,
 	case STATE_MSG:
 		/* Validate Gap ACK blocks, drop if invalid */
 		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
-		if (glen > dlen)
+		if (glen > dlen || ((l->peer_caps & TIPC_GAP_ACK_BLOCK) && !glen))
 			break;
 
 		l->rcv_nxt_state = msg_seqno(hdr) + 1;

Signature · 1 line

2.34.1

Tung Quang Nguyen

maintainer

2026-04-13 03:06 UTC #2 lore raw

Quoted 8 lines

Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
From: Ruide Cao <[email protected]>
tipc_get_gap_ack_blks() reads len, ugack_cnt and bgack_cnt directly from msg_data(hdr) before verifying that a STATE message actually contains the fixed Gap ACK block header in its logical data area.

Quoted 5 lines

A peer that negotiates TIPC_GAP_ACK_BLOCK can send a short STATE message with a declared TIPC payload shorter than struct tipc_gap_ack_blks and still append a few physical bytes after the header. The helper then trusts those bytes as Gap ACK metadata, and the forged bgack_cnt/len values can drive the broadcast receive path into kmemdup() beyond the skb boundary.

Can you explain how that peer can alter the STATE message ? If it can, what concrete values are used and on what fields of the STATE messages ?

Quoted 31 lines

Fix this by rejecting Gap ACK parsing unless the logical STATE payload is large enough to cover the fixed header, and by rejecting declared Gap ACK lengths that are smaller than the fixed header or larger than the logical payload. Return 0 for invalid lengths so malformed Gap ACK data is not treated as a valid payload offset, and drop unicast STATE messages that advertise Gap ACK support but still yield an invalid Gap ACK length. This keeps malformed Gap ACK data ignored without misaligning monitor payload parsing.
Fixes d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
Cc [email protected]
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
net/tipc/link.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
net/tipc/link.c index 49dfc098d89b..44678d98939a renamed from net/tipc/link.c +0 -0
100644
net/tipc/link.c +0 -0
@@ -1415,12 +1415,22 @@ u16 tipc_get_gap_ack_blks(struct
 tipc_gap_ack_blks **ga, struct tipc_link *l,
 			  struct tipc_msg *hdr, bool uc)
 {

net/tipc/link.c u16 tipc_get_gap_ack_blks(struct · line 1426

Quoted 10 lines

net/tipc/link.c +5 -1

 	struct tipc_gap_ack_blks *p;
-	u16 sz = 0;
+	u16 sz = 0, dlen = msg_data_sz(hdr);
 
 	/* Does peer support the Gap ACK blocks feature? */
 	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+		u16 min_sz = struct_size(p, gacks, 0);
+
+		if (dlen < min_sz)
+			goto ignore;

This checking is redundant because with existing sanity checking, the invalid gap ACK blocks will not be used to release acked messages in transmit queue.

net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1432

Quoted 7 lines

net/tipc/link.c +5 -0

+
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
+		if (sz < min_sz || sz > dlen) {
+			sz = 0;
+			goto ignore;
+		}

This checking is redundant. Existing sanity checking is good enough.

Quoted 16 lines

net/tipc/link.c +1 -0

+
 		/* Sanity check */
 		if (sz == struct_size(p, gacks, size_add(p->ugack_cnt, p-

Quoted 1 line

bgack_cnt))) {

			/* Good, check if the desired type exists */ @@ -
1434,6 +1444,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga,
struct tipc_link *l,
			}
		}
	}

net/tipc/link.c +2 -0

+
+ignore:

	/* Other cases: ignore! */
	p = NULL;

net/tipc/link.c static int tipc_link_proto_rcv(struct tipc_link *l, struct · line 2287

Quoted 7 lines

net/tipc/link.c +1 -1

@@ -2270,7 +2282,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct
 sk_buff *skb,
 	case STATE_MSG:
 		/* Validate Gap ACK blocks, drop if invalid */
 		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
-		if (glen > dlen)
+		if (glen > dlen || ((l->peer_caps & TIPC_GAP_ACK_BLOCK) &&
 !glen))

This checking is redundant. Existing sanity checking is good enough.

Quoted 6 lines

net/tipc/link.c +0 -0

 			break;

		l->rcv_nxt_state = msg_seqno(hdr) + 1;

--
2.34.1

Ruide Cao

2026-04-13 06:00 UTC #3 lore raw

Quoted 8 lines · Tung Quang Nguyen

Quoted 8 lines
Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
From: Ruide Cao <[email protected]>
tipc_get_gap_ack_blks() reads len, ugack_cnt and bgack_cnt directly from msg_data(hdr) before verifying that a STATE message actually contains the fixed Gap ACK block header in its logical data area.

Quoted 6 lines

Quoted 5 lines
A peer that negotiates TIPC_GAP_ACK_BLOCK can send a short STATE message with a declared TIPC payload shorter than struct tipc_gap_ack_blks and still append a few physical bytes after the header. The helper then trusts those bytes as Gap ACK metadata, and the forged bgack_cnt/len values can drive the broadcast receive path into kmemdup() beyond the skb boundary.
Can you explain how that peer can alter the STATE message ? If it can, what concrete values are used and on what fields of the STATE messages ?

Thanks for the review.

To clarify, the peer is not "altering" an already received STATE message; it is actively sending a malformed LINK_PROTOCOL/STATE_MSG after the link has already negotiated the TIPC_GAP_ACK_BLOCK capability.

Concretely, the crafted STATE message is sent with a modified msg_size so that msg_data_sz(hdr) is 0, but the actual UDP payload still carries extra physical bytes after the 40-byte TIPC header. Those bytes are then interpreted as the fixed Gap ACK header. For example:
len = 0x07fc
ugack_cnt = 0xff
bgack_cnt = 0xff

These values are specifically chosen so that the existing sanity check remains internally consistent:
struct_size(p, gacks, 0xff + 0xff) == 0x07fc

Therefore, the existing sanity check does not reject this case. It only checks the self-consistency of the attacker-controlled Gap ACK fields; it completely fails to check if the declared Gap ACK record actually fits inside the enclosing STATE message's logical payload length.

Quoted 34 lines

Quoted 34 lines
Fix this by rejecting Gap ACK parsing unless the logical STATE payload is large enough to cover the fixed header, and by rejecting declared Gap ACK lengths that are smaller than the fixed header or larger than the logical payload. Return 0 for invalid lengths so malformed Gap ACK data is not treated as a valid payload offset, and drop unicast STATE messages that advertise Gap ACK support but still yield an invalid Gap ACK length. This keeps malformed Gap ACK data ignored without misaligning monitor payload parsing.
Fixes d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
Cc [email protected]
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
net/tipc/link.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
net/tipc/link.c index 49dfc098d89b..44678d98939a renamed from net/tipc/link.c +0 -0
100644
net/tipc/link.c +1 -1
@@ -1415,12 +1415,22 @@ u16 tipc_get_gap_ack_blks(struct
 tipc_gap_ack_blks **ga, struct tipc_link *l,
 			  struct tipc_msg *hdr, bool uc)
 {
 	struct tipc_gap_ack_blks *p;
-	u16 sz = 0;
+	u16 sz = 0, dlen = msg_data_sz(hdr);
 

Quoted 7 lines

net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1425
Quoted 6 lines
net/tipc/link.c +4 -0
 	/* Does peer support the Gap ACK blocks feature? */
 	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+		u16 min_sz = struct_size(p, gacks, 0);
+
+		if (dlen < min_sz)
+			goto ignore;
This checking is redundant because with existing sanity checking, the invalid gap ACK blocks will not be used to release acked messages in transmit queue.

The dlen < min_sz check is required because the existing sanity check already dereferences p->len, p->ugack_cnt, and p->bgack_cnt. Without this new check, an Out-of-Bounds (OOB) read occurs before the old sanity check even has a chance to run.

Quoted 8 lines

net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1432
Quoted 7 lines
net/tipc/link.c +5 -0
+
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
+		if (sz < min_sz || sz > dlen) {
+			sz = 0;
+			goto ignore;
+		}
This checking is redundant. Existing sanity checking is good enough.

The sz < min_sz || sz > dlen check is not redundant because the old sanity check completely fails to verify if the declared Gap ACK length (sz) actually fits inside the enclosing STATE message's logical payload length (dlen).

Without checking against dlen, an internally consistent spoofed packet will pass the old check and cause OOB reads during the subsequent block parsing.

Quoted 15 lines

net/tipc/link.c +1 -0

+
 		/* Sanity check */
 		if (sz == struct_size(p, gacks, size_add(p->ugack_cnt, p-

Quoted 1 line

bgack_cnt))) {

			/* Good, check if the desired type exists */ @@ -
1434,6 +1444,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga,
struct tipc_link *l,
			}
		}
	}

net/tipc/link.c +2 -0

+
+ignore:

	/* Other cases: ignore! */
	p = NULL;

Quoted 9 lines

net/tipc/link.c static int tipc_link_proto_rcv(struct tipc_link *l, struct · line 2287
Quoted 8 lines
net/tipc/link.c +1 -1
@@ -2270,7 +2282,7 @@ static int tipc_link_proto_rcv(struct tipc_link *l, struct
 sk_buff *skb,
 	case STATE_MSG:
 		/* Validate Gap ACK blocks, drop if invalid */
 		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
-		if (glen > dlen)
+		if (glen > dlen || ((l->peer_caps & TIPC_GAP_ACK_BLOCK) &&
 !glen))
This checking is redundant. Existing sanity checking is good enough.

The unicast caller-side drop ((l->peer_caps & TIPC_GAP_ACK_BLOCK) && !glen) is also necessary. Once the capability is negotiated, a valid Gap ACK record MUST have at least the fixed 4-byte header. If glen == 0 from such a peer, it indicates a malformed payload.

The STATE message must be dropped here so it is not passed on to tipc_mon_rcv() as if monitor data started at data + 0, which would misalign the monitor payload parsing.

Quoted 6 lines

Quoted 6 lines
net/tipc/link.c +0 -0
 			break;
		l->rcv_nxt_state = msg_seqno(hdr) + 1;
--
2.34.1

Tung Quang Nguyen

maintainer

2026-04-13 10:01 UTC #4 lore raw

Quoted 29 lines

Subject: Re: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
Quoted 10 lines · Tung Quang Nguyen
Quoted 10 lines
Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
From: Ruide Cao <[email protected]>
tipc_get_gap_ack_blks() reads len, ugack_cnt and bgack_cnt directly from
msg_data(hdr) before verifying that a STATE message actually contains the fixed Gap ACK block header in its logical data area.
Quoted 5 lines
Quoted 5 lines
A peer that negotiates TIPC_GAP_ACK_BLOCK can send a short STATE message with a declared TIPC payload shorter than struct tipc_gap_ack_blks and still append a few physical bytes after the header. The helper then trusts those bytes as Gap ACK metadata, and the forged bgack_cnt/len values can drive the broadcast receive path into
kmemdup() beyond the skb boundary.
Quoted 1 line
Can you explain how that peer can alter the STATE message ? If it can, what
concrete values are used and on what fields of the STATE messages ?
Thanks for the review.
To clarify, the peer is not "altering" an already received STATE message; it is actively sending a malformed LINK_PROTOCOL/STATE_MSG after the link has already negotiated the TIPC_GAP_ACK_BLOCK capability.

Quoted 8 lines

Concretely, the crafted STATE message is sent with a modified msg_size so that msg_data_sz(hdr) is 0, but the actual UDP payload still carries extra physical bytes after the 40-byte TIPC header. Those bytes are then interpreted as the fixed Gap ACK header. For example:
len = 0x07fc
ugack_cnt = 0xff
bgack_cnt = 0xff

It is surprising that you can modify any field you want in the TIPC message. I do not think that current TIPC code can handle this corrupt message . Can you send me the stack trace at receiving peer when real crash happens after you send the "crafted" state message ?

Quoted 54 lines

These values are specifically chosen so that the existing sanity check remains internally consistent:
struct_size(p, gacks, 0xff + 0xff) == 0x07fc
Therefore, the existing sanity check does not reject this case. It only checks the self-consistency of the attacker-controlled Gap ACK fields; it completely fails to check if the declared Gap ACK record actually fits inside the enclosing STATE message's logical payload length.
Quoted 3 lines
Quoted 3 lines
Fix this by rejecting Gap ACK parsing unless the logical STATE payload is large enough to cover the fixed header, and by rejecting declared Gap ACK lengths that are smaller than the fixed header or larger
than the logical payload.
Quoted 4 lines
Quoted 4 lines
Return 0 for invalid lengths so malformed Gap ACK data is not treated as a valid payload offset, and drop unicast STATE messages that advertise Gap ACK support but still yield an invalid Gap ACK length. This keeps malformed Gap ACK data ignored without misaligning monitor
payload parsing.
Quoted 29 lines
Quoted 29 lines
Fixes d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast
link")
Cc [email protected]
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Co-developed-by Yuan Tan <[email protected]>
Signed-off-by Yuan Tan <[email protected]>
Suggested-by Xin Liu <[email protected]>
Tested-by Ren Wei <[email protected]>
Signed-off-by Ruide Cao <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
net/tipc/link.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
net/tipc/link.c index renamed from net/tipc/link.c +0 -0
49dfc098d89b..44678d98939a
100644
net/tipc/link.c +1 -1
@@ -1415,12 +1415,22 @@ u16 tipc_get_gap_ack_blks(struct
 tipc_gap_ack_blks **ga, struct tipc_link *l,
 			  struct tipc_msg *hdr, bool uc)
 {
 	struct tipc_gap_ack_blks *p;
-	u16 sz = 0;
+	u16 sz = 0, dlen = msg_data_sz(hdr);
 
Quoted 7 lines
net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1425
Quoted 6 lines
net/tipc/link.c +4 -0
 	/* Does peer support the Gap ACK blocks feature? */
 	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+		u16 min_sz = struct_size(p, gacks, 0);
+
+		if (dlen < min_sz)
+			goto ignore;
This checking is redundant because with existing sanity checking, the invalid

Quoted 4 lines

gap ACK blocks will not be used to release acked messages in transmit queue.
The dlen < min_sz check is required because the existing sanity check already dereferences p->len, p->ugack_cnt, and p->bgack_cnt.

In the case of dlen > p->len and p->len = 0x07fc, p->ugack_cnt = 0xff, p->bgack_cnt = 0xff (Sender modified or kept dlen and modified the remaining 3 fields), how could your above and subsequent sanity checks validate p->len, p->bgack_cnt and p->ugack_cnt ?

Quoted 60 lines

Without this new check, an Out-of-Bounds (OOB) read occurs before the old sanity check even has a chance to run.
Quoted 8 lines
net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1432
Quoted 7 lines
net/tipc/link.c +5 -0
+
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
+		if (sz < min_sz || sz > dlen) {
+			sz = 0;
+			goto ignore;
+		}
This checking is redundant. Existing sanity checking is good enough.
The sz < min_sz || sz > dlen check is not redundant because the old sanity check completely fails to verify if the declared Gap ACK length (sz) actually fits inside the enclosing STATE message's logical payload length (dlen).
Without checking against dlen, an internally consistent spoofed packet will pass the old check and cause OOB reads during the subsequent block parsing.
Quoted 15 lines
Quoted 15 lines
net/tipc/link.c +1 -0
+
 		/* Sanity check */
 		if (sz == struct_size(p, gacks, size_add(p->ugack_cnt, p-
Quoted 1 line
bgack_cnt))) {
			/* Good, check if the desired type exists */ @@ -
1434,6 +1444,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks
**ga, struct tipc_link *l,
			}
		}
	}
net/tipc/link.c +2 -0
+
+ignore:
	/* Other cases: ignore! */
	p = NULL;
Quoted 9 lines
net/tipc/link.c static int tipc_link_proto_rcv(struct tipc_link · line 2287
Quoted 8 lines
net/tipc/link.c +1 -1
@@ -2270,7 +2282,7 @@ static int tipc_link_proto_rcv(struct tipc_link
 *l, struct sk_buff *skb,
 	case STATE_MSG:
 		/* Validate Gap ACK blocks, drop if invalid */
 		glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
-		if (glen > dlen)
+		if (glen > dlen || ((l->peer_caps & TIPC_GAP_ACK_BLOCK) &&
 !glen))
This checking is redundant. Existing sanity checking is good enough.
The unicast caller-side drop ((l->peer_caps & TIPC_GAP_ACK_BLOCK) && !glen) is also necessary. Once the capability is negotiated, a valid Gap ACK record MUST have at least the fixed 4-byte header. If glen == 0 from such a peer, it indicates a malformed payload.
The STATE message must be dropped here so it is not passed on to tipc_mon_rcv() as if monitor data started at data + 0, which would misalign the monitor payload parsing.
Quoted 6 lines
Quoted 6 lines
net/tipc/link.c +0 -0
 			break;
		l->rcv_nxt_state = msg_seqno(hdr) + 1;
--
2.34.1