Re: [PATCH] tipc: validate data size before reading Gap ACK block header

Re: [PATCH] tipc: validate data size before reading Gap ACK block header

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

- [Medium] Commit message overstates the impact of the fixed access in
  tipc_get_gap_ack_blks() (net/tipc/link.c:1422-1434).

Pre-existing issues:
- [Medium] tipc_get_gap_ack_blks() (net/tipc/link.c:1421-1441), the
  receive-side validator this patch extends, never bounds the peer-…
- [Low] The backward-compatibility branch of tipc_get_gap_ack_blks()
  writes into peer-supplied received packet data: p->bgack_cnt = 0…

--

tipc: validate data size before reading Gap ACK block header

This adds a length check to tipc_get_gap_ack_blks() so the Gap ACK block
header fields are only read when msg_data_sz(hdr) is at least
sizeof(struct tipc_gap_ack_blks), otherwise the function takes the
p = NULL path.  The commit is marked Fixes: d7626b5acff9 and Cc: stable.

(no Link: tags in the commit header)

> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 6427c69f89294..0be86cbdc913d 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -1419,6 +1419,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
>  
>  	/* Does peer support the Gap ACK blocks feature? */
>  	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
> +		if (msg_data_sz(hdr) < sizeof(*p))
> +			goto out;
>  		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
>  		sz = ntohs(p->len);

[Severity: Medium]
Can the impact described in the changelog actually be produced?  The
changelog says:

    "causing an out-of-bounds read of up to 4 bytes past the valid skb
     data"

and

    "the OOB accesses inside the function itself are undefined behavior
     and are detectable by KASAN."

Both call paths run on a validated and linearized skb.  tipc_msg_validate()
in net/tipc/msg.c enforces:

	msz = msg_size(hdr);
	if (unlikely(msz < hsz))
		return false;
	...
	if (unlikely(skb->len < msz))
		return false;

with hsz bounded by MAX_H_SIZE and pskb_may_pull(skb, hsz) already done.
tipc_rcv() in net/tipc/node.c linearizes the skb, and tipc_link_proto_rcv()
linearizes again.

So msg_data(hdr) is at worst exactly at skb_tail_pointer(), and the worst
case read covers [skb->tail, skb->tail + 4), which is skb tailroom or the
leading bytes of skb_shared_info.  Both live inside the same kmalloc'ed
head allocation, which KASAN does not poison.

Would it be more accurate to describe this as a read of stale bytes past
msg_size() rather than a KASAN-detectable out-of-bounds read?

The changelog also says:

    "In the backward-compatible code path, a 1-byte out-of-bounds write also
     occurs through "p->bgack_cnt = 0"."

That store is reached only when the stale bytes happen to satisfy
sz == struct_size(p, gacks, p->ugack_cnt) with a non-zero p->ugack_cnt, so
"also occurs" reads stronger than the code allows.

Since both callers already discard the result (glen > dlen in
tipc_link_proto_rcv(), glen > msg_data_sz(hdr) in tipc_bcast_sync_rcv()),
is the Cc: stable justified, or is this hardening plus a small behavioural
cleanup?  Before the patch a short STATE_MSG could be dropped or accepted
non-deterministically depending on the trailing garbage, which is worth
mentioning in the changelog instead.

>  		/* Sanity check */

[Severity: Medium]
This isn't a bug introduced by this patch, but while the validator in
tipc_get_gap_ack_blks() is being extended: does the sanity check bound the
peer-supplied counts by MAX_GAP_ACK_BLKS?

The check just below is:

	if (sz == struct_size(p, gacks, size_add(p->ugack_cnt, p->bgack_cnt))) {

With u8 counts this accepts up to 510 blocks, i.e. sz up to about 2044,
while net/tipc/msg.h declares:

	#define MAX_GAP_ACK_BLKS	128

and the legitimate sender caps each direction in
__tipc_build_gap_ack_blks():

			if (++n >= MAX_GAP_ACK_BLKS / 2) {

The consumer then asserts the msg.h limit in tipc_link_advance_transmq():

				WARN_ONCE(more(start, end) ||
					  (!offset &&
					   si < last_ga->bgack_cnt) ||
					  si > MAX_GAP_ACK_BLKS,
					  "Corrupted Gap ACK: %d %d %d %d %d\n",

Can a peer that negotiated TIPC_GAP_ACK_BLOCK send bgack_cnt >= 129 with a
matching len, get the record accepted here, and then trip that WARN_ONCE
via tipc_bcast_sync_rcv() -> tipc_link_bc_ack_rcv() ->
tipc_link_advance_transmq()?  That would be fatal with panic_on_warn.

No out-of-bounds access follows, since the kmemdup is sized by
struct_size(ga, gacks, ga->bgack_cnt) and every walk is bounded by the same
count.  Would adding a ugack_cnt + bgack_cnt <= MAX_GAP_ACK_BLKS check here
be worthwhile while this function is being touched?

> @@ -1435,6 +1437,7 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
>  		}
>  	}
>  	/* Other cases: ignore! */
> +out:
>  	p = NULL;
>  
>  ok:

[Severity: Low]
This is a pre-existing issue, but the backward-compatibility branch of
tipc_get_gap_ack_blks() writes into the received packet:

		} else if (uc && sz == struct_size(p, gacks, p->ugack_cnt)) {
			if (p->ugack_cnt) {
				p->bgack_cnt = 0;

Is the data area known to be exclusively owned at that point?  The only
preparation on this path is skb_linearize():

net/tipc/link.c:tipc_link_proto_rcv() {
	if (skb_linearize(skb))
		goto exit;

	hdr = buf_msg(skb);
	data = msg_data(hdr);

and net/tipc/node.c:tipc_rcv() does the same:

		if (unlikely(skb_linearize(skb))) {
			tipc_node_put(n);
			goto discard;
		}

skb_linearize() returns immediately for an already-linear skb, so it does
not unshare a cloned-but-linear data buffer, and git grep finds no
skb_cloned()/skb_unshare()/skb_ensure_writable() on the LINK_PROTOCOL
receive path in net/tipc/.

If the same frame is delivered to an AF_PACKET tap, packet_rcv() does
skb_share_check() -> skb_clone(), so the buffer TIPC then writes to is
shared, and the clone's reader sees bgack_cnt as 0.

This store predates the patch, and the patch only removes its
out-of-bounds aspect.  If there is a respin, could the unicast count and
size be returned to the caller instead of zeroing a field inside the
received packet?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB000136A75E6C720544CAC017C8B32%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM

[PATCH] tipc: validate data size before reading Gap ACK block header

[Aamir Ahmed]

tipc_get_gap_ack_blks() reads the Gap ACK block header fields (len,
ugack_cnt, bgack_cnt) from msg_data(hdr) without first checking that the
message data area is large enough to hold the header struct.  A peer
that has negotiated TIPC_GAP_ACK_BLOCK capability can send a STATE_MSG
or broadcast PROTOCOL message with a data area shorter than
sizeof(struct tipc_gap_ack_blks), causing an out-of-bounds read of up
to 4 bytes past the valid skb data.

In the backward-compatible code path, a 1-byte out-of-bounds write also
occurs through "p->bgack_cnt = 0".

Both callers (tipc_link_proto_rcv and tipc_bcast_sync_rcv) validate the
returned size against msg_data_sz() after the function returns, so the
invalid data is never used further.  However, the OOB accesses inside
the function itself are undefined behavior and are detectable by KASAN.

Add a check at the start of the function that msg_data_sz(hdr) is at
least sizeof(*p) before reading any fields, jumping to the p = NULL path
when the data area is too small.

Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
Cc: stable@vger.kernel.org
---
 net/tipc/link.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 6427c69f8929..0be86cbdc913 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1419,6 +1419,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
 
 	/* Does peer support the Gap ACK blocks feature? */
 	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
+		if (msg_data_sz(hdr) < sizeof(*p))
+			goto out;
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
 		/* Sanity check */
@@ -1435,6 +1437,7 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks **ga, struct tipc_link *l,
 		}
 	}
 	/* Other cases: ignore! */
+out:
 	p = NULL;
 
 ok:
-- 
2.43.0

RE: [PATCH] tipc: validate data size before reading Gap ACK block header

[Tung Quang Nguyen]

>Subject: [PATCH] tipc: validate data size before reading Gap ACK block header
>
>tipc_get_gap_ack_blks() reads the Gap ACK block header fields (len, ugack_cnt,
>bgack_cnt) from msg_data(hdr) without first checking that the message data
>area is large enough to hold the header struct.  A peer that has negotiated
>TIPC_GAP_ACK_BLOCK capability can send a STATE_MSG or broadcast
>PROTOCOL message with a data area shorter than sizeof(struct
>tipc_gap_ack_blks), causing an out-of-bounds read of up to 4 bytes past the
>valid skb data.
This cannot happen. Please send me your reproducer.
Note that it is not recommended  to create fake TIPC protocol messages via UDP bearer in insecure environment.
In such insecure environment, IPSec must be used: https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-6

Re: [PATCH] tipc: validate data size before reading Gap ACK block header

[Aamir Ahmed]

Thank you for reviewing. I've reviewed this further and there was a
flaw in my methodology.

Reads stay inside the skb and the result is never used, so there is
nothing to show. The commit message overstated this as an
out-of-bounds read and write.

Please drop this patch, apologies.

pw-bot: rejected

Kind Regards,
Aamir Ahmed