From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH net-next] net: sched: fix unprotected access to rcu cookie pointer Date: Wed, 11 Jul 2018 23:29:34 -0300 Message-ID: <20180712022934.GG8880@localhost.localdomain> References: <1531157207-10850-1-git-send-email-vladbu@mellanox.com> <20180709203415.GB10923@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, davem@davemloft.net, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us To: Vlad Buslov Return-path: Received: from mail-qt0-f194.google.com ([209.85.216.194]:43312 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732994AbeGLCgw (ORCPT ); Wed, 11 Jul 2018 22:36:52 -0400 Received: by mail-qt0-f194.google.com with SMTP id f18-v6so22908612qtp.10 for ; Wed, 11 Jul 2018 19:29:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Jul 09, 2018 at 11:44:38PM +0300, Vlad Buslov wrote: > > On Mon 09 Jul 2018 at 20:34, Marcelo Ricardo Leitner 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 > >> Signed-off-by: Vlad Buslov > >> --- > >> 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 Thanks, Marcelo