Merge lp:~christof-mroz/hipl/midauth-nfqueue-fix into lp:hipl

[Christof Mroz]

This is a fix for Bug #1554072, based on an earlier fix which seemed to work. Keeping this as a separate branch since I didn't have a chance to test it yet.

[Miika Komu (miika-iki)]

Tested, works (see the log snippet below)! Please merge and commit.

debug(hipfw/hipfw.c:1060@filter_hip): falling back to default HIP/ESP behavior, target 1
debug(hipfw/conntrack.c:2059@get_tuple_by_hits): connection found,
debug(hipfw/conntrack.c:1736@check_packet): check packet: type 3
debug(hipfw/midauth.c:345@hipfw_midauth_verify_challenge): Correct CHALLENGE_RESPONSE found
info(hipfw/conntrack.c:1065@fw_verify_and_store_host_id): HI -> HIT mapping verified
info(hipfw/conntrack.c:1030@fw_verify_packet): Signature successfully verified
debug(hipfw/conntrack.c:310@get_esp_address): Looking for entry with addr: : 172.17.0.2
debug(hipfw/conntrack.c:328@get_esp_address): no matching entry found
debug(hipfw/conntrack.c:508@update_esp_address): address: ::ffff:172.17.0.2
debug(hipfw/dlist.c:137@append_to_list): List is empty inserting first node
debug(hipfw/conntrack.c:1791@check_packet): udp_encap_hdr=0x6adb94 tuple=0x25c4c90 err=1
debug(hipfw/conntrack.c:1797@check_packet): UDP src port 10500
debug(hipfw/conntrack.c:1798@check_packet): UDP dst port 10500
debug(hipfw/hipfw.c:1656@fw_handle_packet): === Verdict: allow modified packet ===
debug(hipfw/rewrite.c:383@allow_modified_packet): Packet accepted with modifications

review: Approve

[Miika Komu (miika-iki)]

Merged. Thanks again for the fix!

=== modified file 'hipfw/rewrite.c'
--- hipfw/rewrite.c	2016-03-09 14:40:26 +0000
+++ hipfw/rewrite.c	2016-03-09 23:08:08 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2013 Aalto University and RWTH Aachen University.
+ * Copyright (c) 2011, 2013, 2016 Aalto University and RWTH Aachen University.
  *
  * Permission is hereby granted, free of charge, to any person
  * obtaining a copy of this software and associated documentation
@@ -69,16 +69,15 @@
 #include "libcore/debug.h"
 #include "rewrite.h"
 
-// static configuration
-// note: see bug report lp:1554072 (setting this to false does not work)
-static const bool assume_ipq_buffer_sufficient = true;
-
-struct scratch_buffer {
-    hip_ipq_packet_msg ipq;
-    uint8_t           *payload[HIP_MAX_PACKET];
-} __attribute__((packed)); // no gaps between header and payload
-
-static struct scratch_buffer scratch_buffer;
+/**
+ * Don't copy into a temporary buffer if true. May improve performance a bit,
+ * but is not considered safe yet, as explained in this file's top-level
+ * documentation.
+ * May be useful for debugging or benchmarking the effect of copying, however.
+ */
+static const bool assume_ipq_buffer_sufficient = false;
+
+static uint8_t scratch_buffer[HIP_MAX_PACKET];
 
 /**
  * Given the address @a field of a struct member of @a old, return
@@ -146,25 +145,33 @@
         return;
     }
 
-    if (ctx->ipq_packet != &scratch_buffer.ipq) {
-        // simply rebase the old pointers
+    if (ctx->ipq_packet->payload != scratch_buffer) {
+        // copy packet data
+        memcpy(scratch_buffer, ctx->ipq_packet->payload,
+               ctx->ipq_packet->data_len);
+
+        // rebase pointers in ctx to point into copy
         if (ctx->ip_version == 4) {
-            ctx->ip_hdr.ipv4 = rebase(ctx->ip_hdr.ipv4, ctx->ipq_packet,
-                                      &scratch_buffer.ipq);
+            ctx->ip_hdr.ipv4 = rebase(ctx->ip_hdr.ipv4,
+                                      ctx->ipq_packet->payload,
+                                      scratch_buffer);
         } else {
             HIP_ASSERT(ctx->ip_version == 6);
-            ctx->ip_hdr.ipv6 = rebase(ctx->ip_hdr.ipv6, ctx->ipq_packet,
-                                      &scratch_buffer.ipq);
+            ctx->ip_hdr.ipv6 = rebase(ctx->ip_hdr.ipv6,
+                                      ctx->ipq_packet->payload,
+                                      scratch_buffer);
         }
 
         switch (ctx->packet_type) {
         case ESP_PACKET:
-            ctx->transport_hdr.esp = rebase(ctx->transport_hdr.esp, ctx->ipq_packet,
-                                            &scratch_buffer.ipq);
+            ctx->transport_hdr.esp = rebase(ctx->transport_hdr.esp,
+                                            ctx->ipq_packet->payload,
+                                            scratch_buffer);
             break;
         case HIP_PACKET:
-            ctx->transport_hdr.hip = rebase(ctx->transport_hdr.hip, ctx->ipq_packet,
-                                            &scratch_buffer.ipq);
+            ctx->transport_hdr.hip = rebase(ctx->transport_hdr.hip,
+                                            ctx->ipq_packet->payload,
+                                            scratch_buffer);
             break;
         case OTHER_PACKET:
             break;
@@ -173,15 +180,14 @@
         }
 
         if (ctx->udp_encap_hdr) {
-            ctx->udp_encap_hdr = rebase(ctx->udp_encap_hdr, ctx->ipq_packet,
-                                        &scratch_buffer.ipq);
+            ctx->udp_encap_hdr = rebase(ctx->udp_encap_hdr,
+                                        ctx->ipq_packet->payload,
+                                        scratch_buffer);
         }
 
-        // copy ipq packet plus payload
-        memcpy(&scratch_buffer.ipq, ctx->ipq_packet,
-               sizeof(*ctx->ipq_packet) + ctx->ipq_packet->data_len);
-        ctx->ipq_packet = &scratch_buffer.ipq;
-        ctx->modified   = 1;
+        // set payload pointer to copy
+        ctx->ipq_packet->payload = scratch_buffer;
+        ctx->modified            = 1;
     } else {
         // second invocation
         HIP_ASSERT(ctx->modified);
@@ -212,7 +218,7 @@
     // RFC 5201: Types 0 - 1023 are signed, so they must not be moved
     HIP_ASSERT(param_type >= 1024);
 
-    if (ctx->ipq_packet->data_len + param_len > sizeof(scratch_buffer.payload)) {
+    if (ctx->ipq_packet->data_len + param_len > HIP_MAX_PACKET) {
         HIP_ERROR("New parameter of type %u, effective size %u, "
                   "does not fit into packet", param_type, param_len);
         return false;