linux-kernel - Re: [ovs-dev] [PATCH net] net: openvswitch: fix leak of nested actions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f7t4k371yej.fsf@redhat.com>
Date:   Tue, 05 Apr 2022 08:56:52 -0400
From:   Aaron Conole <aconole@...hat.com>
To:     Ilya Maximets <i.maximets@....org>
Cc:     netdev@...r.kernel.org, dev@...nvswitch.org,
        linux-kernel@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net] net: openvswitch: fix leak of nested actions

Ilya Maximets <i.maximets@....org> writes:

> While parsing user-provided actions, openvswitch module may dynamically
> allocate memory and store pointers in the internal copy of the actions.
> So this memory has to be freed while destroying the actions.
>
> Currently there are only two such actions: ct() and set().  However,
> there are many actions that can hold nested lists of actions and
> ovs_nla_free_flow_actions() just jumps over them leaking the memory.
>
> For example, removal of the flow with the following actions will lead
> to a leak of the memory allocated by nf_ct_tmpl_alloc():
>
>   actions:clone(ct(commit),0)
>
> Non-freed set() action may also leak the 'dst' structure for the
> tunnel info including device references.
>
> Under certain conditions with a high rate of flow rotation that may
> cause significant memory leak problem (2MB per second in reporter's
> case).  The problem is also hard to mitigate, because the user doesn't
> have direct control over the datapath flows generated by OVS.
>
> Fix that by iterating over all the nested actions and freeing
> everything that needs to be freed recursively.
>
> New build time assertion should protect us from this problem if new
> actions will be added in the future.
>
> Unfortunately, openvswitch module doesn't use NLA_F_NESTED, so all
> attributes has to be explicitly checked.  sample() and clone() actions
> are mixing extra attributes into the user-provided action list.  That
> prevents some code generalization too.
>
> Fixes: 34ae932a4036 ("openvswitch: Make tunnel set action attach a metadata dst")
> Link: https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392922.html
> Reported-by: Stéphane Graber <stgraber@...ntu.com>
> Signed-off-by: Ilya Maximets <i.maximets@....org>
> ---

Acked-by: Aaron Conole <aconole@...hat.com>