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