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