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@narfation.org Acked-by: Martin Hundebøll martin@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; };
/**