All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net 0/2] net_sched: fix a UAF in tcf_action_init()
@ 2020-09-23  3:56 Cong Wang
  2020-09-23  3:56 ` [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1() Cong Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Cong Wang @ 2020-09-23  3:56 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang

This patchset fixes a use-after-free triggered by syzbot. Please
find more details in each patch description.

---

Cong Wang (2):
  net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  net_sched: commit action insertions together

 include/net/act_api.h      |  2 --
 net/sched/act_api.c        | 52 +++++++++++++++++++++++++-------------
 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, 35 insertions(+), 66 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 10+ messages in thread
* [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  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 ` Cong Wang
  2020-09-25 15:24   ` Vlad Buslov
  2020-09-23  3:56 ` [Patch net 2/2] net_sched: commit action insertions together Cong Wang
  2020-09-25  2:47 ` [Patch net 0/2] net_sched: fix a UAF in tcf_action_init() David Miller
  2 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2020-09-23  3:56 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Vlad Buslov, Jamal Hadi Salim, Jiri Pirko

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>
---
 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);
+	}
+
+	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:
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 54d5652cfe6c..a4c7ba35a343 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -365,9 +365,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (res == ACT_P_CREATED) {
-		tcf_idr_insert(tn, *act);
-	} else {
+	if (res != ACT_P_CREATED) {
 		/* make sure the program being replaced is no longer executing */
 		synchronize_rcu();
 		tcf_bpf_cfg_cleanup(&old);
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index f901421b0634..e19885d7fe2c 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -139,7 +139,6 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
 		ci->net = net;
 		ci->zone = parm->zone;
 
-		tcf_idr_insert(tn, *a);
 		ret = ACT_P_CREATED;
 	} else if (ret > 0) {
 		ci = to_connmark(*a);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index f5826e457679..4fa4fcb842ba 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -110,9 +110,6 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
 	if (params_new)
 		kfree_rcu(params_new, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 2c3619165680..a780afdf570d 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1297,8 +1297,6 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
 		tcf_chain_put_by_act(goto_ch);
 	if (params)
 		call_rcu(&params->rcu, tcf_ct_params_free);
-	if (res == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 
 	return res;
 
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index b5042f3ea079..6084300e51ad 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -269,9 +269,6 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
 	if (cp_new)
 		kfree_rcu(cp_new, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 
 put_chain:
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 410e3bbfb9ca..73c3926358a0 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -140,8 +140,6 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 release_idr:
 	tcf_idr_release(*a, bind);
diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index 1fb8d428d2c1..7c0771dd77a3 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -437,9 +437,6 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 
 chain_put:
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 5c568757643b..a2ddea04183a 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -627,9 +627,6 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
 	if (p)
 		kfree_rcu(p, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 metadata_parse_err:
 	if (goto_ch)
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 400a2cfe8452..8dc3bec0d325 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -189,8 +189,6 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
 	ipt->tcfi_t     = t;
 	ipt->tcfi_hook  = hook;
 	spin_unlock_bh(&ipt->tcf_lock);
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 
 err3:
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index b2705318993b..e24b7e2331cd 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -194,8 +194,6 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
 		spin_lock(&mirred_list_lock);
 		list_add(&m->tcfm_list, &mirred_list);
 		spin_unlock(&mirred_list_lock);
-
-		tcf_idr_insert(tn, *a);
 	}
 
 	return ret;
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index 8118e2640979..e298ec3b3c9e 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -273,8 +273,6 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
 	if (p)
 		kfree_rcu(p, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 855a6fa16a62..1ebd2a86d980 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -93,9 +93,6 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 release_idr:
 	tcf_idr_release(*a, bind);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index c158bfed86d5..b45304446e13 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -238,8 +238,6 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
 	spin_unlock_bh(&p->tcf_lock);
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 
 put_chain:
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 0b431d493768..8d8452b1cdd4 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -201,8 +201,6 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
 	if (new)
 		kfree_rcu(new, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 
 failure:
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 5e2df590bb58..3ebf9ede3cf1 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -116,8 +116,6 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 9813ca4006dd..a4f3d0f0daa9 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -157,8 +157,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
 			goto release_idr;
 	}
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index d0652386c6e2..e5f3fb8b00e3 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -225,8 +225,6 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 39e6d94cfafb..81a1c67335be 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -190,8 +190,6 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 37f1e10f35e0..a229751ee8c4 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -537,9 +537,6 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 	if (goto_ch)
 		tcf_chain_put_by_act(goto_ch);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
-
 	return ret;
 
 put_chain:
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index a5ff9f68ab02..163b0385fd4c 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -229,8 +229,6 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
 	if (p)
 		kfree_rcu(p, rcu);
 
-	if (ret == ACT_P_CREATED)
-		tcf_idr_insert(tn, *a);
 	return ret;
 put_chain:
 	if (goto_ch)
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [Patch net 2/2] net_sched: commit action insertions together
  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-23  3:56 ` 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
  2 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2020-09-23  3:56 UTC (permalink / raw)
  To: netdev
  Cc: Cong Wang, syzbot+2287853d392e4b42374a, Vlad Buslov,
	Jamal Hadi Salim, Jiri Pirko

syzbot is able to trigger a failure case inside the loop in
tcf_action_init(), and when this happens we clean up with
tcf_action_destroy(). But, as these actions are already inserted
into the global IDR, other parallel process could free them
before tcf_action_destroy(), then we will trigger a use-after-free.

Fix this by deferring the insertions even later, after the loop,
and committing all the insertions in a separate loop, so we will
never fail in the middle of the insertions any more.

One side effect is that the window between alloction and final
insertion becomes larger, now it is more likely that the loop in
tcf_del_walker() sees the placeholder -EBUSY pointer. So we have
to check for error pointer in tcf_del_walker().

Reported-and-tested-by: syzbot+2287853d392e4b42374a@syzkaller.appspotmail.com
Fixes: 0190c1d452a9 ("net: sched: atomically check-allocate action")
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>
---
 net/sched/act_api.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 0030f00234ee..104b47f5184f 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -307,6 +307,8 @@ static int tcf_del_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 
 	mutex_lock(&idrinfo->lock);
 	idr_for_each_entry_ul(idr, p, tmp, id) {
+		if (IS_ERR(p))
+			continue;
 		ret = tcf_idr_release_unsafe(p);
 		if (ret == ACT_P_DELETED) {
 			module_put(ops->owner);
@@ -891,14 +893,24 @@ 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)
+static void tcf_idr_insert_many(struct tc_action *actions[])
 {
-	struct tcf_idrinfo *idrinfo = a->idrinfo;
+	int i;
 
-	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);
+	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
+		struct tc_action *a = actions[i];
+		struct tcf_idrinfo *idrinfo;
+
+		if (!a)
+			continue;
+		idrinfo = a->idrinfo;
+		mutex_lock(&idrinfo->lock);
+		/* Replace ERR_PTR(-EBUSY) allocated by tcf_idr_check_alloc if
+		 * it is just created, otherwise this is just a nop.
+		 */
+		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,
@@ -995,9 +1007,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (err == ACT_P_CREATED)
-		tcf_idr_insert(a);
-
 	if (!name && tb[TCA_ACT_COOKIE])
 		tcf_set_action_cookie(&a->act_cookie, cookie);
 
@@ -1053,6 +1062,11 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		actions[i - 1] = act;
 	}
 
+	/* We have to commit them all together, because if any error happened in
+	 * between, we could not handle the failure gracefully.
+	 */
+	tcf_idr_insert_many(actions);
+
 	*attr_size = tcf_action_full_attrs_size(sz);
 	return i - 1;
 
-- 
2.28.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [Patch net 0/2] net_sched: fix a UAF in tcf_action_init()
  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-23  3:56 ` [Patch net 2/2] net_sched: commit action insertions together Cong Wang
@ 2020-09-25  2:47 ` David Miller
  2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2020-09-25  2:47 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 22 Sep 2020 20:56:22 -0700

> This patchset fixes a use-after-free triggered by syzbot. Please
> find more details in each patch description.

Series applied and queued up for -stable, thanks Cong.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  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
  2020-09-25 19:22     ` Cong Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Vlad Buslov @ 2020-09-25 15:24 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, Vlad Buslov, Jamal Hadi Salim, Jiri Pirko

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:

[...]


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 2/2] net_sched: commit action insertions together
  2020-09-23  3:56 ` [Patch net 2/2] net_sched: commit action insertions together Cong Wang
@ 2020-09-25 15:51   ` Vlad Buslov
  0 siblings, 0 replies; 10+ messages in thread
From: Vlad Buslov @ 2020-09-25 15:51 UTC (permalink / raw)
  To: Cong Wang
  Cc: netdev, syzbot+2287853d392e4b42374a, Vlad Buslov,
	Jamal Hadi Salim, Jiri Pirko

On Wed 23 Sep 2020 at 06:56, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> syzbot is able to trigger a failure case inside the loop in
> tcf_action_init(), and when this happens we clean up with
> tcf_action_destroy(). But, as these actions are already inserted
> into the global IDR, other parallel process could free them
> before tcf_action_destroy(), then we will trigger a use-after-free.
>
> Fix this by deferring the insertions even later, after the loop,
> and committing all the insertions in a separate loop, so we will
> never fail in the middle of the insertions any more.
>
> One side effect is that the window between alloction and final
> insertion becomes larger, now it is more likely that the loop in
> tcf_del_walker() sees the placeholder -EBUSY pointer. So we have
> to check for error pointer in tcf_del_walker().
>
> Reported-and-tested-by: syzbot+2287853d392e4b42374a@syzkaller.appspotmail.com
> Fixes: 0190c1d452a9 ("net: sched: atomically check-allocate action")
> 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>
> ---

Reviewed-by: Vlad Buslov <vlad@buslov.dev>

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  2020-09-25 15:24   ` Vlad Buslov
@ 2020-09-25 19:22     ` Cong Wang
  2020-09-25 19:45       ` Vlad Buslov
  0 siblings, 1 reply; 10+ messages in thread
From: Cong Wang @ 2020-09-25 19:22 UTC (permalink / raw)
  To: Vlad Buslov
  Cc: Linux Kernel Network Developers, Vlad Buslov, Jamal Hadi Salim,
	Jiri Pirko

On Fri, Sep 25, 2020 at 8:24 AM Vlad Buslov <vlad@buslov.dev> wrote:
> > +     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.
>

tcf_action_destoy_1() eventually calls free_tcf() which frees cookie and
tcf_action_destroy() which releases module refcnt.

What am I missing here?

Thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  2020-09-25 19:22     ` Cong Wang
@ 2020-09-25 19:45       ` Vlad Buslov
  2020-09-28 10:14         ` Davide Caratti
  0 siblings, 1 reply; 10+ messages in thread
From: Vlad Buslov @ 2020-09-25 19:45 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Vlad Buslov, Jamal Hadi Salim,
	Jiri Pirko

On Fri 25 Sep 2020 at 22:22, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Sep 25, 2020 at 8:24 AM Vlad Buslov <vlad@buslov.dev> wrote:
>> > +     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.
>>
>
> tcf_action_destoy_1() eventually calls free_tcf() which frees cookie and
> tcf_action_destroy() which releases module refcnt.
>
> What am I missing here?
>
> Thanks.

The memory referenced by the function local pointer "cookie" hasn't been
assigned yet to the a->act_cookie because in your patch you moved
goto_chain validation code before the cookie change. That means that if
user overwrites existing action, then action old a->act_cookie will be
freed by tcf_action_destroy_1() but new cookie that was allocated by
nla_memdup_cookie() will leak.

Regards,
Vlad


^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  2020-09-25 19:45       ` Vlad Buslov
@ 2020-09-28 10:14         ` Davide Caratti
  2020-09-28 17:38           ` Cong Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Davide Caratti @ 2020-09-28 10:14 UTC (permalink / raw)
  To: Vlad Buslov, Cong Wang
  Cc: Linux Kernel Network Developers, Vlad Buslov, Jamal Hadi Salim,
	Jiri Pirko

hello,

On Fri, 2020-09-25 at 22:45 +0300, Vlad Buslov wrote:
> On Fri 25 Sep 2020 at 22:22, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Fri, Sep 25, 2020 at 8:24 AM Vlad Buslov <vlad@buslov.dev> wrote:
> > > > +     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.
> > > 
> > 
> > tcf_action_destoy_1() eventually calls free_tcf() which frees cookie and
> > tcf_action_destroy() which releases module refcnt.
> > 
> > What am I missing here?
> > 
> > Thanks.
> 
> The memory referenced by the function local pointer "cookie" hasn't been
> assigned yet to the a->act_cookie because in your patch you moved
> goto_chain validation code before the cookie change. That means that if
> user overwrites existing action, then action old a->act_cookie will be
> freed by tcf_action_destroy_1() but new cookie that was allocated by
> nla_memdup_cookie() will leak.

maybe we can just delete this if (TC_ACT_EXT_CMP(...)) { ... }
statement, instead of moving it? Each TC action already does the check
for NULL "goto chains" with a_o->init() -> tcf_action_check_ctrlact(),
so this if () statement looks dead code to me _ I probably forgot to
remove it after all actions were converted to validate the control
action inside their .init() function.

-- 
davide



^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
  2020-09-28 10:14         ` Davide Caratti
@ 2020-09-28 17:38           ` Cong Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Cong Wang @ 2020-09-28 17:38 UTC (permalink / raw)
  To: Davide Caratti
  Cc: Vlad Buslov, Linux Kernel Network Developers, Vlad Buslov,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Sep 28, 2020 at 3:14 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> hello,
>
> On Fri, 2020-09-25 at 22:45 +0300, Vlad Buslov wrote:
> > On Fri 25 Sep 2020 at 22:22, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Fri, Sep 25, 2020 at 8:24 AM Vlad Buslov <vlad@buslov.dev> wrote:
> > > > > +     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.
> > > >
> > >
> > > tcf_action_destoy_1() eventually calls free_tcf() which frees cookie and
> > > tcf_action_destroy() which releases module refcnt.
> > >
> > > What am I missing here?
> > >
> > > Thanks.
> >
> > The memory referenced by the function local pointer "cookie" hasn't been
> > assigned yet to the a->act_cookie because in your patch you moved
> > goto_chain validation code before the cookie change. That means that if
> > user overwrites existing action, then action old a->act_cookie will be
> > freed by tcf_action_destroy_1() but new cookie that was allocated by
> > nla_memdup_cookie() will leak.

Yes, good catch!


>
> maybe we can just delete this if (TC_ACT_EXT_CMP(...)) { ... }
> statement, instead of moving it? Each TC action already does the check
> for NULL "goto chains" with a_o->init() -> tcf_action_check_ctrlact(),
> so this if () statement looks dead code to me _ I probably forgot to
> remove it after all actions were converted to validate the control
> action inside their .init() function.

Good point, I think you are right, I will send a patch to remove it.

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-28 17:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.