batman-adv: reject unrepresentable multicast TVLV offsets

From: Kyle Zeng <[email protected]>

The network and transport header fields in struct sk_buff are 16-bit offsets from skb->head, and U16_MAX is reserved as the unset transport header value. batadv_tvlv_call_handler() sets both fields from a received multicast TVLV without checking whether the TVLV end is representable.

If the end offset exceeds the field's range, skb_set_transport_header() truncates it so that the transport header precedes the network header. The negative difference is then returned by skb_network_header_len() as a large u32. batadv_mcast_forw_packet() consequently accepts an oversized multicast tracker and accesses memory beyond the skb data.

Add skb_set_transport_header_careful(), an offset-aware counterpart to skb_reset_transport_header_careful(), which validates the final head-relative offset before assigning it. Use the new helper in batadv_tvlv_call_handler() and reject unrepresentable TVLVs before setting the network header.

---
Changes in v3:
- Rename transport_offset to thoff so the comparisons fit on one line.

v2: https://lore.kernel.org/netdev/[email protected]/

Changes in v2:
- Add an offset-aware careful transport-header setter and make batman-adv handle its failure, as suggested by Sven Eckelmann.
- Restore Kyle Zeng as the patch author and correct the sign-off chain.
- Move the research credit below the commit-message separator.

v1: https://lore.kernel.org/all/[email protected]/

Bug found and triaged by OpenAI Security Research and validated by Trail of Bits.

Trail of Bits has a reproducer for this bug that triggers a KASAN use-after-free and can share if needed.

 include/linux/skbuff.h | 24 ++++++++++++++++++++++++
 net/batman-adv/tvlv.c  |  5 ++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

index 22eda1d54a0e..ed6a2bc23db5 100644
@@ -3126,6 +3126,30 @@ static inline void skb_set_transport_header(struct sk_buff *skb,
 	skb->transport_header += offset;
 }
 
+/**
+ * skb_set_transport_header_careful - conditionally set transport header
+ * @skb: buffer to alter
+ * @offset: offset to add to skb->data
+ *
+ * Hardened version of skb_set_transport_header().
+ *
+ * Returns: true if the operation was a success.
+ */
+static inline bool __must_check
+skb_set_transport_header_careful(struct sk_buff *skb, const int offset)
+{
+	long thoff = skb->data - skb->head + offset;
+
+	if (unlikely(thoff != (typeof(skb->transport_header))thoff))
+		return false;
+
+	if (unlikely(thoff == (typeof(skb->transport_header))~0U))
+		return false;
+
+	skb->transport_header = thoff;
+	return true;
+}
+
 static inline unsigned char *skb_network_header(const struct sk_buff *skb)
 {
 	return skb->head + skb->network_header;

index 1c9fb21985f6..c384db16bcac 100644
@@ -433,8 +433,11 @@ static int batadv_tvlv_call_handler(struct batadv_priv *bat_priv,
 			return NET_RX_SUCCESS;
 
 		tvlv_offset = (unsigned char *)tvlv_value - skb->data;
+		if (!skb_set_transport_header_careful(skb,
+						      tvlv_offset + tvlv_value_len))
+			return -EINVAL;
+
 		skb_set_network_header(skb, tvlv_offset);
-		skb_set_transport_header(skb, tvlv_offset + tvlv_value_len);
 
 		return tvlv_handler->mcast_handler(bat_priv, skb);
 	}

2.53.0

[...]

Looks to me like you've also dropped the

b4 diff -v2 [email protected]

Sven Eckelmann <[email protected]>

No, not needed. Just wanted to document what I found.

Regards,
Sven