ofp-actions: Use aligned structures when decoding ofp actions.

Some openflow actions can be misaligned, e.g., actions whithin OF 1.0
replies to statistics reply messages which have a header of 12 bytes
and no additional padding.

Also, buggy controllers might incorrectly encode actions.

When decoding multiple actions in ofpacts_decode(), make sure that
when advancing to the next action it will be properly aligned
(multiple of OFPACT_ALIGNTO).

Detected by UB Sanitizer when running one of the fuzz tests:

  lib/ofp-actions.c:5347:12:
  runtime error: member access within misaligned address 0x0000016ba274
  for type 'const struct nx_action_learn', which requires 8 byte alignment
  0x0000016ba274: note: pointer points here
    20 20 20 20 ff ff 00 38  00 00 23 20 00 10 20 20
                ^
    20 20 20 20 20 20 20 20  20 20 20 20 00 03 20 00

  0 0x52cece in decode_LEARN_common lib/ofp-actions.c:5347
  1 0x52dcf6 in decode_NXAST_RAW_LEARN lib/ofp-actions.c:5463
  2 0x548604 in ofpact_decode lib/ofp-actions.inc2:4723
  3 0x53ee43 in ofpacts_decode lib/ofp-actions.c:7781
  4 0x53efc1 in ofpacts_pull_openflow_actions__ lib/ofp-actions.c:7820
  5 0x5409e1 in ofpacts_pull_openflow_instructions lib/ofp-actions.c:8396
  6 0x5608a8 in ofputil_decode_flow_stats_reply lib/ofp-flow.c:1100

Acked-by: Adrian Moreno <[email protected]>
Acked-by: Aaron Conole <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
Signed-off-by: Ilya Maximets <[email protected]>

include/openvswitch/ofp-actions.h

@@ -203,6 +203,7 @@ BUILD_ASSERT_DECL(sizeof(struct ofpact) == 4);
 /* Alignment. */
 #define OFPACT_ALIGNTO 8
 #define OFPACT_ALIGN(SIZE) ROUND_UP(SIZE, OFPACT_ALIGNTO)
+#define OFPACT_IS_ALIGNED(ADDR) ((uintptr_t) (ADDR) % OFPACT_ALIGNTO == 0)
 #define OFPACT_PADDED_MEMBERS(MEMBERS) PADDED_MEMBERS(OFPACT_ALIGNTO, MEMBERS)
 
 /* Returns the ofpact following 'ofpact'. */

include/openvswitch/ofpbuf.h

@@ -111,6 +111,7 @@ void ofpbuf_use_ds(struct ofpbuf *, const struct ds *);
 void ofpbuf_use_stack(struct ofpbuf *, void *, size_t);
 void ofpbuf_use_stub(struct ofpbuf *, void *, size_t);
 void ofpbuf_use_const(struct ofpbuf *, const void *, size_t);
+void ofpbuf_use_data(struct ofpbuf *, const void *, size_t);
 
 void ofpbuf_init(struct ofpbuf *, size_t);
 void ofpbuf_uninit(struct ofpbuf *);
@@ -149,6 +150,7 @@ static inline size_t ofpbuf_msgsize(const struct ofpbuf *);
 void ofpbuf_prealloc_headroom(struct ofpbuf *, size_t);
 void ofpbuf_prealloc_tailroom(struct ofpbuf *, size_t);
 void ofpbuf_trim(struct ofpbuf *);
+void ofpbuf_align(struct ofpbuf *);
 void ofpbuf_padto(struct ofpbuf *, size_t);
 void ofpbuf_shift(struct ofpbuf *, int);
 

lib/ofp-actions.c

@@ -424,7 +424,8 @@ static void put_reg_load(struct ofpbuf *openflow,
                          const struct mf_subfield *, uint64_t value);
 
 static enum ofperr ofpact_pull_raw(struct ofpbuf *, enum ofp_version,
-                                   enum ofp_raw_action_type *, uint64_t *arg);
+                                   enum ofp_raw_action_type *, uint64_t *arg,
+                                   size_t *raw_len);
 static void *ofpact_put_raw(struct ofpbuf *, enum ofp_version,
                             enum ofp_raw_action_type, uint64_t arg);
 
@@ -7773,45 +7774,87 @@ check_GOTO_TABLE(const struct ofpact_goto_table *a,
 
 static void
 log_bad_action(const struct ofp_action_header *actions, size_t actions_len,
-               const struct ofp_action_header *bad_action, enum ofperr error)
+               size_t action_offset, enum ofperr error)
 {
     if (!VLOG_DROP_WARN(&rl)) {
         struct ds s;
 
         ds_init(&s);
         ds_put_hex_dump(&s, actions, actions_len, 0, false);
-        VLOG_WARN("bad action at offset %#"PRIxPTR" (%s):\n%s",
-                  (char *)bad_action - (char *)actions,
+        VLOG_WARN("bad action at offset %"PRIuSIZE" (%s):\n%s", action_offset,
                   ofperr_get_name(error), ds_cstr(&s));
         ds_destroy(&s);
     }
 }
 
 static enum ofperr
-ofpacts_decode(const void *actions, size_t actions_len,
-               enum ofp_version ofp_version,
-               const struct vl_mff_map *vl_mff_map,
-               uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts)
+ofpacts_decode_aligned(struct ofpbuf *openflow, enum ofp_version ofp_version,
+                       const struct vl_mff_map *vl_mff_map,
+                       uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts,
+                       size_t *bad_action_offset)
 {
-    struct ofpbuf openflow = ofpbuf_const_initializer(actions, actions_len);
-    while (openflow.size) {
-        const struct ofp_action_header *action = openflow.data;
+    size_t decoded_len = 0;
+    enum ofperr error = 0;
+
+    ovs_assert(OFPACT_IS_ALIGNED(openflow->data));
+    while (openflow->size) {
+        /* Ensure the next action data is properly aligned before decoding it.
+         * Some times it's valid to have to decode actions that are not
+         * properly aligned (e.g., when processing OF 1.0 statistics reply
+         * messages which have a header of 12 bytes - struct ofp10_stats_msg).
+         * In other cases the encoder might be buggy.
+         */
+        if (!OFPACT_IS_ALIGNED(openflow->data)) {
+            ofpbuf_align(openflow);
+        }
+
+        const struct ofp_action_header *action = openflow->data;
         enum ofp_raw_action_type raw;
-        enum ofperr error;
+        size_t act_len = 0;
         uint64_t arg;
 
-        error = ofpact_pull_raw(&openflow, ofp_version, &raw, &arg);
+        error = ofpact_pull_raw(openflow, ofp_version, &raw, &arg, &act_len);
         if (!error) {
             error = ofpact_decode(action, raw, ofp_version, arg, vl_mff_map,
                                   ofpacts_tlv_bitmap, ofpacts);
         }
 
         if (error) {
-            log_bad_action(actions, actions_len, action, error);
-            return error;
+            *bad_action_offset = decoded_len;
+            goto done;
         }
+        decoded_len += act_len;
     }
-    return 0;
+
+done:
+    return error;
+}
+
+static enum ofperr
+ofpacts_decode(const void *actions, size_t actions_len,
+               enum ofp_version ofp_version,
+               const struct vl_mff_map *vl_mff_map,
+               uint64_t *ofpacts_tlv_bitmap, struct ofpbuf *ofpacts)
+{
+    size_t bad_action_offset = 0;
+    struct ofpbuf aligned_buf;
+
+    if (!OFPACT_IS_ALIGNED(actions)) {
+        ofpbuf_init(&aligned_buf, actions_len);
+        ofpbuf_put(&aligned_buf, actions, actions_len);
+    } else {
+        ofpbuf_use_data(&aligned_buf, actions, actions_len);
+    }
+
+    enum ofperr error
+        = ofpacts_decode_aligned(&aligned_buf, ofp_version, vl_mff_map,
+                                 ofpacts_tlv_bitmap, ofpacts,
+                                 &bad_action_offset);
+    if (error) {
+        log_bad_action(actions, actions_len, bad_action_offset, error);
+    }
+    ofpbuf_uninit(&aligned_buf);
+    return error;
 }
 
 static enum ofperr
@@ -9664,14 +9707,15 @@ ofpact_decode_raw(enum ofp_version ofp_version,
 
 static enum ofperr
 ofpact_pull_raw(struct ofpbuf *buf, enum ofp_version ofp_version,
-                enum ofp_raw_action_type *raw, uint64_t *arg)
+                enum ofp_raw_action_type *raw, uint64_t *arg,
+                size_t *raw_len)
 {
     const struct ofp_action_header *oah = buf->data;
     const struct ofpact_raw_instance *action;
     unsigned int length;
     enum ofperr error;
 
-    *raw = *arg = 0;
+    *raw = *arg = *raw_len = 0;
     error = ofpact_decode_raw(ofp_version, oah, buf->size, &action);
     if (error) {
         return error;
@@ -9714,6 +9758,7 @@ ofpact_pull_raw(struct ofpbuf *buf, enum ofp_version ofp_version,
     }
 
     ofpbuf_pull(buf, length);
+    *raw_len = length;
 
     return 0;
 }

lib/ofpbuf.c

@@ -115,6 +115,24 @@ ofpbuf_use_const(struct ofpbuf *b, const void *data, size_t size)
     ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STACK);
 }
 
+/* Initializes 'b' as an ofpbuf whose data starts at 'data' and continues for
+ * 'size' bytes.  This is appropriate for an ofpbuf that will be mostly used to
+ * inspect existing data, however, if needed, data may be occasionally changed.
+ *
+ * The first time the data is changed the provided buffer will be copied into
+ * a malloc()'d buffer.  Thus, it is wise to call ofpbuf_uninit() on an ofpbuf
+ * initialized by this function, so that if it expanded into the heap, that
+ * memory is freed.
+ *
+ * 'base' should be appropriately aligned.  Using an array of uint32_t or
+ * uint64_t for the buffer is a reasonable way to ensure appropriate alignment
+ * for 32- or 64-bit data. */
+void
+ofpbuf_use_data(struct ofpbuf *b, const void *data, size_t size)
+{
+    ofpbuf_use__(b, CONST_CAST(void *, data), size, size, OFPBUF_STUB);
+}
+
 /* Initializes 'b' as an empty ofpbuf with an initial capacity of 'size'
  * bytes. */
 void
@@ -322,6 +340,27 @@ ofpbuf_trim(struct ofpbuf *b)
     }
 }
 
+/* Re-aligns the buffer data.  Relies on malloc() to ensure proper alignment.
+ *
+ * This function should not be called for buffers of type OFPBUF_STACK.
+ */
+void
+ofpbuf_align(struct ofpbuf *b)
+{
+    switch (b->source) {
+    case OFPBUF_MALLOC:
+    case OFPBUF_STUB:
+        /* Resizing 'b' always reallocates the buffer, ensuring proper
+         * alignment.
+         */
+        ofpbuf_resize__(b, 0, 0);
+        break;
+    case OFPBUF_STACK:
+        OVS_NOT_REACHED();
+        break;
+    }
+}
+
 /* If 'b' is shorter than 'length' bytes, pads its tail out with zeros to that
  * length. */
 void