[PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[David Lee]

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 without checking
that the end of a received multicast TVLV is representable.

A sufficiently large TVLV therefore wraps the transport header offset.
skb_network_header_len() then returns a bogus large length, allowing
batadv_mcast_forw_packet() to access memory beyond the skb data.

Reject multicast TVLVs whose absolute end offset cannot be represented
before changing either header field.

Fixes: 07afe1ba288c ("batman-adv: mcast: implement multicast packet reception and forwarding")
Bug found and triaged by OpenAI Security Research and 
validated by Trail of Bits.

Assisted-by: Codex:gpt-5.6-sol gpt-5.5-cyber
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
Trail of Bits has a reproducer for this bug that triggers a
KASAN use-after-free and can share if needed.

 net/batman-adv/tvlv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/batman-adv/tvlv.c b/net/batman-adv/tvlv.c
index 1c9fb2198..5e9d0e2dd 100644
--- a/net/batman-adv/tvlv.c
+++ b/net/batman-adv/tvlv.c
@@ -433,6 +433,9 @@ 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_headroom(skb) + tvlv_offset + tvlv_value_len >= U16_MAX)
+			return -EINVAL;
+
 		skb_set_network_header(skb, tvlv_offset);
 		skb_set_transport_header(skb, tvlv_offset + tvlv_value_len);
 
-- 
2.53.0

Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[Sven Eckelmann]

On Fri, 31 Jul 2026 13:52:22 +0000, David Lee <david.lee@trailofbits.com> wrote:
> batman-adv: reject unrepresentable multicast TVLV offsets

Acked-by: Sven Eckelmann <sven@narfation.org>

-- 
Sven Eckelmann <sven@narfation.org>

Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[Sven Eckelmann]

On Fri, 31 Jul 2026 13:52:22 +0000, David Lee <david.lee@trailofbits.com> wrote:
> 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 without checking
> that the end of a received multicast TVLV is representable.
> 
> A sufficiently large TVLV therefore wraps the transport header offset.
> skb_network_header_len() then returns a bogus large length, allowing
> batadv_mcast_forw_packet() to access memory beyond the skb data.

Ok, we end up here with transport_header < network_header. This is then
wrapping in

static inline u32 skb_network_header_len(const struct sk_buff *skb)
{
	DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
	return skb->transport_header - skb->network_header;
}

to a really large value.

And as result, following allows larger tvlv_lens than are actually practical
possible:

	/* check if full tracker tvlv is within skb length */
	tvlv_len = sizeof(*mcast_tracker) + ETH_ALEN * num_dests;
	if (tvlv_len > skb_network_header_len(skb))
		return -EINVAL;

Just because tvlv_len is insane large, correct?


@netdev: Just to sidetrack a little bit: I am wondering if `transport_header <
network_header` should also be handled in skb_network_header_len() (and similar
functions)? And if the return value of u32 should be reduced to u16.

In this specific case, a return value of u16 would have returned the correct
result instead of the insane large length - but trigger and WARN_ON_ONCE for
transport_header == U16_MAX case.

> 
> Reject multicast TVLVs whose absolute end offset cannot be represented
> before changing either header field.
> 
> Fixes: 07afe1ba288c ("batman-adv: mcast: implement multicast packet reception and forwarding")
> Bug found and triaged by OpenAI Security Research and
> validated by Trail of Bits.
> 
> Assisted-by: Codex:gpt-5.6-sol gpt-5.5-cyber

This is not how we write the tags section of a commit message. The "Bug
found..." looks like it was accidentally added and doesn't belong here. And it
seems like this was only added by you for the patches you've send out today:

https://lore.kernel.org/all/?q=Bug%20found%20and%20triaged%20by%20OpenAI%20Security%20Research%20and


See https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
for more documentation how you can document such things.

How do you want to procede here? Because the rest looks good.

>
>
> diff --git a/net/batman-adv/tvlv.c b/net/batman-adv/tvlv.c
> index 5600aaf00627c..8354c62bd86a7 100644
> --- a/net/batman-adv/tvlv.c
> +++ b/net/batman-adv/tvlv.c
> @@ -437,6 +437,9 @@ 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_headroom(skb) + tvlv_offset + tvlv_value_len >= U16_MAX)
> +			return -EINVAL;
> +

Just for documentation purposes:

This is (skb->data - skb->head) + tvlv_offset + tvlv_value_len

The calculation in skb_set_transport_header():

offset = skb->data - skb->head
offset += (tvlv_offset + tvlv_value_len)

>  		skb_set_network_header(skb, tvlv_offset);
>  		skb_set_transport_header(skb, tvlv_offset + tvlv_value_len);
>  

Just a lazy way to show that the return value u16 returns more sane results:

 #include <stdint.h>
 #include <stdio.h>

// #define RETURN_TEST_TYPE uint32_t
 #define RETURN_TEST_TYPE uint16_t

uint16_t network_header(unsigned long headroom, const int offset)
{
	return headroom + offset;
}

uint16_t transport_header(unsigned long headroom, const int offset)
{
	return headroom + offset;
}

RETURN_TEST_TYPE network_header_len(uint16_t network_header, uint16_t transport_header)
{
	// ignore this for now: DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
	return transport_header - network_header;
}

int main(void)
{
        unsigned int tvlv_value_len;
        unsigned int tvlv_offset;
	unsigned int headroom;
	uint16_t transport;
	uint16_t network;
	unsigned int t;

	for (headroom = 0; headroom < 65535; headroom++) {
		for (tvlv_offset = 0; tvlv_offset < 65535; tvlv_offset++) {
			for (tvlv_value_len = 0; tvlv_value_len < 65535; tvlv_value_len++) {
				network = network_header(headroom, tvlv_offset);
				transport = transport_header(headroom,
                                                             tvlv_offset + tvlv_value_len);
				t = network_header_len(network, transport);

				if (t != tvlv_value_len)
					printf("Failed for headroom %u tvlv_offset %u, expected %u, seen %u\n",
					       headroom, tvlv_offset, tvlv_value_len, t);
			}
		}
	}

	return 0;
}

-- 
Sven Eckelmann <sven@narfation.org>

Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

On Friday, 31 July 2026 17:46:08 CEST Sven Eckelmann wrote:
> >
> >
> > diff --git a/net/batman-adv/tvlv.c b/net/batman-adv/tvlv.c
> > index 5600aaf00627c..8354c62bd86a7 100644
> > --- a/net/batman-adv/tvlv.c
> > +++ b/net/batman-adv/tvlv.c
> > @@ -437,6 +437,9 @@ 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_headroom(skb) + tvlv_offset + tvlv_value_len >= U16_MAX)
> > +                      return -EINVAL;
> > +
> 
> Just for documentation purposes:
> 
> This is (skb->data - skb->head) + tvlv_offset + tvlv_value_len
> 
> The calculation in skb_set_transport_header():
> 
> offset = skb->data - skb->head
> offset += (tvlv_offset + tvlv_value_len)
> 
> >                skb_set_network_header(skb, tvlv_offset);
> >                skb_set_transport_header(skb, tvlv_offset + tvlv_value_len);
> >  

I've checked a little bit further and it might not be the preferred solution. 
Please check Eric Dumazet's
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d45cf1e7d7180256e17c9ce88e32e8061a7887fe 
for a similar problem in IPv6. It is basically the same but without the 
additional offset parameter which skb_set_transport_header_careful would need.

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets (+ b4 check signed-off-problems)

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

On Friday, 31 July 2026 15:52:22 CEST David Lee wrote:
> Signed-off-by: Kyle Zeng <kylebot@openai.com>

Another thing just realized (and for whatever reason it was not shown by b4): 
Who is Kyle Zeng <kylebot@openai.com> here (what does kylebot have to do 
with the patch)? David Lee <david.lee@trailofbits.com> is here marked as the 
author but didn't even Signed-off-by the patch. See 
https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
(see the end of mail of pending things for this patch)



@Konstanin (Joe/Andy): Is it to be expected that 
`b4 review track 20260731135222.566367-1-david.lee@trailofbits.com` -> 
`b4 review tui` -> "review" -> "checks" patch doesn't show the Signed-off-by 
error in the checkpatch check view? When doing it manually, it works as 
expected:

$ b4 am 20260731135222.566367-1-david.lee@trailofbits.com
[..]
$ ./scripts/checkpatch.pl -q --terse --no-summary ./20260731_david_lee_batman_adv_reject_unrepresentable_multicast_tvlv_offsets.mbx
./20260731_david_lee_batman_adv_reject_unrepresentable_multicast_tvlv_offsets.mbx:52: ERROR: Missing Signed-off-by: line by nominal patch author 'David Lee <david.lee@trailofbits.com>'

$ b4 --version
0.15.2

The difference comes from the "--mailback" option which b4 adds also to 
checkpatch. I have therefore also Cc'ed Joe Perches and Andy Whitcroft because 
of this suppressed error. The following change would produce the output but I 
have no glue and just fiddled around with the script without understanding the 
bigger concept (of checkpatch and why b4 uses --mailback):

diff --git i/scripts/checkpatch.pl w/scripts/checkpatch.pl
index 7a846a3ea1273..ccb2ec78193e0 100755
--- i/scripts/checkpatch.pl
+++ w/scripts/checkpatch.pl
@@ -7846,12 +7846,6 @@ sub process {
 		exit(0);
 	}
 
-	# In mailback mode only produce a report in the negative, for
-	# things that appear to be patches.
-	if ($mailback && ($clean == 1 || !$is_patch)) {
-		exit(0);
-	}
-
 	# This is not a patch, and we are in 'no-patch' mode so
 	# just keep quiet.
 	if (!$chk_patch && !$is_patch) {
@@ -7902,6 +7896,12 @@ sub process {
 		}
 	}
 
+	# In mailback mode only produce a report in the negative, for
+	# things that appear to be patches.
+	if ($mailback && ($clean == 1 || !$is_patch)) {
+		exit(0);
+	}
+
 	print report_dump();
 	if ($summary && !($clean == 1 && $quiet == 1)) {
 		print "$filename " if ($summary_file);

(this is just to show the cause - not meant as anything which should be applied)

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Friday, 31 July 2026 17:46:08 CEST Sven Eckelmann wrote:
> On Fri, 31 Jul 2026 13:52:22 +0000, David Lee <david.lee@trailofbits.com> wrote:
> > batman-adv: reject unrepresentable multicast TVLV offsets
> 
> Acked-by: Sven Eckelmann <sven@narfation.org>

I am hereby withdrawing this.

NACKed-by: Sven Eckelmann <sven@narfation.org>

This needs a different implementation and a clean patch submission. Summary:

1. weird text + newlines in the tags section which shouldn't be there

2. author didn't sign off the patch

3. weird unrelated Signed-off-by without any understandable connection to 
   patch

4. the checks should be in the API and not added in random places in callers 
   of the API (which makes it hard to understand why the check is there). The 
   users of the API should only handle the return values of the API functions. 
   See commit d45cf1e7d718 ("ipv6: reject malicious packets in 
   ipv6_gso_segment()")

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]