| Message ID | 20201123141213.290987-1-i.maximets@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] odp-util: Fix netlink message overflow with userdata. | expand |
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. > > 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> > --- 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 > 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); > 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 > +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 > + > 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())))))))))))))))))))))))))))))))) >
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@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())))))))))))))))))))))))))))))))) > > >
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@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. It's probably better to change type of 'cookie_offset' to int and let it be implicitly converted to size_t on function return. And keep the odp_put_userspace_action() as it is in this patch. The value should be small and it's positive (I added assertion there), so it should not make any harm. At least, everything will fall apart somewhere else anyway if the value if larger than 4 bytes long. What do you think? > > 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. This is just a sanity check. Must not happen. > > >>> 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())))))))))))))))))))))))))))))))) >>> >> >
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@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);
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@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. 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. > > 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); >
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@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
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 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); 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 +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 + 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()))))))))))))))))))))))))))))))))
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. 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> --- 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(-)