netdev - Re: [PATCH net-next] net: sched: fix unprotected access to rcu cookie pointer

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]

Date:   Mon, 9 Jul 2018 17:34:17 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Vlad Buslov <vladbu@...lanox.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, jhs@...atatu.com,
        xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net-next] net: sched: fix unprotected access to rcu
 cookie pointer

On Mon, Jul 09, 2018 at 08:26:47PM +0300, Vlad Buslov wrote:
> Fix action attribute size calculation function to take rcu read lock and
> access act_cookie pointer with rcu dereference.
> 
> Fixes: eec94fdb0480 ("net: sched: use rcu for action cookie update")
> Reported-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> Signed-off-by: Vlad Buslov <vladbu@...lanox.com>
> ---
>  net/sched/act_api.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 66dc19746c63..148a89ab789b 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -149,10 +149,15 @@ EXPORT_SYMBOL(__tcf_idr_release);
>  
>  static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
>  {
> +	struct tc_cookie *act_cookie;
>  	u32 cookie_len = 0;
>  
> -	if (act->act_cookie)
> -		cookie_len = nla_total_size(act->act_cookie->len);
> +	rcu_read_lock();
> +	act_cookie = rcu_dereference(act->act_cookie);
> +
> +	if (act_cookie)
> +		cookie_len = nla_total_size(act_cookie->len);
> +	rcu_read_unlock();

I am not sure if this is enough to fix the entire issue. Now it will
fetch the length correctly but, what guarantees that when it tries to
actually copy the key (tcf_action_dump_1), the same act_cookie pointer
will be used? As in, can't the new re-fetch be different/smaller than
the object used here?

>  
>  	return  nla_total_size(0) /* action number nested */
>  		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
> -- 
> 2.7.5
>