batman-adv: Check total_size when reassembling fragments

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.

---
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(-)

index 362e91a..3a19d4d 100644
@@ -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;

index 462a70c..c4d7d24 100644
@@ -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

I think this patch should not be applied to maint because parts of the assumptions in the commit message are contradicted by
https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2014-December/012609.html

Maybe I will prepare a v2 which is for master and not for maint. It will not have the "omg, we will all die" scenario in the commit message which no ones seems to have checked (and ended up to be wrong). The main purpose of the upcoming patches would be to remove the skb parameter in the merge function and make sure that the sizes are checked before the queuing is done.

Kind regards,
Sven