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

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 --]