[ovs-dev] [PATCH] odp-util: Fix netlink message overflow with userdata.

Flavio Leitner fbl at sysclose.org
Fri Dec 18 20:46:00 UTC 2020


On Fri, Dec 18, 2020 at 06:49:30PM +0100, Ilya Maximets wrote:
> On 12/17/20 6:53 PM, Flavio Leitner wrote:
> > On Thu, Dec 17, 2020 at 01:00:56PM +0100, Ilya Maximets wrote:
> >> On 11/23/20 3:12 PM, Ilya Maximets wrote:
> >>> 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 saize and returning correct error code.
> >                              ^^^^^
> > typo
> > 
> >>>
> >>> 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 at ovn.org>
> >>> ---
> >>
> >> Any thoughts on this?
> >> This change should resove several issues reported by oss-fuzz.
> >>
> >> Best regards, Ilya Maximets.
> >>
> >>>  lib/odp-util.c               | 30 +++++++++++++++++++++--------
> >>>  lib/odp-util.h               | 10 +++++-----
> >>>  ofproto/ofproto-dpif-xlate.c | 12 ++++++------
> >>>  tests/odp.at                 | 37 ++++++++++++++++++++++++++++++++++++
> >>>  4 files changed, 70 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/lib/odp-util.c b/lib/odp-util.c
> >>> index 252a91bfa..879dea97e 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);
> >>> +            if (res >= 0) {
> >>> +                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);
> >>> +            if (res >= 0) {
> >>> +                res = n + 1;
> >>> +            }
> >>>              goto out;
> >>>          }
> >>>      }
> >>> @@ -7559,8 +7565,10 @@ odp_key_fitness_to_string(enum odp_key_fitness fitness)
> >>>   * 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
> >>> + * null, then the return value is not meaningful.)
> >>> + *
> >>> + * Returns negative error code on failure. */
> >>> +int
> > 
> > The type 'size_t' is 8 bytes while 'int' is 4 bytes and this function
> > is returning 'size_t'.  Later 'cookie_offset' is changed to use
> > 'ssize_t' which then becomes 'size_t' again.
> > 
> > Perhaps it would be cleaner if function returns 'int' as 0 if success
> > or negative on error and optionally provide the offset as an size_t
> > pointer passed as argument when that is required?
> 
> I started do that, but all other functions in odp-util.c that calls this
> one are using int as their type and converts size_t to int everywhere.
> OTOH, most of functions in ofproto-dpif-xlate.c are using size_t, but
> at the same time, some of them receives int from odp-util and converts
> them to size_t unconditionally.  This will be much bigger change over
> the codebase in order to clean all of this up.

Could you give one example? I am not seeing the problem.
Just to be sure we are on the same page, I am proposing the
following change on top of yours: 

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 879dea97e..c09827844 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1458,7 +1458,7 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
             res = odp_put_userspace_action(pid, user_data, user_data_size,
                                            tunnel_out_port, include_actions,
                                            actions);
-            if (res >= 0) {
+            if (!res) {
                 res = n + n1;
             }
             goto out;
@@ -1466,7 +1466,7 @@ parse_odp_userspace_action(const char *s, struct ofpbuf *actions)
             res = odp_put_userspace_action(pid, user_data, user_data_size,
                                            ODPP_NONE, include_actions,
                                            actions);
-            if (res >= 0) {
+            if (!res) {
                 res = n + 1;
             }
             goto out;
@@ -7563,17 +7563,19 @@ 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.)
+ * 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)
+odp_put_userspace_action_ofs(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)
 {
     size_t userdata_ofs;
     size_t offset;
@@ -7614,7 +7616,28 @@ odp_put_userspace_action(uint32_t pid,
     }
     nl_msg_end_nested(odp_actions, offset);
 
-    return userdata_ofs;
+    if (odp_actions_ofs) {
+        *odp_actions_ofs = userdata_ofs;
+    }
+
+    return 0;
+}
+
+/* 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'.
+ *
+ * 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)
+{
+    return odp_put_userspace_action_ofs(pid, userdata, userdata_size,
+                                        tunnel_out_port, include_actions,
+                                        odp_actions, NULL);
 }
 
 void
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 46593c411..a53f2a512 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -361,6 +361,12 @@ int odp_put_userspace_action(uint32_t pid,
                              odp_port_t tunnel_out_port,
                              bool include_actions,
                              struct ofpbuf *odp_actions);
+int odp_put_userspace_action_ofs(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-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9171290e0..64fca6e7a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3222,12 +3222,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);
-    ssize_t cookie_offset = odp_put_userspace_action(pid, cookie,
-                                                     sizeof *cookie,
-                                                     tunnel_out_port,
-                                                     include_actions,
-                                                     ctx->odp_actions);
-    ovs_assert(cookie_offset >= 0);
+    size_t cookie_offset;
+    int ret = odp_put_userspace_action_ofs(pid, cookie, sizeof *cookie,
+                                           tunnel_out_port, include_actions,
+                                           ctx->odp_actions, &cookie_offset);
+    ovs_assert(ret == 0);
     if (is_sample) {
         nl_msg_end_nested(ctx->odp_actions, actions_offset);
         nl_msg_end_nested(ctx->odp_actions, sample_offset);
-- 
2.28.0






More information about the dev mailing list