* Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets
[not found] <20260731135222.566367-1-david.lee@trailofbits.com>
@ 2026-07-31 15:46 ` Sven Eckelmann
2026-07-31 19:05 ` Sven Eckelmann
0 siblings, 1 reply; 2+ messages in thread
From: Sven Eckelmann @ 2026-07-31 15:46 UTC (permalink / raw)
To: marek.lindner, sw, antonio, sven
Cc: David Lee, Kyle Zeng, Dominik 'Disconnect3d' Czarnota,
b.a.t.m.a.n, linux-kernel, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev, Linus Lüssing
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>
^ permalink raw reply [flat|nested] 2+ messages in thread