tipc: validate Gap ACK blocks in STATE message
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
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 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.
--- net/tipc/link.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
net/tipc/link.c +11 -1
@@ -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 ;
Signature · 1 line
2.34.1
Tung Quang Nguyen
maintainerQuoted 40 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_cntandbgack_cntdirectly frommsg_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_BLOCKcan send a short STATE message with a declared TIPC payload shorter than structtipc_gap_ack_blksand 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 intokmemdup()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 <[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| 12 +++++++++++-1 file changed, 11 insertions(+), 1 deletion(-)net/tipc/link.c index 49dfc098d89b..a364822c1cd8 renamed from net/tipc/link.c +0
-0100644
net/tipc/link.c u16 tipc_get_gap_ack_blks(struct · line 1423
Quoted 10 lines
net/tipc/link.c +2
-1@@ -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.
net/tipc/link.c u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l, · line 1427
Quoted 5 lines
net/tipc/link.c +3
-0+ 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;
...
}
Quoted 21 lines
net/tipc/link.c +4
-0+ sz =0 ; + goto ignore; + } +/* 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 +1442,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;--
2.34.1