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

[Tung Quang Nguyen]

>Subject: [PATCH net] 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.
>
>Apply the same bound the unicast path uses to the broadcast caller: drop the
>Gap ACK blocks when the reported size exceeds the message data size.
>A NULL ga is already the defined "no Gap ACK blocks" case, so well-formed
>state messages are unaffected.
>
>Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
>Cc: stable@xxxxxxxxxxxxxxx
>Assisted-by: Bynario AI
>Signed-off-by: Samuel Page <sam@xxxxxxxx>
>---
>Before posting I found an earlier thread for what looks like the same (or a very
>closely related) issue:
>
>
>https://lore.kernel.org/netdev/1316452e465e9a96fce44ec15130a14f3872149f.
>1775809727.git.caoruide123@xxxxxxxxx/
>  [PATCH net 1/1] tipc: validate Gap ACK blocks in STATE message
>
>That one added the validation inside tipc_get_gap_ack_blks() and the thread
>stalled on whether the extra checks were redundant. This patch instead adds,
>on the broadcast caller, only the same bound the unicast path already applies,
>and includes the KASAN reproducer that was asked for there.
>
> net/tipc/bcast.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/bcast.c b/net/tipc/bcast.c index
>76a1585d3f6b..61c83bd95755 100644
>--- a/net/tipc/bcast.c
>+++ b/net/tipc/bcast.c
>@@ -502,6 +502,7 @@ int tipc_bcast_sync_rcv(struct net *net, struct tipc_link
>*l,
> 	struct sk_buff_head *inputq = &tipc_bc_base(net)->inputq;
> 	struct tipc_gap_ack_blks *ga;
> 	struct sk_buff_head xmitq;
>+	u16 glen;
> 	int rc = 0;
>
> 	__skb_queue_head_init(&xmitq);
>@@ -510,7 +511,10 @@ 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);
>+		/* Validate Gap ACK blocks, drop if invalid */
>+		glen = tipc_get_gap_ack_blks(&ga, l, hdr, false);
>+		if (glen > msg_data_sz(hdr))
>+			ga = NULL;

This is wrong because the skb is not dropped as it should be.
Note that 'ga' is NULL just for legacy TIPC that does not support Selective ACK.
To correctly fix this issue, you need to set a flag (for example, a Boolean output parameter) to TRUE instead of 'ga=NULL'.
Then, immediately return and repeatedly pass the flag to tipc_rcv() in order to drop the skb.

> 		if (!sysctl_tipc_bc_retruni)
> 			retrq = &xmitq;
> 		rc = tipc_link_bc_ack_rcv(l, msg_bcast_ack(hdr),
>
>base-commit: a986fde914d88af47eb78fd29c5d1af7952c3500
>--
>2.54.0