* Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets
2026-07-31 13:52 [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets David Lee
@ 2026-07-31 15:46 ` Sven Eckelmann
2026-07-31 19:05 ` Sven Eckelmann
2026-07-31 15:46 ` Sven Eckelmann
2026-08-01 9:54 ` [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets (+ b4 check signed-off-problems) Sven Eckelmann
2 siblings, 1 reply; 6+ 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] 6+ messages in thread* Re: [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets (+ b4 check signed-off-problems)
2026-07-31 13:52 [PATCH] batman-adv: reject unrepresentable multicast TVLV offsets David Lee
2026-07-31 15:46 ` Sven Eckelmann
2026-07-31 15:46 ` Sven Eckelmann
@ 2026-08-01 9:54 ` Sven Eckelmann
2 siblings, 0 replies; 6+ messages in thread
From: Sven Eckelmann @ 2026-08-01 9:54 UTC (permalink / raw)
To: David Lee, Kyle Zeng
Cc: Dominik 'Disconnect3d' Czarnota, b.a.t.m.a.n,
linux-kernel, Konstantin Ryabitsev, Kernel.org Tools,
Andy Whitcroft, Joe Perches
[-- 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 --]
^ permalink raw reply related [flat|nested] 6+ messages in thread