RE: [PATCH net v2 2/4] tipc: validate discovery message length before reading media address

[Tung Quang Nguyen]

>Subject: [PATCH net v2 2/4] tipc: validate discovery message length before
>reading media address
>
>tipc_disc_rcv() reads the sender's media address from the fixed media-info
>area of the header (msg_media_addr(), offset
>TIPC_MEDIA_INFO_OFFSET) and, when the peer advertises 128-bit node ids,
>copies a NODE_ID_LEN node id appended after the header. Neither read is
>bounded against the actual received length: tipc_msg_validate() only enforces
>a header size in the range [MIN_H_SIZE, MAX_H_SIZE], so a LINK_CONFIG
>message as short as MIN_H_SIZE (24 bytes) passes validation while the media-
>address read reaches up to MAX_H_SIZE and the node-id read reaches
>MAX_H_SIZE + NODE_ID_LEN.
>
>A node always builds discovery messages at MAX_H_SIZE + NODE_ID_LEN
>(tipc_disc_init_msg()), so a shorter LINK_CONFIG message is malformed.
>Drop such messages before the reads so the media address and node id are
>taken from received data rather than from uninitialised tail room or memory
>beyond the buffer.
>
>A crafted short LINK_CONFIG datagram otherwise makes tipc_disc_rcv() read
>past the received message data when a bearer is enabled.
>
>Fixes: 3d749a6a26b0 ("tipc: Hide media-specific addressing details from
>generic bearer code")
>Assisted-by: Claude:claude-opus-4-7
>Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
>---
> net/tipc/discover.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
>diff --git a/net/tipc/discover.c b/net/tipc/discover.c index
>3e54d2df5683a..daf5f11fc82b4 100644
>--- a/net/tipc/discover.c
>+++ b/net/tipc/discover.c
>@@ -217,6 +217,20 @@ void tipc_disc_rcv(struct net *net, struct sk_buff *skb,
> 	}
> 	hdr = buf_msg(skb);
>
>+	/* A discovery message carries the sender's media address within the
>+	 * fixed-size header and, when 128-bit ids are advertised, a node id
>+	 * appended after it.  A node always builds these messages at
>+	 * MAX_H_SIZE + NODE_ID_LEN, so drop anything too short to hold
>what
>+	 * is read below and keep msg2addr() and the node-id copy within the
>+	 * received data.
>+	 */
>+	if (skb->len < MAX_H_SIZE ||
>+	    ((caps & TIPC_NODE_ID128) && skb->len < MAX_H_SIZE +
>NODE_ID_LEN)) {
>+		pr_warn_ratelimited("Rcv corrupt discovery message\n");
>+		kfree_skb(skb);
>+		return;
>+	}
>+

This patch has 2 problems:
1. It is wrong because 'skb->len' is validated too late. The message has been accessed earlier in variable declarations.
2. It is redundant and never hit because tipc_msg_validate() has already validated it:
tipc_msg_validate()
{
...
if (unlikely(skb->len < msz))
        return false;
...
}

> 	if (caps & TIPC_NODE_ID128)
> 		memcpy(peer_id, msg_node_id(hdr), NODE_ID_LEN);
> 	else
>--
>2.53.0
>