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 <[email protected]> > >>>>> --- > >>>> > >>>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
