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

[Ruide Cao <caoruide123@gmail.com>]

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