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

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

[Vlad Buslov]

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();
 
 	return  nla_total_size(0) /* action number nested */
 		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
-- 
2.7.5

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

[Marcelo Ricardo Leitner]

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
> 

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

[Vlad Buslov]

On Mon 09 Jul 2018 at 20:34, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> 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?

I checked the code of nlmsg_put() and similar functions, and they check
that there is enough free space at skb tailroom. If not, they fail
gracefully and return error. Am I missing something?

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

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

[Eric Dumazet]

On 07/09/2018 01:34 PM, Marcelo Ricardo Leitner wrote:

> 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?
> 

Yes, this presumably should use rtnl_dereference()

RTNL should be held between tcf_action_shared_attrs_size() and tcf_action_dump_1()

Although it might not be the case anymore, we keep changing this RTNL requirement
in dump operations ;)

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

[Marcelo Ricardo Leitner]

On Mon, Jul 09, 2018 at 11:44:38PM +0300, Vlad Buslov wrote:
> 
> On Mon 09 Jul 2018 at 20:34, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> > 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?
> 
> I checked the code of nlmsg_put() and similar functions, and they check
> that there is enough free space at skb tailroom. If not, they fail
> gracefully and return error. Am I missing something?

Talked offline with Vlad and I agree that this is fine as is.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Thanks,
Marcelo

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

[David Miller]

From: Vlad Buslov <vladbu@mellanox.com>
Date: Mon,  9 Jul 2018 20:26:47 +0300

> 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>

Applied.