Skip to content

Commit

Permalink
odp-util: Fix netlink message overflow with userdata.
Browse files Browse the repository at this point in the history 
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 size and returning correct error code.

Credit to OSS-Fuzz.

Reported-at: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=27640
Fixes: e995e3d ("Allow OVS_USERSPACE_ATTR_USERDATA to be variable length.")
Signed-off-by: Ilya Maximets <[email protected]>
Acked-by: Flavio Leitner <[email protected]>
  • Loading branch information
@igsilya
igsilya committed Dec 21, 2020
1 parent c5b4b0c commit 55f2b06
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 25 deletions.
43 changes: 31 additions & 12 deletions lib/odp-util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, NULL);
if (!res) {
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, NULL);
if (!res) {
res = n + 1;
}
goto out;
}
}
Expand Down Expand Up @@ -7557,22 +7563,28 @@ 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.) */
size_t
* 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)
struct ofpbuf *odp_actions, size_t *odp_actions_ofs)
{
size_t userdata_ofs;
size_t offset;

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
Expand All @@ -7598,9 +7610,16 @@ 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;
if (odp_actions_ofs) {
*odp_actions_ofs = userdata_ofs;
}

return 0;
}

void
Expand Down
11 changes: 6 additions & 5 deletions lib/odp-util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,12 @@ 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,
size_t *odp_actions_ofs);
void odp_put_tunnel_action(const struct flow_tnl *tunnel,
struct ofpbuf *odp_actions,
const char *tnl_type);
Expand Down
2 changes: 1 addition & 1 deletion ofproto/ofproto-dpif-upcall.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout,
}

odp_put_userspace_action(pid, &cookie, sizeof cookie,
ODPP_NONE, false, buf);
ODPP_NONE, false, buf, NULL);

if (meter_id != UINT32_MAX) {
nl_msg_end_nested(buf, ac_offset);
Expand Down
13 changes: 6 additions & 7 deletions ofproto/ofproto-dpif-xlate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3223,12 +3223,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);
size_t cookie_offset = odp_put_userspace_action(pid, cookie,
sizeof *cookie,
tunnel_out_port,
include_actions,
ctx->odp_actions);

size_t cookie_offset;
int res = odp_put_userspace_action(pid, cookie, sizeof *cookie,
tunnel_out_port, include_actions,
ctx->odp_actions, &cookie_offset);
ovs_assert(res == 0);
if (is_sample) {
nl_msg_end_nested(ctx->odp_actions, actions_offset);
nl_msg_end_nested(ctx->odp_actions, sample_offset);
Expand Down Expand Up @@ -4832,7 +4831,7 @@ put_controller_user_action(struct xlate_ctx *ctx,
ctx->xin->flow.in_port.ofp_port);
uint32_t pid = dpif_port_get_pid(ctx->xbridge->dpif, odp_port);
odp_put_userspace_action(pid, &cookie, sizeof cookie, ODPP_NONE,
false, ctx->odp_actions);
false, ctx->odp_actions, NULL);
}

static void
Expand Down
37 changes: 37 additions & 0 deletions tests/odp.at
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 part 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 acceptable 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()))))))))))))))))))))))))))))))))
Expand Down

0 comments on commit 55f2b06

Please sign in to comment.