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

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

[Ren Wei]

From: Ruide Cao <caoruide123@gmail.com>

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 callers do not treat malformed Gap
ACK data as monitor payload offset, while preserving the declared size for
valid but unused Gap ACK records. This keeps malformed Gap ACK data ignored
without misaligning monitor payload parsing in unicast STATE messages.

Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Ruide Cao <caoruide123@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/tipc/link.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 49dfc098d89b..a364822c1cd8 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1415,12 +1415,20 @@ 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) {
+		if (dlen < sizeof(*p))
+			goto ignore;
+
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
+		if (sz < sizeof(*p) || 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 +1442,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
 			}
 		}
 	}
+
+ignore:
 	/* Other cases: ignore! */
 	p = NULL;
 
-- 
2.34.1

RE: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message

[Tung Quang Nguyen]

>Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
>
>From: Ruide Cao <caoruide123@gmail.com>
>
>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 callers do not treat malformed Gap ACK data as
>monitor payload offset, while preserving the declared size for valid but unused
>Gap ACK records. This keeps malformed Gap ACK data ignored without
>misaligning monitor payload parsing in unicast STATE messages.
>
>Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
>Reported-by: Yifan Wu <yifanwucs@gmail.com>
>Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>Co-developed-by: Yuan Tan <yuantan098@gmail.com>
>Signed-off-by: Yuan Tan <yuantan098@gmail.com>
>Suggested-by: Xin Liu <bird@lzu.edu.cn>
>Tested-by: Ren Wei <enjou1224z@gmail.com>
>Signed-off-by: Ruide Cao <caoruide123@gmail.com>
>Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>---
> net/tipc/link.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/link.c b/net/tipc/link.c index 49dfc098d89b..a364822c1cd8
>100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -1415,12 +1415,20 @@ 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) {
>+		if (dlen < sizeof(*p))
Your patch is wrong. sizeof(*p) is equivalent to sizeof(NULL) which is 4 bytes. I guess you did not really test this code to see if it is ever hit.
>+			goto ignore;
>+
> 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
> 		sz = ntohs(p->len);
>+		if (sz < sizeof(*p) || sz > dlen) {
sizeof(*p) cannot be used because tipc_gap_ack_blks contains flexible array. struct_size() must be used in stead.
The above 2 comparisons are redundant because they are already done in below "Sanity check" and in tipc_link_proto_rcv():
tipc_link_proto_rcv()
{
...
/* Validate Gap ACK blocks, drop if invalid */
glen = tipc_get_gap_ack_blks(&ga, l, hdr, true);
      if (glen > dlen)
           break;
...
}
>+			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 +1442,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga,
>struct tipc_link *l,
> 			}
> 		}
> 	}
>+
>+ignore:
> 	/* Other cases: ignore! */
> 	p = NULL;
>
>--
>2.34.1
>

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

[Ren Wei]

From: Ruide Cao <caoruide123@gmail.com>

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: stable@kernel.org
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Co-developed-by: Yuan Tan <yuantan098@gmail.com>
Signed-off-by: Yuan Tan <yuantan098@gmail.com>
Suggested-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Ren Wei <enjou1224z@gmail.com>
Signed-off-by: Ruide Cao <caoruide123@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/tipc/link.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 49dfc098d89b..44678d98939a 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -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;
-- 
2.34.1

RE: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message

[Tung Quang Nguyen]

>Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
>
>From: Ruide Cao <caoruide123@gmail.com>
>
>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.
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 ?
>
>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: stable@kernel.org
>Reported-by: Yifan Wu <yifanwucs@gmail.com>
>Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>Co-developed-by: Yuan Tan <yuantan098@gmail.com>
>Signed-off-by: Yuan Tan <yuantan098@gmail.com>
>Suggested-by: Xin Liu <bird@lzu.edu.cn>
>Tested-by: Ren Wei <enjou1224z@gmail.com>
>Signed-off-by: Ruide Cao <caoruide123@gmail.com>
>Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>---
> net/tipc/link.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/net/tipc/link.c b/net/tipc/link.c index 49dfc098d89b..44678d98939a
>100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -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;
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.
>+
> 		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.
>+
> 		/* 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))
This checking is redundant. Existing sanity checking is good enough.
> 			break;
>
> 		l->rcv_nxt_state = msg_seqno(hdr) + 1;
>--
>2.34.1
>

Re: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message

[Ruide Cao]

On 4/12/2026 8:06 PM, Tung Quang Nguyen wrote:
>> Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
>>
>> From: Ruide Cao <caoruide123@gmail.com>
>>
>> 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.
> 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.

>> 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: stable@kernel.org
>> Reported-by: Yifan Wu <yifanwucs@gmail.com>
>> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>> Co-developed-by: Yuan Tan <yuantan098@gmail.com>
>> Signed-off-by: Yuan Tan <yuantan098@gmail.com>
>> Suggested-by: Xin Liu <bird@lzu.edu.cn>
>> Tested-by: Ren Wei <enjou1224z@gmail.com>
>> Signed-off-by: Ruide Cao <caoruide123@gmail.com>
>> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>> ---
>> net/tipc/link.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/tipc/link.c b/net/tipc/link.c index 49dfc098d89b..44678d98939a
>> 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -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;
> 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.

>> +
>> 		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.

>> +
>> 		/* 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))
> 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.

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

RE: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message

[Tung Quang Nguyen]

>Subject: Re: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
>
>
>On 4/12/2026 8:06 PM, Tung Quang Nguyen wrote:
>>> Subject: [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE
>>> message
>>>
>>> From: Ruide Cao <caoruide123@gmail.com>
>>>
>>> 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.
>> 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
>
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 ?
>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.
>
>>> 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: stable@kernel.org
>>> Reported-by: Yifan Wu <yifanwucs@gmail.com>
>>> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
>>> Co-developed-by: Yuan Tan <yuantan098@gmail.com>
>>> Signed-off-by: Yuan Tan <yuantan098@gmail.com>
>>> Suggested-by: Xin Liu <bird@lzu.edu.cn>
>>> Tested-by: Ren Wei <enjou1224z@gmail.com>
>>> Signed-off-by: Ruide Cao <caoruide123@gmail.com>
>>> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
>>> ---
>>> net/tipc/link.c | 16 ++++++++++++++--
>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/tipc/link.c b/net/tipc/link.c index
>>> 49dfc098d89b..44678d98939a
>>> 100644
>>> --- a/net/tipc/link.c
>>> +++ b/net/tipc/link.c
>>> @@ -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;
>> 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`.
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 ?
>Without this new check, an Out-of-Bounds (OOB) read occurs before the old
>sanity check even has a chance to run.
>
>>> +
>>> 		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.
>
>>> +
>>> 		/* 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))
>> 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.
>
>>> 			break;
>>>
>>> 		l->rcv_nxt_state = msg_seqno(hdr) + 1;
>>> --
>>> 2.34.1
>>>