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

[Flavio Leitner]

On Fri, Dec 18, 2020 at 11:33:53PM +0100, Ilya Maximets wrote:
> On 12/18/20 9:46 PM, Flavio Leitner wrote:
> > 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: 
> 
> Oh.  Thanks for the example.  Now I see what is your idea here.
> 
> I missed the fact that we're not using the result of odp_put_userspace_action()
> inside parse_odp_userspace_action().  With that I thought that we will need
> to change the type of the 'res' variable and all the way up through the stack.
> But that is not the case.  I see it now.

Cool!

> For the change below, I'll incorporate it, but I'd rather not introduce
> extra function.  I think, It'll be fine if we'll just add an argument to
> the existing one.

Either way works for me.
Thanks,
fbl