odp-util: Fix netlink message overflow with userdata.

Too big userdata could overflow netlink message leading to out-of-bound
memory accesses or assertion while formatting nested actions.

Fix that by checking the size and returning correct error code.

Credit to OSS-Fuzz.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27640
Fixes: e995e3df57ea ("Allow OVS_USERSPACE_ATTR_USERDATA to be variable length.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Flavio Leitner <fbl@sysclose.org>

5 files changed, 81 insertions, 25 deletions

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 252a91bfa..d65ebb541 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1455,14 +1455,20 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
         int n1 = -1;
         if (ovs_scan(&s[n], ",tunnel_out_port=%"SCNi32")%n",
                      &tunnel_out_port, &n1)) {
-            odp_put_userspace_action(pid, user_data, user_data_size,
-                                     tunnel_out_port, include_actions, actions);
-            res = n + n1;
+            res = odp_put_userspace_action(pid, user_data, user_data_size,
+                                           tunnel_out_port, include_actions,
+                                           actions, NULL);
+            if (!res) {
+                res = n + n1;
+            }
             goto out;
         } else if (s[n] == ')') {
-            odp_put_userspace_action(pid, user_data, user_data_size,
-                                     ODPP_NONE, include_actions, actions);
-            res = n + 1;
+            res = odp_put_userspace_action(pid, user_data, user_data_size,
+                                           ODPP_NONE, include_actions,
+                                           actions, NULL);
+            if (!res) {
+                res = n + 1;
+            }
             goto out;
         }
     }
@@ -7557,15 +7563,18 @@ odp_key_fitness_to_string(enum odp_key_fitness fitness)
 
 /* Appends an OVS_ACTION_ATTR_USERSPACE action to 'odp_actions' that specifies
  * Netlink PID 'pid'.  If 'userdata' is nonnull, adds a userdata attribute
- * whose contents are the 'userdata_size' bytes at 'userdata' and returns the
- * offset within 'odp_actions' of the start of the cookie.  (If 'userdata' is
- * null, then the return value is not meaningful.) */
-size_t
+ * whose contents are the 'userdata_size' bytes at 'userdata' and sets
+ * 'odp_actions_ofs' if nonnull with the offset within 'odp_actions' of the
+ * start of the cookie.  (If 'userdata' is null, then the 'odp_actions_ofs'
+ * value is not meaningful.)
+ *
+ * Returns negative error code on failure. */
+int
 odp_put_userspace_action(uint32_t pid,
                          const void *userdata, size_t userdata_size,
                          odp_port_t tunnel_out_port,
                          bool include_actions,
-                         struct ofpbuf *odp_actions)
+                         struct ofpbuf *odp_actions, size_t *odp_actions_ofs)
 {
     size_t userdata_ofs;
     size_t offset;
@@ -7573,6 +7582,9 @@ odp_put_userspace_action(uint32_t pid,
     offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_USERSPACE);
     nl_msg_put_u32(odp_actions, OVS_USERSPACE_ATTR_PID, pid);
     if (userdata) {
+        if (nl_attr_oversized(userdata_size)) {
+            return -E2BIG;
+        }
         userdata_ofs = odp_actions->size + NLA_HDRLEN;
 
         /* The OVS kernel module before OVS 1.11 and the upstream Linux kernel
@@ -7598,9 +7610,16 @@ odp_put_userspace_action(uint32_t pid,
     if (include_actions) {
         nl_msg_put_flag(odp_actions, OVS_USERSPACE_ATTR_ACTIONS);
     }
+    if (nl_attr_oversized(odp_actions->size - offset - NLA_HDRLEN)) {
+        return -E2BIG;
+    }
     nl_msg_end_nested(odp_actions, offset);
 
-    return userdata_ofs;
+    if (odp_actions_ofs) {
+        *odp_actions_ofs = userdata_ofs;
+    }
+
+    return 0;
 }
 
 void
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 623a66aa2..a1d0d0fba 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -356,11 +356,12 @@ struct user_action_cookie {
 };
 BUILD_ASSERT_DECL(sizeof(struct user_action_cookie) == 48);
 
-size_t odp_put_userspace_action(uint32_t pid,
-                                const void *userdata, size_t userdata_size,
-                                odp_port_t tunnel_out_port,
-                                bool include_actions,
-                                struct ofpbuf *odp_actions);
+int odp_put_userspace_action(uint32_t pid,
+                             const void *userdata, size_t userdata_size,
+                             odp_port_t tunnel_out_port,
+                             bool include_actions,
+                             struct ofpbuf *odp_actions,
+                             size_t *odp_actions_ofs);
 void odp_put_tunnel_action(const struct flow_tnl *tunnel,
                            struct ofpbuf *odp_actions,
                            const char *tnl_type);
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index d79f48aa7..5fae46adf 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1084,7 +1084,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
     }
 
     odp_put_userspace_action(pid, &cookie, sizeof cookie,
-                             ODPP_NONE, false, buf);
+                             ODPP_NONE, false, buf, NULL);
 
     if (meter_id != UINT32_MAX) {
         nl_msg_end_nested(buf, ac_offset);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4ea776052..2715a142b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3223,12 +3223,11 @@ compose_sample_action(struct xlate_ctx *ctx,
     odp_port_t odp_port = ofp_port_to_odp_port(
         ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
     uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
-    size_t cookie_offset = odp_put_userspace_action(pid, cookie,
-                                                    sizeof *cookie,
-                                                    tunnel_out_port,
-                                                    include_actions,
-                                                    ctx->odp_actions);
-
+    size_t cookie_offset;
+    int res = odp_put_userspace_action(pid, cookie, sizeof *cookie,
+                                       tunnel_out_port, include_actions,
+                                       ctx->odp_actions, &cookie_offset);
+    ovs_assert(res == 0);
     if (is_sample) {
         nl_msg_end_nested(ctx->odp_actions, actions_offset);
         nl_msg_end_nested(ctx->odp_actions, sample_offset);
@@ -4832,7 +4831,7 @@ put_controller_user_action(struct xlate_ctx *ctx,
                                              ctx->xin->flow.in_port.ofp_port);
     uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
     odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE,
-                             false, ctx->odp_actions);
+                             false, ctx->odp_actions, NULL);
 }
 
 static void
diff --git a/tests/odp.at b/tests/odp.at
index 1ebdf0515..b762ebb2b 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -398,6 +398,43 @@ odp_actions_from_string: error
 ])
 AT_CLEANUP
 
+AT_SETUP([OVS datapath actions parsing and formatting - userdata overflow])
+dnl Userdata should fit in a single netlink message, i.e. should be less than
+dnl UINT16_MAX - NLA_HDRLEN = 65535 - 4 = 65531 bytes.  OVS should not accept
+dnl larger userdata.  OTOH, userdata is part of a nested netlink message, that
+dnl should not be oversized too.  'pid' takes NLA_HDRLEN + 4 = 8 bytes.
+dnl Plus NLA_HDRLEN for the nested header.  'actions' flag takes NLA_HDRLEN = 4
+dnl and 'tunnel_out_port' takes NLA_HDRLEN + 4 = 8 bytes.
+dnl So, for the variant with 'actions' maximum length of userdata should be:
+dnl UINT16_MAX -  NLA_HDRLEN   - (NLA_HDRLEN + 4) - NLA_HDRLEN - NLA_HDRLEN
+dnl  total max   nested header        pid             actions     userdata
+dnl Result: 65515 bytes for the actual userdata.
+dnl For the case with 'tunnel_out_port': 65511
+dnl Size of userdata will be rounded up to be multiple of 4, so highest
+dnl acceptable sizes are 65512 and 65508.
+
+dnl String with length 65512 * 2 = 131024 is valid, while 131026 is not.
+data_valid=$(  printf '%*s' 131024 | tr ' ' "a")
+data_invalid=$(printf '%*s' 131026 | tr ' ' "a")
+
+echo "userspace(pid=1234567,userdata(${data_valid}),actions)" > actions.txt
+echo "userspace(pid=1234567,userdata(${data_invalid}),actions)" >> actions.txt
+
+dnl String with length 65508 * 2 = 131016 is valid, while 131018 is not.
+data_valid=$(  printf '%*s' 131016 | tr ' ' "a")
+data_invalid=$(printf '%*s' 131018 | tr ' ' "a")
+
+echo "userspace(pid=1234567,userdata(${data_valid}),tunnel_out_port=10)" >> actions.txt
+echo "userspace(pid=1234567,userdata(${data_invalid}),tunnel_out_port=10)" >> actions.txt
+
+AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [dnl
+`cat actions.txt | head -1`
+odp_actions_from_string: error
+`cat actions.txt | head -3 | tail -1`
+odp_actions_from_string: error
+])
+AT_CLEANUP
+
 AT_SETUP([OVS datapath keys parsing and formatting - 33 nested encap ])
 AT_DATA([odp-in.txt], [dnl
 encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap(encap()))))))))))))))))))))))))))))))))