From: Vlad Buslov <vlad@buslov.dev>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: netdev@vger.kernel.org, Vlad Buslov <vladbu@mellanox.com>,
Jamal Hadi Salim <jhs@mojatatu.com>,
Jiri Pirko <jiri@resnulli.us>
Subject: Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
Date: Fri, 25 Sep 2020 18:24:21 +0300 [thread overview]
Message-ID: <877dsh98wq.fsf@buslov.dev> (raw)
In-Reply-To: <20200923035624.7307-2-xiyou.wangcong@gmail.com>
On Wed 23 Sep 2020 at 06:56, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> All TC actions call tcf_idr_insert() for new action at the end
> of their ->init(), so we can actually move it to a central place
> in tcf_action_init_1().
>
> And once the action is inserted into the global IDR, other parallel
> process could free it immediately as its refcnt is still 1, so we can
> not fail after this, we need to move it after the goto action
> validation to avoid handling the failure case after insertion.
>
> This is found during code review, is not directly triggered by syzbot.
> And this prepares for the next patch.
>
> Cc: Vlad Buslov <vladbu@mellanox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
Hi Cong,
Thanks for fixing this!
> include/net/act_api.h | 2 --
> net/sched/act_api.c | 38 ++++++++++++++++++++------------------
> net/sched/act_bpf.c | 4 +---
> net/sched/act_connmark.c | 1 -
> net/sched/act_csum.c | 3 ---
> net/sched/act_ct.c | 2 --
> net/sched/act_ctinfo.c | 3 ---
> net/sched/act_gact.c | 2 --
> net/sched/act_gate.c | 3 ---
> net/sched/act_ife.c | 3 ---
> net/sched/act_ipt.c | 2 --
> net/sched/act_mirred.c | 2 --
> net/sched/act_mpls.c | 2 --
> net/sched/act_nat.c | 3 ---
> net/sched/act_pedit.c | 2 --
> net/sched/act_police.c | 2 --
> net/sched/act_sample.c | 2 --
> net/sched/act_simple.c | 2 --
> net/sched/act_skbedit.c | 2 --
> net/sched/act_skbmod.c | 2 --
> net/sched/act_tunnel_key.c | 3 ---
> net/sched/act_vlan.c | 2 --
> 22 files changed, 21 insertions(+), 66 deletions(-)
>
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index cb382a89ea58..87214927314a 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -166,8 +166,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
> struct nlattr *est, struct tc_action **a,
> const struct tc_action_ops *ops, int bind,
> u32 flags);
> -void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a);
> -
> void tcf_idr_cleanup(struct tc_action_net *tn, u32 index);
> int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
> struct tc_action **a, int bind);
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 063d8aaf2900..0030f00234ee 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -467,17 +467,6 @@ int tcf_idr_create_from_flags(struct tc_action_net *tn, u32 index,
> }
> EXPORT_SYMBOL(tcf_idr_create_from_flags);
>
> -void tcf_idr_insert(struct tc_action_net *tn, struct tc_action *a)
> -{
> - struct tcf_idrinfo *idrinfo = tn->idrinfo;
> -
> - mutex_lock(&idrinfo->lock);
> - /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
> - WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
> - mutex_unlock(&idrinfo->lock);
> -}
> -EXPORT_SYMBOL(tcf_idr_insert);
> -
> /* Cleanup idr index that was allocated but not initialized. */
>
> void tcf_idr_cleanup(struct tc_action_net *tn, u32 index)
> @@ -902,6 +891,16 @@ static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = {
> [TCA_ACT_HW_STATS] = NLA_POLICY_BITFIELD32(TCA_ACT_HW_STATS_ANY),
> };
>
> +static void tcf_idr_insert(struct tc_action *a)
> +{
> + struct tcf_idrinfo *idrinfo = a->idrinfo;
> +
> + mutex_lock(&idrinfo->lock);
> + /* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc */
> + WARN_ON(!IS_ERR(idr_replace(&idrinfo->action_idr, a, a->tcfa_index)));
> + mutex_unlock(&idrinfo->lock);
> +}
> +
> struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> struct nlattr *nla, struct nlattr *est,
> char *name, int ovr, int bind,
> @@ -989,6 +988,16 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> if (err < 0)
> goto err_mod;
>
> + if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
> + !rcu_access_pointer(a->goto_chain)) {
> + tcf_action_destroy_1(a, bind);
> + NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
> + return ERR_PTR(-EINVAL);
> + }
I don't think calling tcf_action_destoy_1() is enough here. Since you
moved this block before assigning cookie and releasing the module, you
also need to release them manually in addition to destroying the action
instance.
> +
> + if (err == ACT_P_CREATED)
> + tcf_idr_insert(a);
> +
> if (!name && tb[TCA_ACT_COOKIE])
> tcf_set_action_cookie(&a->act_cookie, cookie);
>
> @@ -1002,13 +1011,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> if (err != ACT_P_CREATED)
> module_put(a_o->owner);
>
> - if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
> - !rcu_access_pointer(a->goto_chain)) {
> - tcf_action_destroy_1(a, bind);
> - NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
> - return ERR_PTR(-EINVAL);
> - }
> -
> return a;
>
> err_mod:
[...]
next prev parent reply other threads:[~2020-09-25 15:24 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-23 3:56 [Patch net 0/2] net_sched: fix a UAF in tcf_action_init() Cong Wang
2020-09-23 3:56 ` [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1() Cong Wang
2020-09-25 15:24 ` Vlad Buslov [this message]
2020-09-25 19:22 ` Cong Wang
2020-09-25 19:45 ` Vlad Buslov
2020-09-28 10:14 ` Davide Caratti
2020-09-28 17:38 ` Cong Wang
2020-09-23 3:56 ` [Patch net 2/2] net_sched: commit action insertions together Cong Wang
2020-09-25 15:51 ` Vlad Buslov
2020-09-25 2:47 ` [Patch net 0/2] net_sched: fix a UAF in tcf_action_init() David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877dsh98wq.fsf@buslov.dev \
--to=vlad@buslov.dev \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=netdev@vger.kernel.org \
--cc=vladbu@mellanox.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).