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

Flavio Leitner fbl at sysclose.org
Thu Dec 17 17:53:51 UTC 2020


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?

  int
  odp_put_userspace_action(uint32_t pid,
                           const void *userdata, size_t userdata_size,
                           odp_port_t tunnel_out_port,
                           bool include_actions,
                           size_t *offset,  <-- new parameter
                           struct ofpbuf *odp_actions)
  {
  [...]
      if (userdata) {
        if (nl_attr_oversized(userdata_size)) {
            return -E2BIG;
        }
  [...]

      if (offset) {
          *offset = userdata_ofs
      }

      return 0;
  }
        


> >  odp_put_userspace_action(uint32_t pid,
> >                           const void *userdata, size_t userdata_size,
> >                           odp_port_t tunnel_out_port,
> > @@ -7573,6 +7581,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,6 +7609,9 @@ 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;
> > diff --git a/lib/odp-util.h b/lib/odp-util.h
> > index 623a66aa2..46593c411 100644
> > --- a/lib/odp-util.h
> > +++ b/lib/odp-util.h
> > @@ -356,11 +356,11 @@ 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);
> >  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 11aa20754..9171290e0 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3222,12 +3222,12 @@ 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);
> > -
> > +    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);

It seems at this point there is no much else to do because rolling
back would cause OVS to be unreliable, then crashing would be the
best option.


> >      if (is_sample) {
> >          nl_msg_end_nested(ctx->odp_actions, actions_offset);
> >          nl_msg_end_nested(ctx->odp_actions, sample_offset);
> > diff --git a/tests/odp.at b/tests/odp.at
> > index 1ebdf0515..0fa644620 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 pat of a nested netlink message, that
                                             ^^^
typo

> > +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 aceptable 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

works for me.


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

-- 
fbl


More information about the dev mailing list