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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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@gmail.com>
> Signed-off-by: Vlad Buslov <vladbu@mellanox.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
>