netdev - Re: [PATCH net v4] ovs: limit ovs recursions in ovs_execute

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: <CAOrHB_BCZRG8NinFintTcaRd0MBsYPt-hAH9LG+6uNwOa0GQaA@mail.gmail.com>
Date:	Sat, 16 Jan 2016 09:04:50 -0800
From:	pravin shelar <pshelar@....org>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Simon Horman <simon.horman@...ronome.com>,
	Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net v4] ovs: limit ovs recursions in ovs_execute_actions
 to not corrupt stack

On Fri, Jan 15, 2016 at 8:51 AM, Hannes Frederic Sowa
<hannes@...essinduktion.org> wrote:
> It was seen that defective configurations of openvswitch could overwrite
> the STACK_END_MAGIC and cause a hard crash of the kernel because of too
> many recursions within ovs.
>
> This problem arises due to the high stack usage of openvswitch. The rest
> of the kernel is fine with the current limit of 10 (RECURSION_LIMIT).
>
> We use the already existing recursion counter in ovs_execute_actions to
> implement an upper bound of 5 recursions.
>
> Cc: Pravin Shelar <pshelar@....org>
> Cc: Simon Horman <simon.horman@...ronome.com>
> Cc: Eric Dumazet <eric.dumazet@...il.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@...essinduktion.org>
> Reviewed-by: Simon Horman <simon.horman@...ronome.com>
> ---
> v2) added preemption guards
> v3) Pravin suggested to reuse the ovs_execute_actions counter which this
>     patch does. Also only allow 5 recursions as suggested by Pravin.
> v4) added unlikely as suggested by Eric
>
> Thanks to all reviewers!
>
>  net/openvswitch/actions.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index c88d0f2d3e019b..da66f9e1660dbb 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -1160,17 +1160,28 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                         const struct sw_flow_actions *acts,
>                         struct sw_flow_key *key)
>  {
> -       int level = this_cpu_read(exec_actions_level);
> -       int err;
> +       static const int ovs_recursion_limit = 5;
> +       int err, level;
> +
> +       preempt_disable();

This function always run in soft-irq context. Do we really need this
preempt-disable call here?