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

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)

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

[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