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