Hi devs,
I found out that the batman-adv kernel modules is quite out-dated in
Linux Mint (and probably also Ubuntu/Debian unstable).
Using 'lsmod batman-adv' it says: version 2013.5.0. But the current
version is 2014.3.0.
Is it on the planning to update to the newest version? Or what could
we / I do to update the package(s)?
Thanks!
Kind regards,
Melroy van den Berg
Hi,
the mail below went to the wrong mailing list (commits@ is read-only).
Cheers,
Marek
On Friday 12 December 2014 22:04:10 Melroy van den Berg wrote:
> Hi,
>
> This is my first patch :). This is small one, but it's a bug.
>
> From 414638b05aacdfb111c8f4bf0e35d63b65b8ebdf Mon Sep 17 00:00:00 2001
> From: Melroy van den Berg <webmaster1989(a)gmail.com>
> Date: Fri, 12 Dec 2014 21:56:32 +0100
> Subject: [PATCH] batman-adv: Use batman_adv (instead of batman-adv) to grep
> in
> dmesg, because batman_adv is the real module name.
>
> Signed-off-by: Melroy van den Berg <webmaster1989(a)gmail.com>
> ---
> README | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/README b/README
> index 58e4904..4f401e5 100644
> --- a/README
> +++ b/README
> @@ -135,7 +135,7 @@ mands: dmesg, logread, or looking in the files
> /var/log/kern.log
> or /var/log/syslog. All batman-adv messages are prefixed with
> "batman-adv:" So to see just these messages try
>
> -# dmesg | grep batman-adv
> +# dmesg | grep batman_adv
>
> When investigating problems with your mesh network it is some-
> times necessary to see more detail debug messages. This must be
The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606
("batman-adv: Receive fragmented packets and merge"). The new code provided a
mostly unused parameter skb for the merging function. It is used inside the
function to calculate the additionally needed skb tailroom. But instead of
increasing its own tailroom, it is only increasing the tailroom of the first
queued skb. This is not correct in some situations because the first queued
entry can be a different one than the parameter.
An observed problem was:
1. packet with size 104, total_size 1464, fragno 1 was received
- packet is queued
2. packet with size 1400, total_size 1464, fragno 0 was received
- packet is queued at the end of the list
3. enough data was received and can be given to the merge function
(1464 == (1400 - 20) + (104 - 20))
- merge functions gets 1400 byte large packet as skb argument
4. merge function gets first entry in queue (104 byte)
- stored as skb_out
5. merge function calculates the required extra tail as total_size - skb->len
- pskb_expand_head tail of skb_out with 64 bytes
6. merge function tries to squeeze the extra 1380 bytes from the second queued
skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out
Instead calculate the extra required tail bytes for skb_out also using skb_out
instead of using the parameter skb. The skb parameter is only used to get the
total_size from the last received packet. This is also the total_size used to
decide that all fragments were received.
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
Reported-by: Philipp Psurek <philipp.psurek(a)gmail.com>
---
This is a minimized version which doesn't require the patch
"[PATCH-maint] batman-adv: Check total_size when reassembling fragments".
---
fragmentation.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fragmentation.c b/fragmentation.c
index f14e54a..af16844 100644
--- a/fragmentation.c
+++ b/fragmentation.c
@@ -247,7 +247,7 @@ batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
kfree(entry);
/* Make room for the rest of the fragments. */
- if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
+ if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) {
kfree_skb(skb_out);
skb_out = NULL;
goto free;
--
2.1.3
Hi All,
I've partially covered a large site in routers running batman-adv. The
mesh is used for mobile access to equipment on a static site copper
and fibre network around the site. A few of the routers have WAN
connections to the site network. The routers with WAN connections are
configured as gateways in batman-adv. There are gaps in the mesh;
typically there is no route between WAN-connected routers via the
mesh.
I would like to join up the separate mesh segments, via the site
network, such that a client to one segment can ping a client of
another network, having the traffic transparently tunnel via the WAN.
Has anyone looked at doing this in the past?
Thanks,
Travis.
Hi,
please explain how this file can now be used unmodified in userspace (batctl)
and kernel. If it cannot be done without modifying the file then please
provide patches to modify the daily checks which make sure that the files in
both repos are the same. I think the batman-adv developers can give you the
original scripts.
Kind regards,
Sven
The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606
("batman-adv: Receive fragmented packets and merge"). The new code provided a
mostly unused parameter skb for the merging function. It is used inside the
function to calculate the additionally needed skb tailroom. But instead of
increasing its own tailroom, it is only increasing the tailroom of the first
queued skb. This is not correct in most situations because the first queued
entry can be a different one than the parameter.
An observed problem was:
1. packet with size 104, total_size 1464, fragno 1 was received
- packet is queued
2. packet with size 1400, total_size 1464, fragno 0 was received
- packet is queued at the end of the list
3. enough data was received and can be given to the merge function
(1464 == (1400 - 20) + (104 - 20))
- merge functions gets 1400 byte large packet as skb argument
4. merge function gets first entry in queue (104 byte)
- stored as skb_out
5. merge function calculates the required extra tail as total_size - skb->len
- pskb_expand_head tail of skb_out with 64 bytes
6. merge function tries to squeeze the extra 1380 bytes from the second queued
skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out
Instead take only skbs from the queue to merge a packet and remove the
problematic parameter.
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
Reported-by: Philipp Psurek <philipp.psurek(a)gmail.com>
---
This patch requires also the patch
"Check total_size when reassembling fragments" to be applied.
This is only compile tested.
fragmentation.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/fragmentation.c b/fragmentation.c
index c741318..aeaf1ab 100644
--- a/fragmentation.c
+++ b/fragmentation.c
@@ -228,18 +228,13 @@ err:
* Returns the merged skb or NULL on error.
*/
static struct sk_buff *
-batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
+batadv_frag_merge_packets(struct hlist_head *chain)
{
struct batadv_frag_packet *packet;
struct batadv_frag_list_entry *entry;
struct sk_buff *skb_out = NULL;
int size, hdr_size = sizeof(struct batadv_frag_packet);
-
- /* Make sure incoming skb has non-bogus data. */
- packet = (struct batadv_frag_packet *)skb->data;
- size = ntohs(packet->total_size);
- if (size > batadv_frag_size_limit())
- goto free;
+ int extra_tail;
/* Remove first entry, as this is the destination for the rest of the
* fragments.
@@ -249,11 +244,17 @@ batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
skb_out = entry->skb;
kfree(entry);
+ packet = (struct batadv_frag_packet *)skb_out->data;
+ size = ntohs(packet->total_size);
+
/* Make room for the rest of the fragments. */
- if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
- kfree_skb(skb_out);
- skb_out = NULL;
- goto free;
+ if (size > skb_out->len) {
+ extra_tail = size - skb_out->len;
+ if (pskb_expand_head(skb_out, 0, extra_tail, GFP_ATOMIC) < 0) {
+ kfree_skb(skb_out);
+ skb_out = NULL;
+ goto free;
+ }
}
/* Move the existing MAC header to just before the payload. (Override
@@ -304,7 +305,7 @@ bool batadv_frag_skb_buffer(struct sk_buff **skb,
if (hlist_empty(&head))
goto out;
- skb_out = batadv_frag_merge_packets(&head, *skb);
+ skb_out = batadv_frag_merge_packets(&head);
if (!skb_out)
goto out_err;
--
2.1.3
Hi,
I've just noticed that the padding by the underlying network protocol seems
not to be handled by the fragmentation. Maybe Martin can correct me. I will
now use following assumptions:
* the fragmentation code is sending first the last part of the packet and
tries to fill the complete skb (max 1400 byte)
* the mtu of the underlying device is 1400
* the minimum packet size (user data + eth header) of the underlying device
is 70
* the packet send by the user would end up to be 1401 bytes before
fragmentation
Ok, then I would guess that the fragmentation code would try to generate
fragments with the max_fragment_size 1366 (+headers of course, not sure why
the code assumes that the ethernet header is part of the MTU). This would mean
that the 1401 byte packet is split into a 1366 byte fragment (+header) and a
35 byte fragment (+header).
But the 35 byte fragment containing the first part of the packet is (even with
the headers) still smaller than the required packet size of the underlying
device. Now some extra bytes are added as padding to the last fragment
(containing the first part of the original packet).
The receiving node cannot merge the fragments anymore because the length of
the last fragment skb will be too large and therefore the
total_size < chain->size.
Even when it could be merged (because of some bug in the size check) then the
resulting packet would have a padding byte in the middle of of the original
byte.
And just in case somebody has something against the imaginary 70 bytes padding
(802.3 has 60): I had to work with virtual devices in the past which had a
fixed MTU of ~1400 and a minimum packet size of ~1400.
And yes, I am fully aware of the workaround of using an extra virtual device
between batman-adv and the actual device which only adds a header with the
payload length and restores this length on the receiver site. This (or at
least something similar) was used by me in the other project with the MTU/min
packet size of ~1400 device.
Any comments, corrections?
Kind regards,
Sven
The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606
("batman-adv: Receive fragmented packets and merge") by an implementation which
handles the queueing+merging of fragments based on their size and the
total_size of the non-fragmented packet. This total_size is announced by each
fragment. The new implementation doesn't check if the the total_size
information of the packets inside one chain is consistent. This allows an
attacker to inject packets belonging to the same fragmentation sequence number
with varying total_size information. The missing validation can cause a crash
when the fragments are merged because the total_size information is only
retrieved from the first packet by batadv_frag_merge_packets. But the queueing
function batadv_frag_insert_packet always uses the total_size from the latest
packet to check if the fragmented packet was transferred completely and is now
ready to be merged.
Assume two packets with the size x and y.
1. first packet (fragno 1) is sent with a size x and the total_size x+y' (y' < y)
2. second packet (fragno 0) is sent with a size y and the total_size x+y
The fragmentation code would try to merge the two packets because the
accumulated packets have a combined size of x+y and the second packet was sent
with total_size of x+y.
The fragments merging code only took the information from the first packet with
the total_size x+y' and created a buffer with enough space for x+y' bytes. But
the second packet cannot be copied inside the prepared free space because it is
y-y' bytes larger than the remaining space.
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
Acked-by: Martin Hundebøll <martin(a)hundeboll.net>
---
This is only a resend of the patch
https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-November/012584.html
This was necessary because the mail thread was hijacked by others who started
to discuss a different problem which may or may not be caused by the
fragmentation code (or batman-adv at all). At least they removed me from the
Cc so I had not received their "responses".
This also gave me the opportunity to change some words in the commit message.
---
fragmentation.c | 7 +++++--
types.h | 2 ++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/fragmentation.c b/fragmentation.c
index 362e91a..3a19d4d 100644
--- a/fragmentation.c
+++ b/fragmentation.c
@@ -161,6 +161,7 @@ static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,
hlist_add_head(&frag_entry_new->list, &chain->head);
chain->size = skb->len - hdr_size;
chain->timestamp = jiffies;
+ chain->total_size = ntohs(frag_packet->total_size);
ret = true;
goto out;
}
@@ -195,9 +196,11 @@ static bool batadv_frag_insert_packet(struct batadv_orig_node *orig_node,
out:
if (chain->size > batadv_frag_size_limit() ||
- ntohs(frag_packet->total_size) > batadv_frag_size_limit()) {
+ chain->total_size != ntohs(frag_packet->total_size) ||
+ chain->total_size > batadv_frag_size_limit()) {
/* Clear chain if total size of either the list or the packet
- * exceeds the maximum size of one merged packet.
+ * exceeds the maximum size of one merged packet. Don't allow
+ * packets to have different total_size.
*/
batadv_frag_clear_chain(&chain->head);
chain->size = 0;
diff --git a/types.h b/types.h
index 462a70c..c4d7d24 100644
--- a/types.h
+++ b/types.h
@@ -132,6 +132,7 @@ struct batadv_orig_ifinfo {
* @timestamp: time (jiffie) of last received fragment
* @seqno: sequence number of the fragments in the list
* @size: accumulated size of packets in list
+ * @total_size: expected size of the assembled packet
*/
struct batadv_frag_table_entry {
struct hlist_head head;
@@ -139,6 +140,7 @@ struct batadv_frag_table_entry {
unsigned long timestamp;
uint16_t seqno;
uint16_t size;
+ uint16_t total_size;
};
/**
--
2.1.3