* [PATCH net 00/16] validate the control action with all the other parameters
@ 2019-02-27 17:40 Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
` (15 more replies)
0 siblings, 16 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
currently, the kernel checks for bad values of the control action in
tcf_action_init_1(), after a successful call to the action's init()
function. This causes three bad behaviors:
1. the "half configuration"
if the action is overwritten, the new configuration data are
applied successfully - but the value of the control action remains
the previous one.
2. the "refcount leak that makes kmemleak complain"
when a valid 'goto chain' action is overwritten with another 'goto chain'
action, the kernel leaks two refcounts.
3. the "kernel crash in the traffic plane"
when an action is overwritten with an invalid 'goto chain' action,
packets hitting the new rule will trigger a NULL pointer dereference.
all the above three problems can be fixed if we validate the control
action in each action's init() handler, the same way as we are already
doing for all the other parameters.
Changes since RFC:
- include a fix for all TC actions
- add a selftest for each TC action
- squash fix for refcount leaks into a single patch, the first in the
series, thanks to Cong Wang
- ensure that chain refcount is released without tcfa_lock held, thanks
to Vlad Buslov
Notes:
- act_ipt didn't need any fix, as the control action is constantly equal
to TC_ACT_OK.
- the selftest for act_simple fails because userspace tc backend for
'simple' does not parse the control action correctly (and hardcodes it
to TC_ACT_PIPE).
Davide Caratti (16):
net/sched: prepare TC actions to properly validate the control action
net/sched: act_bpf: validate the control action inside init()
net/sched: act_csum: validate the control action inside init()
net/sched: act_gact: validate the control action inside init()
net/sched: act_ife: validate the control action inside init()
net/sched: act_mirred: validate the control action inside init()
net/sched: act_connmark: validate the control action inside init()
net/sched: act_nat: validate the control action inside init()
net/sched: act_pedit: validate the control action inside init()
net/sched: act_police: validate the control action inside init()
net/sched: act_sample: validate the control action inside init()
net/sched: act_simple: validate the control action inside init()
net/sched: act_skbedit: validate the control action inside init()
net/sched: act_skbmod: validate the control action inside init()
net/sched: act_tunnel_key: validate the control action inside init()
net/sched: act_vlan: validate the control action inside init()
include/net/act_api.h | 7 +-
net/sched/act_api.c | 84 +++++++++++--------
net/sched/act_bpf.c | 15 +++-
net/sched/act_connmark.c | 25 +++++-
net/sched/act_csum.c | 23 ++++-
net/sched/act_gact.c | 15 +++-
net/sched/act_ife.c | 34 +++++---
net/sched/act_ipt.c | 11 +--
net/sched/act_mirred.c | 24 +++++-
net/sched/act_nat.c | 15 +++-
net/sched/act_pedit.c | 19 ++++-
net/sched/act_police.c | 13 +++
net/sched/act_sample.c | 22 ++++-
net/sched/act_simple.c | 52 +++++++++---
net/sched/act_skbedit.c | 23 ++++-
net/sched/act_skbmod.c | 27 ++++--
net/sched/act_tunnel_key.c | 19 ++++-
net/sched/act_vlan.c | 23 ++++-
.../tc-testing/tc-tests/actions/bpf.json | 25 ++++++
.../tc-testing/tc-tests/actions/connmark.json | 25 ++++++
.../tc-testing/tc-tests/actions/csum.json | 25 ++++++
.../tc-testing/tc-tests/actions/gact.json | 25 ++++++
.../tc-testing/tc-tests/actions/ife.json | 25 ++++++
.../tc-testing/tc-tests/actions/mirred.json | 25 ++++++
.../tc-testing/tc-tests/actions/nat.json | 25 ++++++
.../tc-testing/tc-tests/actions/pedit.json | 51 +++++++++++
.../tc-testing/tc-tests/actions/police.json | 25 ++++++
.../tc-testing/tc-tests/actions/sample.json | 25 ++++++
.../tc-testing/tc-tests/actions/simple.json | 25 ++++++
.../tc-testing/tc-tests/actions/skbedit.json | 25 ++++++
.../tc-testing/tc-tests/actions/skbmod.json | 25 ++++++
.../tc-tests/actions/tunnel_key.json | 25 ++++++
.../tc-testing/tc-tests/actions/vlan.json | 25 ++++++
33 files changed, 757 insertions(+), 95 deletions(-)
create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
--
2.20.1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-28 1:28 ` Cong Wang
2019-02-27 17:40 ` [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init() Davide Caratti
` (14 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
- add tcf_action_check_ctrlact(), and pass a pointer to struct tcf_proto
in each actions's init() handler, to allow validation of 'goto chain'
control action.
- remove code that validates the control action after a successful call
to the action's init() handler, and replace it with a test that forbids
addition of actions having 'goto_chain' and no chain handle at the same
time.
This disallows 'goto_chain' on actions that don't initialize it properly
in their init() handler, i.e. calling tcf_action_check_ctrlact() after
successful IDR reservation and then assigning 'tcf_goto_chain' and
'tcf_action' consistently.
By doing this, the kernel does not leak anymore refcounts when a valid
'goto chain' handle is replaced in TC actions, causing kmemleak splats
like the following one:
# tc chain add dev dd0 chain 42 ingress protocol ip flower \
> ip_proto tcp action drop
# tc chain add dev dd0 chain 43 ingress protocol ip flower \
> ip_proto udp action drop
# tc filter add dev dd0 ingress matchall \
> action gact goto chain 42 index 66
# tc filter replace dev dd0 ingress matchall \
> action gact goto chain 43 index 66
# echo scan >/sys/kernel/debug/kmemleak
<...>
unreferenced object 0xffff93c0ee09f000 (size 1024):
comm "tc", pid 2565, jiffies 4295339808 (age 65.426s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 08 00 06 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<000000009b63f92d>] tc_ctl_chain+0x3d2/0x4c0
[<00000000683a8d72>] rtnetlink_rcv_msg+0x263/0x2d0
[<00000000ddd88f8e>] netlink_rcv_skb+0x4a/0x110
[<000000006126a348>] netlink_unicast+0x1a0/0x250
[<00000000b3340877>] netlink_sendmsg+0x2c1/0x3c0
[<00000000a25a2171>] sock_sendmsg+0x36/0x40
[<00000000f19ee1ec>] ___sys_sendmsg+0x280/0x2f0
[<00000000d0422042>] __sys_sendmsg+0x5e/0xa0
[<000000007a6c61f9>] do_syscall_64+0x5b/0x180
[<00000000ccd07542>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0000000013eaa334>] 0xffffffffffffffff
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
include/net/act_api.h | 7 +++-
net/sched/act_api.c | 84 ++++++++++++++++++++++----------------
net/sched/act_bpf.c | 2 +-
net/sched/act_connmark.c | 1 +
net/sched/act_csum.c | 2 +-
net/sched/act_gact.c | 2 +-
net/sched/act_ife.c | 2 +-
net/sched/act_ipt.c | 11 ++---
net/sched/act_mirred.c | 1 +
net/sched/act_nat.c | 3 +-
net/sched/act_pedit.c | 2 +-
net/sched/act_police.c | 1 +
net/sched/act_sample.c | 2 +-
net/sched/act_simple.c | 2 +-
net/sched/act_skbedit.c | 1 +
net/sched/act_skbmod.c | 1 +
net/sched/act_tunnel_key.c | 1 +
net/sched/act_vlan.c | 2 +-
18 files changed, 77 insertions(+), 50 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index dbc795ec659e..f21830b7753a 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -50,6 +50,7 @@ struct tc_action {
#define tcf_qstats common.tcfa_qstats
#define tcf_rate_est common.tcfa_rate_est
#define tcf_lock common.tcfa_lock
+#define tcf_goto_chain common.goto_chain
/* Update lastuse only if needed, to avoid dirtying a cache line.
* We use a temp variable to avoid fetching jiffies twice.
@@ -90,7 +91,7 @@ struct tc_action_ops {
int (*lookup)(struct net *net, struct tc_action **a, u32 index);
int (*init)(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **act, int ovr,
- int bind, bool rtnl_held,
+ int bind, bool rtnl_held, struct tcf_proto *tp,
struct netlink_ext_ack *extack);
int (*walk)(struct net *, struct sk_buff *,
struct netlink_callback *, int,
@@ -181,6 +182,10 @@ int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
int tcf_action_copy_stats(struct sk_buff *, struct tc_action *, int);
+int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
+ struct tcf_chain **handle,
+ struct netlink_ext_ack *extack);
+
#endif /* CONFIG_NET_CLS_ACT */
static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d4b8355737d8..ae44eaa85df8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -28,18 +28,6 @@
#include <net/act_api.h>
#include <net/netlink.h>
-static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp)
-{
- u32 chain_index = a->tcfa_action & TC_ACT_EXT_VAL_MASK;
-
- if (!tp)
- return -EINVAL;
- a->goto_chain = tcf_chain_get_by_act(tp->chain->block, chain_index);
- if (!a->goto_chain)
- return -ENOMEM;
- return 0;
-}
-
static void tcf_action_goto_chain_fini(struct tc_action *a)
{
tcf_chain_put_by_act(a->goto_chain);
@@ -71,6 +59,48 @@ static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie,
call_rcu(&old->rcu, tcf_free_cookie_rcu);
}
+int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
+ struct tcf_chain **handle,
+ struct netlink_ext_ack *extack)
+{
+ int opcode = TC_ACT_EXT_OPCODE(action), ret = -EINVAL;
+ u32 chain_index;
+
+ if (!opcode)
+ ret = action > TC_ACT_VALUE_MAX ? -EINVAL : 0;
+ else if (opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC)
+ ret = 0;
+ if (ret) {
+ NL_SET_ERR_MSG(extack, "invalid control action");
+ goto end;
+ }
+
+ if (TC_ACT_EXT_CMP(action, TC_ACT_GOTO_CHAIN)) {
+ chain_index = action & TC_ACT_EXT_VAL_MASK;
+ if (!tp) {
+ ret = -EINVAL;
+ NL_SET_ERR_MSG(extack,
+ "can't use goto_chain with NULL proto");
+ goto end;
+ }
+ if (!handle) {
+ ret = -EINVAL;
+ NL_SET_ERR_MSG(extack,
+ "can't put goto_chain on NULL handle");
+ goto end;
+ }
+ *handle = tcf_chain_get_by_act(tp->chain->block, chain_index);
+ if (!*handle) {
+ ret = -ENOMEM;
+ NL_SET_ERR_MSG(extack,
+ "can't allocate goto_chain handle");
+ }
+ }
+end:
+ return ret;
+}
+EXPORT_SYMBOL(tcf_action_check_ctrlact);
+
/* XXX: For standalone actions, we don't need a RCU grace period either, because
* actions are always connected to filters and filters are already destroyed in
* RCU callbacks, so after a RCU grace period actions are already disconnected
@@ -800,15 +830,6 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
return c;
}
-static bool tcf_action_valid(int action)
-{
- int opcode = TC_ACT_EXT_OPCODE(action);
-
- if (!opcode)
- return action <= TC_ACT_VALUE_MAX;
- return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
-}
-
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,
@@ -890,10 +911,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
/* backward compatibility for policer */
if (name == NULL)
err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, ovr, bind,
- rtnl_held, extack);
+ rtnl_held, tp, extack);
else
err = a_o->init(net, nla, est, &a, ovr, bind, rtnl_held,
- extack);
+ tp, extack);
if (err < 0)
goto err_mod;
@@ -907,19 +928,12 @@ 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)) {
- err = tcf_action_goto_chain_init(a, tp);
- if (err) {
- tcf_action_destroy_1(a, bind);
- NL_SET_ERR_MSG(extack, "Failed to init TC action chain");
- return ERR_PTR(err);
- }
- }
-
- if (!tcf_action_valid(a->tcfa_action)) {
+ if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
+ !a->goto_chain) {
+ NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
tcf_action_destroy_1(a, bind);
- NL_SET_ERR_MSG(extack, "Invalid control action value");
- return ERR_PTR(-EINVAL);
+ err = -EINVAL;
+ return ERR_PTR(err);
}
return a;
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index c7633843e223..88a729bdab25 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -278,7 +278,7 @@ static void tcf_bpf_prog_fill_cfg(const struct tcf_bpf *prog,
static int tcf_bpf_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **act,
int replace, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, bpf_net_id);
struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 8475913f2070..30c4c109c80c 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -97,6 +97,7 @@ static const struct nla_policy connmark_policy[TCA_CONNMARK_MAX + 1] = {
static int tcf_connmark_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, connmark_net_id);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 3dc25b7806d7..1ae120c9ab02 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -46,7 +46,7 @@ static struct tc_action_ops act_csum_ops;
static int tcf_csum_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, bool rtnl_held,
+ int bind, bool rtnl_held, struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, csum_net_id);
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index b61c20ebb314..727bbca9534b 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -57,7 +57,7 @@ static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
static int tcf_gact_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, gact_net_id);
struct nlattr *tb[TCA_GACT_MAX + 1];
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 30b63fa23ee2..9b2eb941e093 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -469,7 +469,7 @@ static int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb,
static int tcf_ife_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, ife_net_id);
struct nlattr *tb[TCA_IFE_MAX + 1];
diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index faa1addf89b3..13998231bed5 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c
@@ -97,7 +97,8 @@ static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
- const struct tc_action_ops *ops, int ovr, int bind)
+ const struct tc_action_ops *ops, int ovr, int bind,
+ struct tcf_proto *tp)
{
struct tc_action_net *tn = net_generic(net, id);
struct nlattr *tb[TCA_IPT_MAX + 1];
@@ -205,20 +206,20 @@ static int __tcf_ipt_init(struct net *net, unsigned int id, struct nlattr *nla,
static int tcf_ipt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, bool rtnl_held,
+ int bind, bool rtnl_held, struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
return __tcf_ipt_init(net, ipt_net_id, nla, est, a, &act_ipt_ops, ovr,
- bind);
+ bind, tp);
}
static int tcf_xt_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, bool unlocked,
+ int bind, bool unlocked, struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
return __tcf_ipt_init(net, xt_net_id, nla, est, a, &act_xt_ops, ovr,
- bind);
+ bind, tp);
}
static int tcf_ipt_act(struct sk_buff *skb, const struct tc_action *a,
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index c8cf4d10c435..69dda57f1097 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -94,6 +94,7 @@ static struct tc_action_ops act_mirred_ops;
static int tcf_mirred_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, mirred_net_id);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index c5c1e23add77..526c4c99bcce 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -38,7 +38,8 @@ static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
struct tc_action **a, int ovr, int bind,
- bool rtnl_held, struct netlink_ext_ack *extack)
+ bool rtnl_held, struct tcf_proto *tp,
+ struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, nat_net_id);
struct nlattr *tb[TCA_NAT_MAX + 1];
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 2b372a06b432..1c7a0db7b466 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -138,7 +138,7 @@ static int tcf_pedit_key_ex_dump(struct sk_buff *skb,
static int tcf_pedit_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, pedit_net_id);
struct nlattr *tb[TCA_PEDIT_MAX + 1];
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index ec8ec55e0fe8..a444dd78a244 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -83,6 +83,7 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
static int tcf_police_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
int ret = 0, tcfp_result = TC_ACT_OK, err, size;
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 1a0c682fd734..b2154edcb535 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -37,7 +37,7 @@ static const struct nla_policy sample_policy[TCA_SAMPLE_MAX + 1] = {
static int tcf_sample_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a, int ovr,
- int bind, bool rtnl_held,
+ int bind, bool rtnl_held, struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, sample_net_id);
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 902957beceb3..640ee5b785dc 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -80,7 +80,7 @@ static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
static int tcf_simp_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, simp_net_id);
struct nlattr *tb[TCA_DEF_MAX + 1];
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index cfceed28c333..9a8a0f2d4418 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -96,6 +96,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbedit_net_id);
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 59710a183bd3..35572d0e4576 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -82,6 +82,7 @@ static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbmod_net_id);
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 3f943de9a2c9..eaa4a9c80898 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -209,6 +209,7 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
static int tunnel_key_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
+ struct tcf_proto *tp,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 93fdaf707313..80fd0e238a10 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -105,7 +105,7 @@ static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
static int tcf_vlan_init(struct net *net, struct nlattr *nla,
struct nlattr *est, struct tc_action **a,
int ovr, int bind, bool rtnl_held,
- struct netlink_ext_ack *extack)
+ struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, vlan_net_id);
struct nlattr *tb[TCA_VLAN_MAX + 1];
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
` (13 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc filter add dev crash0 egress matchall \
> action bpf bytecode '1,6 0 0 4294967295' pass index 90
# tc actions replace action bpf \
> bytecode '1,6 0 0 4294967295' goto chain 42 index 90 cookie c1a0c1a0
# tc action show action bpf
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: bpf bytecode '1,6 0 0 4294967295' default-action goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffb3a0803dfa90 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff942b347ada00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffb3a08034d038 RDI: ffff942b347ada00
RBP: ffffb3a0803dfb30 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: ffffb3a0803dfb0c R12: ffff942b3b682b00
R13: ffff942b3b682b08 R14: 0000000000000001 R15: ffff942b3b682f00
FS: 00007f6160a72740(0000) GS:ffff942b3da80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000795a4002 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip_finish_output2+0x16f/0x430
ip_finish_output2+0x16f/0x430
? ip_output+0x69/0xe0
ip_output+0x69/0xe0
? ip_forward_options+0x1a0/0x1a0
ip_send_skb+0x15/0x40
raw_sendmsg+0x8e1/0xbd0
? sched_clock+0x5/0x10
? sched_clock_cpu+0xc/0xa0
? try_to_wake_up+0x54/0x480
? ldsem_down_read+0x3f/0x280
? _cond_resched+0x15/0x40
? down_read+0xe/0x30
? copy_termios+0x1e/0x70
? tty_mode_ioctl+0x1b6/0x4c0
? sock_sendmsg+0x36/0x40
sock_sendmsg+0x36/0x40
__sys_sendto+0x10e/0x140
? do_vfs_ioctl+0xa4/0x640
? handle_mm_fault+0xdc/0x210
? syscall_trace_enter+0x1df/0x2e0
? __audit_syscall_exit+0x216/0x260
__x64_sys_sendto+0x24/0x30
do_syscall_64+0x5b/0x180
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f615f7e3c03
Code: 48 8b 0d 90 62 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 83 3d 9d c3 2c 00 00 75 13 49 89 ca b8 2c 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 34 c3 48 83 ec 08 e8 4b cc 00 00 48 89 04 24
RSP: 002b:00007ffee5d8cc28 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 000055a4f28f1700 RCX: 00007f615f7e3c03
RDX: 0000000000000040 RSI: 000055a4f28f1700 RDI: 0000000000000003
RBP: 00007ffee5d8e340 R08: 000055a4f28ee510 R09: 0000000000000010
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000040
R13: 000055a4f28f16c0 R14: 000055a4f28ef69c R15: 0000000000000080
Modules linked in: act_bpf veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 mbcache crct10dif_pclmul jbd2 crc32_pclmul snd_hda_codec_generic ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper pcspkr joydev virtio_balloon snd_timer snd i2c_piix4 soundcore nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper virtio_blk virtio_net virtio_console net_failover failover syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm crc32c_intel ata_piix serio_raw libata virtio_pci virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_bpf_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_bpf.c | 13 ++++++++++
.../tc-testing/tc-tests/actions/bpf.json | 25 +++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 88a729bdab25..9aa24decffc4 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -17,6 +17,7 @@
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_bpf.h>
#include <net/tc_act/tc_bpf.h>
@@ -281,6 +282,7 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, bpf_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
struct tcf_bpf_cfg cfg, old;
struct tc_act_bpf *parm;
@@ -323,6 +325,10 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
return ret;
}
+ ret = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (ret < 0)
+ goto release_idr;
+
is_bpf = tb[TCA_ACT_BPF_OPS_LEN] && tb[TCA_ACT_BPF_OPS];
is_ebpf = tb[TCA_ACT_BPF_FD];
@@ -350,6 +356,8 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
if (cfg.bpf_num_ops)
prog->bpf_num_ops = cfg.bpf_num_ops;
+ oldchain = prog->tcf_goto_chain;
+ prog->tcf_goto_chain = newchain;
prog->tcf_action = parm->action;
rcu_assign_pointer(prog->filter, cfg.filter);
spin_unlock_bh(&prog->tcf_lock);
@@ -361,9 +369,14 @@ static int tcf_bpf_init(struct net *net, struct nlattr *nla,
synchronize_rcu();
tcf_bpf_cfg_cleanup(&old);
}
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return res;
out:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
tcf_idr_release(*act, bind);
return ret;
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
index 5970cee6d05f..b074ea9b6fe8 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/bpf.json
@@ -286,5 +286,30 @@
"teardown": [
"$TC action flush action bpf"
]
+ },
+ {
+ "id": "b8a1",
+ "name": "Replace bpf action with invalid goto_chain control",
+ "category": [
+ "actions",
+ "bpf"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action bpf",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC action add action bpf bytecode '1,6 0 0 4294967295' pass index 90"
+ ],
+ "cmdUnderTest": "$TC action replace action bpf bytecode '1,6 0 0 4294967295' goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC action list action bpf",
+ "matchPattern": "action order [0-9]*: bpf.* default-action pass.*index 90",
+ "matchCount": "1",
+ "teardown": [
+ "$TC action flush action bpf"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
2019-02-27 17:40 ` [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init() Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-28 1:50 ` Cong Wang
2019-02-27 17:40 ` [PATCH net 04/16] net/sched: act_gact: " Davide Caratti
` (12 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall action csum icmp pass index 90
# tc actions replace action csum icmp goto chain 42 index 90 \
> cookie c1a0c1a0
# tc actions show action csum
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: csum (icmp) action goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 8000000074692067 P4D 8000000074692067 PUD 2e210067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff93153da03be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9314ee40f700 RCX: 0000000000003a00
RDX: 0000000000000000 RSI: ffff931537c87828 RDI: ffff931537c87818
RBP: ffff93153da03c80 R08: 00000000527cffff R09: 0000000000000003
R10: 000000000000003f R11: 0000000000000028 R12: ffff9314edf68400
R13: ffff9314edf68408 R14: 0000000000000001 R15: ffff9314ed67b600
FS: 0000000000000000(0000) GS:ffff93153da00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000073e32003 CR4: 00000000001606f0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? tick_sched_timer+0x37/0x70
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 66 ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffffffff9a803e98 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffff99e184f0 RBX: 0000000000000000 RCX: 0000000000000001
RDX: 0000000000000001 RSI: 0000000000000087 RDI: 0000000000000000
RBP: 0000000000000000 R08: 000eb5c4572376b3 R09: 0000000000000000
R10: ffffa53e806a3ca0 R11: 00000000000f4240 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_kernel+0x49e/0x4be
secondary_startup_64+0xa4/0xb0
Modules linked in: act_csum veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul crc32_pclmul snd_hda_codec_generic ghash_clmulni_intel snd_hda_intel mbcache snd_hda_codec jbd2 snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd snd_timer glue_helper snd joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect virtio_net sysimgblt net_failover fb_sys_fops virtio_console virtio_blk ttm failover drm ata_piix crc32c_intel floppy virtio_pci serio_raw libata virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_csum_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_csum.c | 21 ++++++++++++++--
.../tc-testing/tc-tests/actions/csum.json | 25 +++++++++++++++++++
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 1ae120c9ab02..34c0f30dbc83 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -33,6 +33,7 @@
#include <net/sctp/checksum.h>
#include <net/act_api.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_csum.h>
#include <net/tc_act/tc_csum.h>
@@ -50,6 +51,7 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, csum_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct tcf_csum_params *params_new;
struct nlattr *tb[TCA_CSUM_MAX + 1];
struct tc_csum *parm;
@@ -87,16 +89,22 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
return err;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
+
p = to_tcf_csum(*a);
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
if (unlikely(!params_new)) {
- tcf_idr_release(*a, bind);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto put_chain;
}
params_new->update_flags = parm->update_flags;
spin_lock_bh(&p->tcf_lock);
+ oldchain = p->tcf_goto_chain;
+ p->tcf_goto_chain = newchain;
p->tcf_action = parm->action;
rcu_swap_protected(p->params, params_new,
lockdep_is_held(&p->tcf_lock));
@@ -108,7 +116,16 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
+
return ret;
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
/**
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/csum.json b/tools/testing/selftests/tc-testing/tc-tests/actions/csum.json
index a022792d392a..ddabb2fbb7c7 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/csum.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/csum.json
@@ -500,5 +500,30 @@
"matchPattern": "^[ \t]+index [0-9]+ ref",
"matchCount": "0",
"teardown": []
+ },
+ {
+ "id": "d128",
+ "name": "Replace csum action with invalid goto chain control",
+ "category": [
+ "actions",
+ "csum"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action csum",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action csum iph index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action csum iph goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action csum index 90",
+ "matchPattern": "action order [0-9]*: csum \\(iph\\) action pass.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action csum"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 04/16] net/sched: act_gact: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (2 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
` (11 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action gact pass index 90
# tc actions replace action gact \
> goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action gact
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: gact action goto chain 42
random type none pass val 0
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff8c2434703be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff8c23ed6d7e00 RCX: 000000000000005a
RDX: 0000000000000000 RSI: 0000000000000001 RDI: ffff8c23ed6d7e00
RBP: ffff8c2434703c80 R08: ffff8c243b639ac8 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8c2429e68b00
R13: ffff8c2429e68b08 R14: 0000000000000001 R15: ffff8c2429c5a480
FS: 0000000000000000(0000) GS:ffff8c2434700000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000002dc0e005 CR4: 00000000001606e0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? tick_sched_timer+0x37/0x70
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 74 ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffff9c8640387eb8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffff8b2184f0 RBX: 0000000000000002 RCX: 0000000000000001
RDX: 0000000000000001 RSI: 0000000000000087 RDI: 0000000000000002
RBP: 0000000000000002 R08: 000eb57882b36cc3 R09: 0000000000000020
R10: 0000000000000004 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_secondary+0x1a7/0x200
secondary_startup_64+0xa4/0xb0
Modules linked in: act_gact act_bpf veth ip6table_filter ip6_tables iptable_filter binfmt_misc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec_generic ext4 snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core mbcache jbd2 snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper virtio_balloon joydev pcspkr snd_timer snd i2c_piix4 soundcore nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea virtio_net sysfillrect net_failover virtio_blk sysimgblt fb_sys_fops virtio_console ttm failover drm crc32c_intel serio_raw ata_piix libata floppy virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod [last unloaded: act_bpf]
CR2: 0000000000000000
Validating the control action within tcf_gact_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_gact.c | 13 ++++++++++
.../tc-testing/tc-tests/actions/gact.json | 25 +++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 727bbca9534b..abe1e23a4db3 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -20,6 +20,7 @@
#include <linux/init.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_gact.h>
#include <net/tc_act/tc_gact.h>
@@ -60,6 +61,7 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, gact_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_GACT_MAX + 1];
struct tc_gact *parm;
struct tcf_gact *gact;
@@ -116,9 +118,14 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
return err;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
gact = to_gact(*a);
spin_lock_bh(&gact->tcf_lock);
+ oldchain = gact->tcf_goto_chain;
+ gact->tcf_goto_chain = newchain;
gact->tcf_action = parm->action;
#ifdef CONFIG_GACT_PROB
if (p_parm) {
@@ -135,7 +142,13 @@ static int tcf_gact_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
+
return ret;
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_gact_act(struct sk_buff *skb, const struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json b/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
index 89189a03ce3d..814b7a8a478b 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
@@ -560,5 +560,30 @@
"teardown": [
"$TC actions flush action gact"
]
+ },
+ {
+ "id": "ca89",
+ "name": "Replace gact action with invalid goto chain control",
+ "category": [
+ "actions",
+ "gact"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action gact",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action pass random determ drop 2 index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action goto chain 42 random determ drop 5 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions list action gact",
+ "matchPattern": "action order [0-9]*: gact action pass.*random type determ drop val 2.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action gact"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 05/16] net/sched: act_ife: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (3 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 04/16] net/sched: act_gact: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-03-01 16:33 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 06/16] net/sched: act_mirred: " Davide Caratti
` (10 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action ife encode allow mark pass index 90
# tc actions replace action ife \
> encode allow mark goto chain 42 index 90 cookie c1a0c1a0
# tc action show action ife
had the following output:
IFE type 0xED3E
IFE type 0xED3E
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: ife encode action goto chain 42 type 0XED3E
allow mark
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000007b4e7067 P4D 800000007b4e7067 PUD 7b4e6067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 2 PID: 164 Comm: kworker/2:1 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffa6a7c0553ad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9796ee1bbd00 RCX: 0000000000000001
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: ffffa6a7c0553b70 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: ffff9797385bb038 R12: ffff9796ead9d700
R13: ffff9796ead9d708 R14: 0000000000000001 R15: ffff9796ead9d800
FS: 0000000000000000(0000) GS:ffff97973db00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007c41e006 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_gact act_meta_mark act_ife dummy veth ip6table_filter ip6_tables iptable_filter binfmt_misc snd_hda_codec_generic ext4 snd_hda_intel snd_hda_codec crct10dif_pclmul mbcache crc32_pclmul jbd2 snd_hwdep snd_hda_core ghash_clmulni_intel snd_seq snd_seq_device snd_pcm snd_timer aesni_intel crypto_simd snd cryptd glue_helper virtio_balloon joydev pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl virtio_net drm_kms_helper virtio_blk net_failover syscopyarea failover sysfillrect virtio_console sysimgblt fb_sys_fops ttm drm crc32c_intel serio_raw ata_piix virtio_pci virtio_ring libata virtio floppy dm_mirror dm_region_hash dm_log dm_mod [last unloaded: act_ife]
CR2: 0000000000000000
Validating the control action within tcf_ife_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_ife.c | 32 ++++++++++++-------
.../tc-testing/tc-tests/actions/ife.json | 25 +++++++++++++++
2 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 9b2eb941e093..4480edb386d7 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -29,6 +29,7 @@
#include <net/net_namespace.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <uapi/linux/tc_act/tc_ife.h>
#include <net/tc_act/tc_ife.h>
#include <linux/etherdevice.h>
@@ -472,6 +473,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, ife_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_IFE_MAX + 1];
struct nlattr *tb2[IFE_META_MAX + 1];
struct tcf_ife_params *p;
@@ -531,6 +533,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
}
ife = to_ife(*a);
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
+
p->flags = parm->flags;
if (parm->flags & IFE_ENCODE) {
@@ -563,13 +569,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
if (tb[TCA_IFE_METALST]) {
err = nla_parse_nested(tb2, IFE_META_MAX, tb[TCA_IFE_METALST],
NULL, NULL);
- if (err) {
-metadata_parse_err:
- tcf_idr_release(*a, bind);
- kfree(p);
- return err;
- }
-
+ if (err)
+ goto metadata_parse_err;
err = populate_metalist(ife, tb2, exists, rtnl_held);
if (err)
goto metadata_parse_err;
@@ -581,15 +582,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
* going to bail out
*/
err = use_all_metadata(ife, exists);
- if (err) {
- tcf_idr_release(*a, bind);
- kfree(p);
- return err;
- }
+ if (err)
+ goto metadata_parse_err;
}
if (exists)
spin_lock_bh(&ife->tcf_lock);
+ oldchain = ife->tcf_goto_chain;
+ ife->tcf_goto_chain = newchain;
ife->tcf_action = parm->action;
/* protected by tcf_lock when modifying existing action */
rcu_swap_protected(ife->params, p, 1);
@@ -603,6 +603,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
tcf_idr_insert(tn, *a);
return ret;
+
+metadata_parse_err:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ kfree(p);
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
index 0da3545cabdb..c13a68b98fc7 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
@@ -1060,5 +1060,30 @@
"matchPattern": "action order [0-9]*: ife encode action pipe.*allow prio.*index 4",
"matchCount": "0",
"teardown": []
+ },
+ {
+ "id": "a0e2",
+ "name": "Replace ife encode action with invalid goto chain control",
+ "category": [
+ "actions",
+ "ife"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action ife",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action ife encode allow mark pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action ife encode allow mark goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action ife index 90",
+ "matchPattern": "action order [0-9]*: ife encode action pass.*type 0[xX]ED3E .*allow mark.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action ife"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 06/16] net/sched: act_mirred: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (4 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
` (9 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action mirred ingress mirror dev lo pass
# tc actions replace action mirred \
> ingress mirror dev lo goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action mirred
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: mirred (Ingress Mirror to device lo) goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
Mirror/redirect action on
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 47 Comm: kworker/3:1 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffa772404b7ad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9c5afc3f4300 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff9c5afdba9380 RDI: 0000000000029380
RBP: ffffa772404b7b70 R08: ffff9c5af7010028 R09: ffff9c5af7010029
R10: 0000000000000000 R11: ffff9c5af94c6a38 R12: ffff9c5af7953000
R13: ffff9c5af7953008 R14: 0000000000000001 R15: ffff9c5af7953d00
FS: 0000000000000000(0000) GS:ffff9c5afdb80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007c514004 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_mirred veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul snd_hda_codec_generic crc32_pclmul snd_hda_intel snd_hda_codec mbcache ghash_clmulni_intel jbd2 snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel snd_timer snd crypto_simd cryptd glue_helper soundcore virtio_balloon joydev pcspkr i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net ttm virtio_blk net_failover virtio_console failover drm ata_piix crc32c_intel virtio_pci serio_raw libata virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_mirred_init() proved to fix the
above issue. For the same reason, postpone the assignment of tcfa_action
and tcfm_eaction to avoid partial reconfiguration of a mirred rule when
it's replaced by another one that mirrors to a device that does not
exist. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_mirred.c | 23 ++++++++++++++---
.../tc-testing/tc-tests/actions/mirred.json | 25 +++++++++++++++++++
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 69dda57f1097..49f5116d01e6 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -99,6 +99,7 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
{
struct tc_action_net *tn = net_generic(net, mirred_net_id);
struct nlattr *tb[TCA_MIRRED_MAX + 1];
+ struct tcf_chain *newchain = NULL, *oldchain;
bool mac_header_xmit = false;
struct tc_mirred *parm;
struct tcf_mirred *m;
@@ -158,18 +159,20 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
+
m = to_mirred(*a);
spin_lock_bh(&m->tcf_lock);
- m->tcf_action = parm->action;
- m->tcfm_eaction = parm->eaction;
if (parm->ifindex) {
dev = dev_get_by_index(net, parm->ifindex);
if (!dev) {
spin_unlock_bh(&m->tcf_lock);
- tcf_idr_release(*a, bind);
- return -ENODEV;
+ err = -ENODEV;
+ goto put_chain;
}
mac_header_xmit = dev_is_mac_header_xmit(dev);
rcu_swap_protected(m->tcfm_dev, dev,
@@ -178,6 +181,10 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
dev_put(dev);
m->tcfm_mac_header_xmit = mac_header_xmit;
}
+ oldchain = m->tcf_goto_chain;
+ m->tcf_goto_chain = newchain;
+ m->tcf_action = parm->action;
+ m->tcfm_eaction = parm->eaction;
spin_unlock_bh(&m->tcf_lock);
if (ret == ACT_P_CREATED) {
@@ -188,7 +195,15 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
tcf_idr_insert(tn, *a);
}
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_mirred_act(struct sk_buff *skb, const struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json b/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
index db49fd0f8445..6e5fb3d25681 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/mirred.json
@@ -434,5 +434,30 @@
"teardown": [
"$TC actions flush action mirred"
]
+ },
+ {
+ "id": "2a9a",
+ "name": "Replace mirred action with invalid goto chain control",
+ "category": [
+ "actions",
+ "mirred"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action mirred",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action mirred ingress mirror dev lo drop index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action mirred ingress mirror dev lo goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action mirred index 90",
+ "matchPattern": "action order [0-9]*: mirred \\(Ingress Mirror to device lo\\) drop.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action mirred"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 07/16] net/sched: act_connmark: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (5 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 06/16] net/sched: act_mirred: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-03-01 16:35 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 08/16] net/sched: act_nat: " Davide Caratti
` (8 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action connmark pass index 90
# tc actions replace action connmark \
> goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action connmark
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: connmark zone 0 goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 0 PID: 302 Comm: kworker/0:2 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff9bea406c3ad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff8c5dfc009f00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff9bea406c3a80 RDI: ffff8c5dfb9d6ec0
RBP: ffff9bea406c3b70 R08: ffff8c5dfda222a0 R09: ffffffff90933c3c
R10: 0000000000000000 R11: 0000000092793f7d R12: ffff8c5df48b3c00
R13: ffff8c5df48b3c08 R14: 0000000000000001 R15: ffff8c5dfb9d6e40
FS: 0000000000000000(0000) GS:ffff8c5dfda00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000062e0e006 CR4: 00000000001606f0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul mbcache crc32_pclmul jbd2 snd_hda_codec_generic ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel snd_timer crypto_simd cryptd snd glue_helper joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper virtio_net net_failover syscopyarea virtio_blk failover virtio_console sysfillrect sysimgblt fb_sys_fops ttm drm ata_piix crc32c_intel serio_raw libata virtio_pci virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_connmark_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_connmark.c | 24 +++++++++++++++++-
.../tc-testing/tc-tests/actions/connmark.json | 25 +++++++++++++++++++
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index 30c4c109c80c..5e02f9f6850a 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -21,6 +21,7 @@
#include <net/netlink.h>
#include <net/pkt_sched.h>
#include <net/act_api.h>
+#include <net/pkt_cls.h>
#include <uapi/linux/tc_act/tc_connmark.h>
#include <net/tc_act/tc_connmark.h>
@@ -101,10 +102,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, connmark_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_CONNMARK_MAX + 1];
struct tcf_connmark_info *ci;
struct tc_connmark *parm;
- int ret = 0;
+ int ret = 0, err;
if (!nla)
return -EINVAL;
@@ -129,6 +131,12 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
}
ci = to_connmark(*a);
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
+ extack);
+ if (err)
+ goto release_idr;
+ oldchain = ci->tcf_goto_chain;
+ ci->tcf_goto_chain = newchain;
ci->tcf_action = parm->action;
ci->net = net;
ci->zone = parm->zone;
@@ -143,15 +151,29 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
+ extack);
+ if (err)
+ goto release_idr;
/* replacing action and zone */
spin_lock_bh(&ci->tcf_lock);
+ oldchain = ci->tcf_goto_chain;
+ ci->tcf_goto_chain = newchain;
ci->tcf_action = parm->action;
ci->zone = parm->zone;
spin_unlock_bh(&ci->tcf_lock);
ret = 0;
+ } else {
+ goto end;
}
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
+end:
return ret;
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
index 13147a1f5731..cadde8f41fcd 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
@@ -287,5 +287,30 @@
"teardown": [
"$TC actions flush action connmark"
]
+ },
+ {
+ "id": "c506",
+ "name": "Replace connmark with invalid goto chain control",
+ "category": [
+ "actions",
+ "connmark"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action connmark",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action connmark pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action connmark goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action connmark index 90",
+ "matchPattern": "action order [0-9]+: connmark zone 0 pass.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action connmark"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 08/16] net/sched: act_nat: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (6 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 09/16] net/sched: act_pedit: " Davide Caratti
` (7 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action nat ingress 1.18.1.1 1.18.2.2 pass index 90
# tc actions replace action nat \
> ingress 1.18.1.1 1.18.2.2 goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action nat
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: nat ingress 1.18.1.1/32 1.18.2.2 goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000002d180067 P4D 800000002d180067 PUD 7cb8b067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 164 Comm: kworker/3:1 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffae4500e2fad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9fa52e28c800 RCX: 0000000001011201
RDX: 0000000000000000 RSI: 0000000000000056 RDI: ffff9fa52ca12800
RBP: ffffae4500e2fb70 R08: 0000000000000022 R09: 000000000000000e
R10: 00000000ffffffff R11: 0000000001011201 R12: ffff9fa52cbc9c00
R13: ffff9fa52cbc9c08 R14: 0000000000000001 R15: ffff9fa52ca12780
FS: 0000000000000000(0000) GS:ffff9fa57db80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000073f8c004 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_nat veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel mbcache jbd2 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper snd_timer snd joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs qxl ata_generic pata_acpi drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm virtio_net virtio_blk net_failover failover virtio_console drm crc32c_intel floppy ata_piix libata virtio_pci virtio_ring virtio serio_raw dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_nat_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_nat.c | 12 +++++++++
.../tc-testing/tc-tests/actions/nat.json | 25 +++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 526c4c99bcce..97aa55b4b268 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -21,6 +21,7 @@
#include <linux/string.h>
#include <linux/tc_act/tc_nat.h>
#include <net/act_api.h>
+#include <net/pkt_cls.h>
#include <net/icmp.h>
#include <net/ip.h>
#include <net/netlink.h>
@@ -42,6 +43,7 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, nat_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_NAT_MAX + 1];
struct tc_nat *parm;
int ret = 0, err;
@@ -77,6 +79,9 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
} else {
return err;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
p = to_tcf_nat(*a);
spin_lock_bh(&p->tcf_lock);
@@ -85,13 +90,20 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
p->mask = parm->mask;
p->flags = parm->flags;
+ oldchain = p->tcf_goto_chain;
+ p->tcf_goto_chain = newchain;
p->tcf_action = parm->action;
spin_unlock_bh(&p->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_nat_act(struct sk_buff *skb, const struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/nat.json b/tools/testing/selftests/tc-testing/tc-tests/actions/nat.json
index 0080dc2fd41c..bc12c1ccad30 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/nat.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/nat.json
@@ -589,5 +589,30 @@
"teardown": [
"$TC actions flush action nat"
]
+ },
+ {
+ "id": "4b12",
+ "name": "Replace nat action with invalid goto chain control",
+ "category": [
+ "actions",
+ "nat"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action nat",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action nat ingress 1.18.1.1 1.18.2.2 drop index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action nat ingress 1.18.1.1 1.18.2.2 goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action nat index 90",
+ "matchPattern": "action order [0-9]+: nat ingress 1.18.1.1/32 1.18.2.2 drop.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action nat"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 09/16] net/sched: act_pedit: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (7 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 08/16] net/sched: act_nat: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 10/16] net/sched: act_police: " Davide Caratti
` (6 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc filter add dev crash0 egress matchall \
> action pedit ex munge ip ttl set 10 pass index 90
# tc actions replace action pedit \
> ex munge ip ttl set 10 goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action pedit
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: pedit action goto chain 42 keys 1
index 90 ref 2 bind 1
key #0 at ipv4+8: val 0a000000 mask 00ffffff
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff94a73db03be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff94a6ee4c0700 RCX: 000000000000000a
RDX: 0000000000000000 RSI: ffff94a6ed22c800 RDI: 0000000000000000
RBP: ffff94a73db03c80 R08: ffff94a7386fa4c8 R09: ffff94a73229ea20
R10: 0000000000000000 R11: 0000000000000000 R12: ffff94a6ed22cb00
R13: ffff94a6ed22cb08 R14: 0000000000000001 R15: ffff94a6ed22c800
FS: 0000000000000000(0000) GS:ffff94a73db00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007120e002 CR4: 00000000001606e0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? tick_sched_timer+0x37/0x70
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 4e ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffffab1740387eb8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffffb18184f0 RBX: 0000000000000002 RCX: 0000000000000001
RDX: 0000000000000001 RSI: 0000000000000087 RDI: 0000000000000002
RBP: 0000000000000002 R08: 000f168fa695f9a9 R09: 0000000000000020
R10: 0000000000000004 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_secondary+0x1a7/0x200
secondary_startup_64+0xa4/0xb0
Modules linked in: act_pedit veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 mbcache jbd2 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep aesni_intel snd_hda_core crypto_simd snd_seq cryptd glue_helper snd_seq_device snd_pcm joydev snd_timer pcspkr virtio_balloon snd soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs qxl ata_generic pata_acpi drm_kms_helper virtio_net net_failover syscopyarea sysfillrect sysimgblt failover virtio_blk fb_sys_fops virtio_console ttm drm crc32c_intel serio_raw ata_piix virtio_pci libata virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_pedit_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_pedit.c | 17 ++++++-
.../tc-testing/tc-tests/actions/pedit.json | 51 +++++++++++++++++++
2 files changed, 66 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 1c7a0db7b466..1eccd5053e70 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -23,6 +23,7 @@
#include <linux/tc_act/tc_pedit.h>
#include <net/tc_act/tc_pedit.h>
#include <uapi/linux/tc_act/tc_pedit.h>
+#include <net/pkt_cls.h>
static unsigned int pedit_net_id;
static struct tc_action_ops act_pedit_ops;
@@ -141,6 +142,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, pedit_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_PEDIT_MAX + 1];
struct tc_pedit_key *keys = NULL;
struct tcf_pedit_key_ex *keys_ex;
@@ -205,6 +207,11 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
goto out_free;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err) {
+ ret = err;
+ goto out_release;
+ }
p = to_pedit(*a);
spin_lock_bh(&p->tcf_lock);
@@ -214,7 +221,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
if (!keys) {
spin_unlock_bh(&p->tcf_lock);
ret = -ENOMEM;
- goto out_release;
+ goto put_chain;
}
kfree(p->tcfp_keys);
p->tcfp_keys = keys;
@@ -223,6 +230,8 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
memcpy(p->tcfp_keys, parm->keys, ksize);
p->tcfp_flags = parm->flags;
+ oldchain = p->tcf_goto_chain;
+ p->tcf_goto_chain = newchain;
p->tcf_action = parm->action;
kfree(p->tcfp_keys_ex);
@@ -231,14 +240,18 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
spin_unlock_bh(&p->tcf_lock);
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
out_release:
tcf_idr_release(*a, bind);
out_free:
kfree(keys_ex);
return ret;
-
}
static void tcf_pedit_cleanup(struct tc_action *a)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json b/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
new file mode 100644
index 000000000000..b73ceb9e28b1
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/pedit.json
@@ -0,0 +1,51 @@
+[
+ {
+ "id": "319a",
+ "name": "Add pedit action that mangles IP TTL",
+ "category": [
+ "actions",
+ "pedit"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action pedit",
+ 0,
+ 1,
+ 255
+ ]
+ ],
+ "cmdUnderTest": "$TC actions add action pedit ex munge ip ttl set 10",
+ "expExitCode": "0",
+ "verifyCmd": "$TC actions ls action pedit",
+ "matchPattern": "action order [0-9]+: pedit action pass keys 1.*index 1 ref.*key #0 at ipv4\\+8: val 0a000000 mask 00ffffff",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action pedit"
+ ]
+ },
+ {
+ "id": "7e67",
+ "name": "Replace pedit action with invalid goto chain",
+ "category": [
+ "actions",
+ "pedit"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action pedit",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action pedit ex munge ip ttl set 10 pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action pedit ex munge ip ttl set 10 goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions ls action pedit",
+ "matchPattern": "action order [0-9]+: pedit action pass keys 1.*index 90 ref.*key #0 at ipv4\\+8: val 0a000000 mask 00ffffff",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action pedit"
+ ]
+ }
+]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 10/16] net/sched: act_police: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (8 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 09/16] net/sched: act_pedit: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 11/16] net/sched: act_sample: " Davide Caratti
` (5 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action police rate 3mbit burst 250k pass index 90
# tc actions replace action police \
> rate 3mbit burst 250k goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action police rate 3mbit burst
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: police 0x5a rate 3Mbit burst 250Kb mtu 2Kb action goto chain 42 overhead 0b
ref 2 bind 1
cookie c1a0c1a0
Then, when crash0 starts transmitting more than 3Mbit/s, the following
kernel crash is observed:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000007a779067 P4D 800000007a779067 PUD 2ad96067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 5032 Comm: netperf Not tainted 5.0.0-rc4.gotochain_crash+ #533
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffb0e04064fa60 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff93bb3322cce0 RCX: 0000000000000005
RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff93bb3322cce0
RBP: ffffb0e04064fb00 R08: 0000000000000022 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: ffff93bb3beed300
R13: ffff93bb3beed308 R14: 0000000000000001 R15: ffff93bb3b64d000
FS: 00007f0bc6be5740(0000) GS:ffff93bb3db80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000746a8001 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ipt_do_table+0x31c/0x420 [ip_tables]
? ip_finish_output2+0x16f/0x430
ip_finish_output2+0x16f/0x430
? ip_output+0x69/0xe0
ip_output+0x69/0xe0
? ip_forward_options+0x1a0/0x1a0
__tcp_transmit_skb+0x563/0xa40
tcp_write_xmit+0x243/0xfa0
__tcp_push_pending_frames+0x32/0xf0
tcp_sendmsg_locked+0x404/0xd30
tcp_sendmsg+0x27/0x40
sock_sendmsg+0x36/0x40
__sys_sendto+0x10e/0x140
? __sys_connect+0x87/0xf0
? syscall_trace_enter+0x1df/0x2e0
? __audit_syscall_exit+0x216/0x260
__x64_sys_sendto+0x24/0x30
do_syscall_64+0x5b/0x180
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7f0bc5ffbafd
Code: 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 8b 05 ae c4 2c 00 85 c0 75 2d 45 31 c9 45 31 c0 4c 63 d1 48 63 ff b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 01 c3 48 8b 15 63 63 2c 00 f7 d8 64 89 02 48
RSP: 002b:00007fffef94b7f8 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 0000000000004000 RCX: 00007f0bc5ffbafd
RDX: 0000000000004000 RSI: 00000000017e5420 RDI: 0000000000000004
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
R13: 00000000017e51d0 R14: 0000000000000010 R15: 0000000000000006
Modules linked in: act_police veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 snd_hda_codec_generic mbcache crct10dif_pclmul jbd2 crc32_pclmul ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper snd_timer snd joydev pcspkr virtio_balloon soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm drm virtio_blk virtio_net virtio_console net_failover failover crc32c_intel ata_piix libata serio_raw virtio_pci virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_police_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_police.c | 12 +++++++++
.../tc-testing/tc-tests/actions/police.json | 25 +++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index a444dd78a244..1f7b30b645e7 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -21,6 +21,7 @@
#include <linux/slab.h>
#include <net/act_api.h>
#include <net/netlink.h>
+#include <net/pkt_cls.h>
struct tcf_police_params {
int tcfp_result;
@@ -87,6 +88,7 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
int ret = 0, tcfp_result = TC_ACT_OK, err, size;
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_POLICE_MAX + 1];
struct tc_police *parm;
struct tcf_police *police;
@@ -129,6 +131,9 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
police = to_police(*a);
if (parm->rate.rate) {
@@ -214,6 +219,8 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
if (new->peak_present)
police->tcfp_ptoks = new->tcfp_mtu_ptoks;
spin_unlock_bh(&police->tcfp_lock);
+ oldchain = police->tcf_goto_chain;
+ police->tcf_goto_chain = newchain;
police->tcf_action = parm->action;
rcu_swap_protected(police->params,
new,
@@ -225,11 +232,16 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
failure:
qdisc_put_rtab(P_tab);
qdisc_put_rtab(R_tab);
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
tcf_idr_release(*a, bind);
return err;
}
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
index 4086a50a670e..b8268da5adaa 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/police.json
@@ -739,5 +739,30 @@
"teardown": [
"$TC actions flush action police"
]
+ },
+ {
+ "id": "689e",
+ "name": "Replace police action with invalid goto chain control",
+ "category": [
+ "actions",
+ "police"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action police",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action police rate 3mbit burst 250k drop index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action police rate 3mbit burst 250k goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action police index 90",
+ "matchPattern": "action order [0-9]*: police 0x5a rate 3Mbit burst 250Kb mtu 2Kb action drop",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action police"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 11/16] net/sched: act_sample: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (9 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 10/16] net/sched: act_police: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
` (4 subsequent siblings)
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action sample rate 1024 group 4 pass index 90
# tc actions replace action sample \
> rate 1024 group 4 goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action sample
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: sample rate 1/1024 group 4 goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 8000000079966067 P4D 8000000079966067 PUD 7987b067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 0 PID: 5 Comm: kworker/0:0 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffbee60033fad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff99d7ae6e3b00 RCX: 00000000e555df9b
RDX: 0000000000000000 RSI: 00000000b0352718 RDI: ffff99d7fda1fcf0
RBP: ffffbee60033fb70 R08: 0000000070731ab1 R09: 0000000000000400
R10: 0000000000000000 R11: ffff99d7ac733838 R12: ffff99d7f3c2be00
R13: ffff99d7f3c2be08 R14: 0000000000000001 R15: ffff99d7f3c2b600
FS: 0000000000000000(0000) GS:ffff99d7fda00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000797de006 CR4: 00000000001606f0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_sample psample veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel mbcache jbd2 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device aesni_intel crypto_simd snd_pcm cryptd glue_helper snd_timer joydev snd pcspkr virtio_balloon i2c_piix4 soundcore nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect virtio_net sysimgblt fb_sys_fops net_failover ttm failover virtio_blk virtio_console drm ata_piix serio_raw crc32c_intel libata virtio_pci virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_sample_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_sample.c | 20 +++++++++++++--
.../tc-testing/tc-tests/actions/sample.json | 25 +++++++++++++++++++
2 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index b2154edcb535..1effafc7fd0b 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -22,6 +22,7 @@
#include <linux/tc_act/tc_sample.h>
#include <net/tc_act/tc_sample.h>
#include <net/psample.h>
+#include <net/pkt_cls.h>
#include <linux/if_arp.h>
@@ -41,6 +42,7 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, sample_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_SAMPLE_MAX + 1];
struct psample_group *psample_group;
struct tc_sample *parm;
@@ -79,17 +81,22 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
psample_group_num = nla_get_u32(tb[TCA_SAMPLE_PSAMPLE_GROUP]);
psample_group = psample_group_get(net, psample_group_num);
if (!psample_group) {
- tcf_idr_release(*a, bind);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto put_chain;
}
s = to_sample(*a);
spin_lock_bh(&s->tcf_lock);
+ oldchain = s->tcf_goto_chain;
+ s->tcf_goto_chain = newchain;
s->tcf_action = parm->action;
s->rate = nla_get_u32(tb[TCA_SAMPLE_RATE]);
s->psample_group_num = psample_group_num;
@@ -103,7 +110,16 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static void tcf_sample_cleanup(struct tc_action *a)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json b/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
index 3aca33c00039..27f0acaed880 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/sample.json
@@ -584,5 +584,30 @@
"teardown": [
"$TC actions flush action sample"
]
+ },
+ {
+ "id": "0a6e",
+ "name": "Replace sample action with invalid goto chain control",
+ "category": [
+ "actions",
+ "sample"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action sample",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action sample rate 1024 group 4 pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action sample rate 1024 group 7 goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions list action sample",
+ "matchPattern": "action order [0-9]+: sample rate 1/1024 group 4 pass.*index 90",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action sample"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 12/16] net/sched: act_simple: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (10 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 11/16] net/sched: act_sample: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-03-01 16:39 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
` (3 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action simple sdata hello pass index 90
# tc actions replace action simple \
> sdata world goto chain 42 index 90 cookie c1a0c1a0
# tc action show action simple
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: Simple <world>
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000006a6fb067 P4D 800000006a6fb067 PUD 6aed6067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 2 PID: 3241 Comm: kworker/2:0 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffbe6781763ad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9e59bdb80e00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff9e59b4716738 RDI: ffff9e59ab12d140
RBP: ffffbe6781763b70 R08: 0000000000000234 R09: 0000000000aaaaaa
R10: 0000000000000000 R11: ffff9e59b247cd50 R12: ffff9e59b112f100
R13: ffff9e59b112f108 R14: 0000000000000001 R15: ffff9e59ab12d0c0
FS: 0000000000000000(0000) GS:ffff9e59b4700000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000006af92004 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_simple veth ip6table_filter ip6_tables iptable_filter binfmt_misc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel ext4 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep mbcache snd_hda_core jbd2 snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd snd_timer glue_helper snd joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net ttm net_failover virtio_console virtio_blk failover drm crc32c_intel serio_raw floppy ata_piix libata virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_simple_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_simple.c | 50 +++++++++++++++----
.../tc-testing/tc-tests/actions/simple.json | 25 ++++++++++
2 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 640ee5b785dc..ca8ef8378c33 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -18,6 +18,7 @@
#include <linux/rtnetlink.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#define TCA_ACT_SIMP 22
@@ -62,14 +63,28 @@ static int alloc_defdata(struct tcf_defact *d, const struct nlattr *defdata)
return 0;
}
-static void reset_policy(struct tcf_defact *d, const struct nlattr *defdata,
- struct tc_defact *p)
+static int reset_policy(struct tcf_defact *d, const struct nlattr *defdata,
+ struct tc_defact *p, struct tcf_proto *tp,
+ struct netlink_ext_ack *extack)
{
+ struct tcf_chain *newchain = NULL, *oldchain;
+ int err;
+
+ err = tcf_action_check_ctrlact(p->action, tp, &newchain, extack);
+ if (err)
+ return err;
+
spin_lock_bh(&d->tcf_lock);
+ oldchain = d->tcf_goto_chain;
+ d->tcf_goto_chain = newchain;
d->tcf_action = p->action;
memset(d->tcfd_defdata, 0, SIMP_MAX_DATA);
nla_strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
spin_unlock_bh(&d->tcf_lock);
+
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
+ return err;
}
static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
@@ -83,6 +98,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, simp_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_DEF_MAX + 1];
struct tc_defact *parm;
struct tcf_defact *d;
@@ -124,27 +140,41 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
}
d = to_defact(*a);
- ret = alloc_defdata(d, tb[TCA_DEF_DATA]);
- if (ret < 0) {
- tcf_idr_release(*a, bind);
- return ret;
- }
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
+ extack);
+ if (err < 0)
+ goto release_idr;
+
+ err = alloc_defdata(d, tb[TCA_DEF_DATA]);
+ if (err < 0)
+ goto release_idr;
+
+ oldchain = d->tcf_goto_chain;
+ d->tcf_goto_chain = newchain;
d->tcf_action = parm->action;
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
ret = ACT_P_CREATED;
} else {
d = to_defact(*a);
if (!ovr) {
- tcf_idr_release(*a, bind);
- return -EEXIST;
+ err = -EEXIST;
+ goto release_idr;
}
- reset_policy(d, tb[TCA_DEF_DATA], parm);
+ err = reset_policy(d, tb[TCA_DEF_DATA], parm, tp, extack);
+ if (err)
+ goto release_idr;
}
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
return ret;
+
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
index e89a7aa4012d..8e8c1ae12260 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
@@ -126,5 +126,30 @@
"teardown": [
""
]
+ },
+ {
+ "id": "b776",
+ "name": "Replace simple action with invalid goto chain control",
+ "category": [
+ "actions",
+ "simple"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action simple",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action simple sdata \"hello\" pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action simple sdata \"world\" goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions list action simple",
+ "matchPattern": "action order [0-9]*: Simple <hello>.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action simple"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 13/16] net/sched: act_skbedit: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (11 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-03-01 16:43 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 14/16] net/sched: act_skbmod: " Davide Caratti
` (2 subsequent siblings)
15 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action skbedit ptype host pass index 90
# tc actions replace action skbedit \
> ptype host goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action skbedit
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: skbedit ptype host goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 0 P4D 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 3467 Comm: kworker/3:3 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffffb50a81e1fad0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9aa47ba4ea00 RCX: 0000000000000001
RDX: 0000000000000000 RSI: ffff9aa469eeb3c0 RDI: ffff9aa47ba4ea00
RBP: ffffb50a81e1fb70 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: ffff9aa47bce0638 R12: ffff9aa4793b0c00
R13: ffff9aa4793b0c08 R14: 0000000000000001 R15: ffff9aa469eeb3c0
FS: 0000000000000000(0000) GS:ffff9aa474780000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007360e005 CR4: 00000000001606e0
Call Trace:
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ndisc_next_option+0x50/0x50
? ___neigh_create+0x4d5/0x680
? ip6_finish_output2+0x1b5/0x590
ip6_finish_output2+0x1b5/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.28+0x79/0xc0
ndisc_send_skb+0x248/0x2e0
ndisc_send_ns+0xf8/0x200
? addrconf_dad_work+0x389/0x4b0
addrconf_dad_work+0x389/0x4b0
? __switch_to_asm+0x34/0x70
? process_one_work+0x195/0x380
? addrconf_dad_completed+0x370/0x370
process_one_work+0x195/0x380
worker_thread+0x30/0x390
? process_one_work+0x380/0x380
kthread+0x113/0x130
? kthread_park+0x90/0x90
ret_from_fork+0x35/0x40
Modules linked in: act_skbedit veth ip6table_filter ip6_tables iptable_filter binfmt_misc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel ext4 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep mbcache snd_hda_core jbd2 snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd snd_timer glue_helper snd joydev soundcore pcspkr virtio_balloon i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm virtio_net net_failover drm failover virtio_blk virtio_console ata_piix virtio_pci crc32c_intel serio_raw libata virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_skbedit_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_skbedit.c | 22 +++++++++++++---
.../tc-testing/tc-tests/actions/skbedit.json | 25 +++++++++++++++++++
2 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 9a8a0f2d4418..5a0d35b2dac3 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -26,6 +26,7 @@
#include <net/ip.h>
#include <net/ipv6.h>
#include <net/dsfield.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_skbedit.h>
#include <net/tc_act/tc_skbedit.h>
@@ -100,6 +101,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbedit_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct tcf_skbedit_params *params_new;
struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
struct tc_skbedit *parm;
@@ -187,12 +189,13 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
return -EEXIST;
}
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
- if (unlikely(!params_new)) {
- tcf_idr_release(*a, bind);
- return -ENOMEM;
- }
+ if (unlikely(!params_new))
+ goto put_chain;
params_new->flags = flags;
if (flags & SKBEDIT_F_PRIORITY)
@@ -209,6 +212,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
params_new->mask = *mask;
spin_lock_bh(&d->tcf_lock);
+ oldchain = d->tcf_goto_chain;
+ d->tcf_goto_chain = newchain;
d->tcf_action = parm->action;
rcu_swap_protected(d->params, params_new,
lockdep_is_held(&d->tcf_lock));
@@ -218,7 +223,16 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json b/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
index 5aaf593b914a..ecd96eda7f6a 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
@@ -484,5 +484,30 @@
"teardown": [
"$TC actions flush action skbedit"
]
+ },
+ {
+ "id": "1b2b",
+ "name": "Replace skbedit action with invalid goto_chain control",
+ "category": [
+ "actions",
+ "skbedit"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action skbedit",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action skbedit ptype host pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action skbedit ptype host goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions list action skbedit",
+ "matchPattern": "action order [0-9]*: skbedit ptype host pass.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action skbedit"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 14/16] net/sched: act_skbmod: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (12 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 15/16] net/sched: act_tunnel_key: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 16/16] net/sched: act_vlan: " Davide Caratti
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action skbmod set smac 00:c1:a0:c1:a0:00 pass index 90
# tc actions replace action skbmod \
> set smac 00:c1:a0:c1:a0:00 goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action skbmod
had the following output:
src MAC address <00:c1:a0:c1:a0:00>
src MAC address <00:c1:a0:c1:a0:00>
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: skbmod goto chain 42 set smac 00:c1:a0:c1:a0:00
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000002d5c7067 P4D 800000002d5c7067 PUD 77e16067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff8987ffd83be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff8987aeb68800 RCX: ffff8987fa263640
RDX: 0000000000000000 RSI: ffff8987f51c8802 RDI: 00000000000000a0
RBP: ffff8987ffd83c80 R08: ffff8987f939bac8 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8987f5c77d00
R13: ffff8987f5c77d08 R14: 0000000000000001 R15: ffff8987f0c29f00
FS: 0000000000000000(0000) GS:ffff8987ffd80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007832c004 CR4: 00000000001606e0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? tick_sched_timer+0x37/0x70
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 56 ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffffa2a1c038feb8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffffa94184f0 RBX: 0000000000000003 RCX: 0000000000000001
RDX: 0000000000000001 RSI: 0000000000000087 RDI: 0000000000000003
RBP: 0000000000000003 R08: 001123cfc2ba71ac R09: 0000000000000000
R10: 0000000000000000 R11: 00000000000f4240 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_secondary+0x1a7/0x200
secondary_startup_64+0xa4/0xb0
Modules linked in: act_skbmod veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul crc32_pclmul ghash_clmulni_intel mbcache jbd2 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device aesni_intel crypto_simd cryptd glue_helper snd_pcm joydev pcspkr virtio_balloon snd_timer snd i2c_piix4 soundcore nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect virtio_net sysimgblt fb_sys_fops net_failover virtio_console ttm virtio_blk failover drm crc32c_intel serio_raw ata_piix virtio_pci libata virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_skbmod_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_skbmod.c | 26 +++++++++++++++----
.../tc-testing/tc-tests/actions/skbmod.json | 25 ++++++++++++++++++
2 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index 35572d0e4576..dbba5ed873c2 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -16,6 +16,7 @@
#include <linux/rtnetlink.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_skbmod.h>
#include <net/tc_act/tc_skbmod.h>
@@ -86,6 +87,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, skbmod_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_SKBMOD_MAX + 1];
struct tcf_skbmod_params *p, *p_old;
struct tc_skbmod *parm;
@@ -154,20 +156,25 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
tcf_idr_release(*a, bind);
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
d = to_skbmod(*a);
p = kzalloc(sizeof(struct tcf_skbmod_params), GFP_KERNEL);
if (unlikely(!p)) {
- tcf_idr_release(*a, bind);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto put_chain;
}
- p->flags = lflags;
- d->tcf_action = parm->action;
-
if (ovr)
spin_lock_bh(&d->tcf_lock);
+
+ p->flags = lflags;
+ oldchain = d->tcf_goto_chain;
+ d->tcf_goto_chain = newchain;
+ d->tcf_action = parm->action;
/* Protected by tcf_lock if overwriting existing action. */
p_old = rcu_dereference_protected(d->skbmod_p, 1);
@@ -187,7 +194,16 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static void tcf_skbmod_cleanup(struct tc_action *a)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/skbmod.json b/tools/testing/selftests/tc-testing/tc-tests/actions/skbmod.json
index fe3326e939c1..6eb4c4f97060 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/skbmod.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/skbmod.json
@@ -392,5 +392,30 @@
"teardown": [
"$TC actions flush action skbmod"
]
+ },
+ {
+ "id": "b651",
+ "name": "Replace skbmod action with invalid goto_chain control",
+ "category": [
+ "actions",
+ "skbmod"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action skbmod",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action skbmod set etype 0x1111 pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action skbmod set etype 0x1111 goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions ls action skbmod",
+ "matchPattern": "action order [0-9]*: skbmod pass set etype 0x1111\\s+index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action skbmod"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 15/16] net/sched: act_tunnel_key: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (13 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 14/16] net/sched: act_skbmod: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 16/16] net/sched: act_vlan: " Davide Caratti
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action tunnel_key set src_ip 10.10.10.1 dst_ip 20.20.2 dst_port 3128 \
> nocsum id 1 pass index 90
# tc actions replace action tunnel_key \
> set src_ip 10.10.10.1 dst_ip 20.20.2 dst_port 3128 nocsum id 1 \
> goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action tunnel_key
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: tunnel_key set
src_ip 10.10.10.1
dst_ip 20.20.2.0
key_id 1
dst_port 3128
nocsum goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000002aba4067 P4D 800000002aba4067 PUD 795f9067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff9346bdb83be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff9346bb795c00 RCX: 0000000000000002
RDX: 0000000000000000 RSI: ffff93466c881700 RDI: 0000000000000246
RBP: ffff9346bdb83c80 R08: ffff9346b3e1e0c8 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff9346b978f000
R13: ffff9346b978f008 R14: 0000000000000001 R15: ffff93466dceeb40
FS: 0000000000000000(0000) GS:ffff9346bdb80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000007a6c2002 CR4: 00000000001606e0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? tick_sched_timer+0x37/0x70
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 55 ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffffa48a8038feb8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffffaa8184f0 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000087 RDI: 0000000000000003
RBP: 0000000000000003 R08: 0011251c6fcfac49 R09: ffff9346b995be00
R10: ffffa48a805e7ce8 R11: 00000000024c38dd R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_secondary+0x1a7/0x200
secondary_startup_64+0xa4/0xb0
Modules linked in: act_tunnel_key veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul crc32_pclmul snd_hda_codec_generic ghash_clmulni_intel mbcache snd_hda_intel jbd2 snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper joydev snd_timer snd pcspkr virtio_balloon soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect virtio_net sysimgblt fb_sys_fops ttm net_failover virtio_console virtio_blk failover drm serio_raw crc32c_intel ata_piix virtio_pci floppy virtio_ring libata virtio dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_tunnel_key_init() proved to fix
the above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_tunnel_key.c | 18 ++++++++++++-
.../tc-tests/actions/tunnel_key.json | 25 +++++++++++++++++++
2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index eaa4a9c80898..dfc932fd0ff7 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -17,6 +17,7 @@
#include <net/netlink.h>
#include <net/pkt_sched.h>
#include <net/dst.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_tunnel_key.h>
#include <net/tc_act/tc_tunnel_key.h>
@@ -213,6 +214,7 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, tunnel_key_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_TUNNEL_KEY_MAX + 1];
struct tcf_tunnel_key_params *params_new;
struct metadata_dst *metadata = NULL;
@@ -353,6 +355,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
goto release_tun_meta;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err) {
+ ret = err;
+ exists = true;
+ goto release_tun_meta;
+ }
t = to_tunnel_key(*a);
params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
@@ -360,12 +368,14 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
NL_SET_ERR_MSG(extack, "Cannot allocate tunnel key parameters");
ret = -ENOMEM;
exists = true;
- goto release_tun_meta;
+ goto put_chain;
}
params_new->tcft_action = parm->t_action;
params_new->tcft_enc_metadata = metadata;
spin_lock_bh(&t->tcf_lock);
+ oldchain = t->tcf_goto_chain;
+ t->tcf_goto_chain = newchain;
t->tcf_action = parm->action;
rcu_swap_protected(t->params, params_new,
lockdep_is_held(&t->tcf_lock));
@@ -374,9 +384,15 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+
release_tun_meta:
if (metadata)
dst_release(&metadata->dst);
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
index e7e15a7336b6..28453a445fdb 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
@@ -884,5 +884,30 @@
"teardown": [
"$TC actions flush action tunnel_key"
]
+ },
+ {
+ "id": "8242",
+ "name": "Replace tunnel_key set action with invalid goto chain",
+ "category": [
+ "actions",
+ "tunnel_key"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action tunnel_key",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action tunnel_key set src_ip 10.10.10.1 dst_ip 20.20.20.2 dst_port 3128 nocsum id 1 pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action tunnel_key set src_ip 10.10.10.2 dst_ip 20.20.20.1 dst_port 3129 id 2 csum goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action tunnel_key index 90",
+ "matchPattern": "action order [0-9]+: tunnel_key.*set.*src_ip 10.10.10.1.*dst_ip 20.20.20.2.*key_id 1.*dst_port 3128.*csum pass.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action tunnel_key"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH net 16/16] net/sched: act_vlan: validate the control action inside init()
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
` (14 preceding siblings ...)
2019-02-27 17:40 ` [PATCH net 15/16] net/sched: act_tunnel_key: " Davide Caratti
@ 2019-02-27 17:40 ` Davide Caratti
15 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-02-27 17:40 UTC (permalink / raw)
To: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, Vlad Buslov
Cc: pabeni, netdev
the following script:
# tc qdisc add dev crash0 clsact
# tc filter add dev crash0 egress matchall \
> action vlan pop pass index 90
# tc actions replace action vlan \
> pop goto chain 42 index 90 cookie c1a0c1a0
# tc actions show action vlan
had the following output:
Error: Failed to init TC action chain.
We have an error talking to the kernel
total acts 1
action order 0: vlan pop goto chain 42
index 90 ref 2 bind 1
cookie c1a0c1a0
Then, the first packet transmitted by crash0 made the kernel crash:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
#PF error: [normal kernel read fault]
PGD 800000007974f067 P4D 800000007974f067 PUD 79638067 PMD 0
Oops: 0000 [#1] SMP PTI
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.0.0-rc4.gotochain_crash+ #536
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
RIP: 0010:tcf_action_exec+0xb8/0x100
Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
RSP: 0018:ffff982dfdb83be0 EFLAGS: 00010246
RAX: 000000002000002a RBX: ffff982dfc55db00 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff982df97099c0 RDI: ffff982dfc55db00
RBP: ffff982dfdb83c80 R08: ffff982df983fec8 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffff982df5aacd00
R13: ffff982df5aacd08 R14: 0000000000000001 R15: ffff982df97099c0
FS: 0000000000000000(0000) GS:ffff982dfdb80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000796d0005 CR4: 00000000001606e0
Call Trace:
<IRQ>
tcf_classify+0x58/0x120
__dev_queue_xmit+0x40a/0x890
? ip6_finish_output2+0x369/0x590
ip6_finish_output2+0x369/0x590
? ip6_output+0x68/0x110
ip6_output+0x68/0x110
? nf_hook.constprop.35+0x79/0xc0
mld_sendpack+0x16f/0x220
mld_ifc_timer_expire+0x195/0x2c0
? igmp6_timer_handler+0x70/0x70
call_timer_fn+0x2b/0x130
run_timer_softirq+0x3e8/0x440
? enqueue_hrtimer+0x39/0x90
__do_softirq+0xe3/0x2f5
irq_exit+0xf0/0x100
smp_apic_timer_interrupt+0x6c/0x130
apic_timer_interrupt+0xf/0x20
</IRQ>
RIP: 0010:native_safe_halt+0x2/0x10
Code: 7b ff ff ff 7f f3 c3 65 48 8b 04 25 00 5c 01 00 f0 80 48 02 20 48 8b 00 a8 08 74 8b eb c1 90 90 90 90 90 90 90 90 90 90 fb f4 <c3> 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 f4 c3 90 90 90 90 90 90
RSP: 0018:ffffa4714038feb8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff13
RAX: ffffffff840184f0 RBX: 0000000000000003 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000001e57d3f387
RBP: 0000000000000003 R08: 001125d9ca39e1eb R09: 0000000000000000
R10: 000000000000027d R11: 000000000009f400 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
? __sched_text_end+0x1/0x1
default_idle+0x1c/0x140
do_idle+0x1c4/0x280
cpu_startup_entry+0x19/0x20
start_secondary+0x1a7/0x200
secondary_startup_64+0xa4/0xb0
Modules linked in: act_vlan veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 snd_hda_codec_generic mbcache crct10dif_pclmul jbd2 snd_hda_intel crc32_pclmul snd_hda_codec ghash_clmulni_intel snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd glue_helper joydev snd_timer virtio_balloon snd pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt virtio_net fb_sys_fops virtio_blk ttm net_failover virtio_console failover ata_piix drm libata crc32c_intel virtio_pci serio_raw virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
CR2: 0000000000000000
Validating the control action within tcf_vlan_init() proved to fix the
above issue. A TDC selftest is added to verify the correct behavior.
Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_vlan.c | 21 ++++++++++++++--
.../tc-testing/tc-tests/actions/vlan.json | 25 +++++++++++++++++++
2 files changed, 44 insertions(+), 2 deletions(-)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 80fd0e238a10..b59e138100dd 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -15,6 +15,7 @@
#include <linux/if_vlan.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
#include <linux/tc_act/tc_vlan.h>
#include <net/tc_act/tc_vlan.h>
@@ -108,6 +109,7 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
struct tcf_proto *tp, struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, vlan_net_id);
+ struct tcf_chain *newchain = NULL, *oldchain;
struct nlattr *tb[TCA_VLAN_MAX + 1];
struct tcf_vlan_params *p;
struct tc_vlan *parm;
@@ -200,12 +202,16 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
return -EEXIST;
}
+ err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
+ if (err)
+ goto release_idr;
+
v = to_vlan(*a);
p = kzalloc(sizeof(*p), GFP_KERNEL);
if (!p) {
- tcf_idr_release(*a, bind);
- return -ENOMEM;
+ err = -ENOMEM;
+ goto put_chain;
}
p->tcfv_action = action;
@@ -214,6 +220,8 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
p->tcfv_push_proto = push_proto;
spin_lock_bh(&v->tcf_lock);
+ oldchain = v->tcf_goto_chain;
+ v->tcf_goto_chain = newchain;
v->tcf_action = parm->action;
rcu_swap_protected(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
spin_unlock_bh(&v->tcf_lock);
@@ -223,7 +231,16 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
if (ret == ACT_P_CREATED)
tcf_idr_insert(tn, *a);
+ if (oldchain)
+ tcf_chain_put_by_act(oldchain);
return ret;
+
+put_chain:
+ if (newchain)
+ tcf_chain_put_by_act(newchain);
+release_idr:
+ tcf_idr_release(*a, bind);
+ return err;
}
static void tcf_vlan_cleanup(struct tc_action *a)
diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
index 69ea09eefffc..cc7c7d758008 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/vlan.json
@@ -688,5 +688,30 @@
"teardown": [
"$TC actions flush action vlan"
]
+ },
+ {
+ "id": "e394",
+ "name": "Replace vlan push action with invalid goto chain control",
+ "category": [
+ "actions",
+ "vlan"
+ ],
+ "setup": [
+ [
+ "$TC actions flush action vlan",
+ 0,
+ 1,
+ 255
+ ],
+ "$TC actions add action vlan push id 500 pass index 90"
+ ],
+ "cmdUnderTest": "$TC actions replace action vlan push id 500 goto chain 42 index 90 cookie c1a0c1a0",
+ "expExitCode": "255",
+ "verifyCmd": "$TC actions get action vlan index 90",
+ "matchPattern": "action order [0-9]+: vlan.*push id 500 protocol 802.1Q priority 0 pass.*index 90 ref",
+ "matchCount": "1",
+ "teardown": [
+ "$TC actions flush action vlan"
+ ]
}
]
--
2.20.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
@ 2019-02-28 1:28 ` Cong Wang
2019-03-01 17:59 ` Davide Caratti
0 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2019-02-28 1:28 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
On Wed, Feb 27, 2019 at 9:41 AM Davide Caratti <dcaratti@redhat.com> wrote:
> +int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> + struct tcf_chain **handle,
Please use a better name than 'handle'. 'handle' is usually used
for a hex numeric ID. Here you just want to save the allocated
tcf_chain to this address.
> + struct netlink_ext_ack *extack)
> +{
> + int opcode = TC_ACT_EXT_OPCODE(action), ret = -EINVAL;
> + u32 chain_index;
> +
> + if (!opcode)
> + ret = action > TC_ACT_VALUE_MAX ? -EINVAL : 0;
> + else if (opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC)
> + ret = 0;
> + if (ret) {
> + NL_SET_ERR_MSG(extack, "invalid control action");
> + goto end;
> + }
> +
> + if (TC_ACT_EXT_CMP(action, TC_ACT_GOTO_CHAIN)) {
> + chain_index = action & TC_ACT_EXT_VAL_MASK;
> + if (!tp) {
> + ret = -EINVAL;
> + NL_SET_ERR_MSG(extack,
> + "can't use goto_chain with NULL proto");
> + goto end;
> + }
> + if (!handle) {
> + ret = -EINVAL;
> + NL_SET_ERR_MSG(extack,
> + "can't put goto_chain on NULL handle");
> + goto end;
> + }
> + *handle = tcf_chain_get_by_act(tp->chain->block, chain_index);
> + if (!*handle) {
> + ret = -ENOMEM;
Is -ENOMEM okay here? I feel like it should be -ENOSPC or whatever
tcf_chain_get_by_act() says.
Thanks.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
@ 2019-02-28 1:50 ` Cong Wang
2019-03-01 18:02 ` Davide Caratti
0 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2019-02-28 1:50 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
On Wed, Feb 27, 2019 at 9:41 AM Davide Caratti <dcaratti@redhat.com> wrote:
> @@ -108,7 +116,16 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
> if (ret == ACT_P_CREATED)
> tcf_idr_insert(tn, *a);
>
> + if (oldchain)
> + tcf_chain_put_by_act(oldchain);
Do we need to respect RCU grace period here?
Same question for other patches on similar paths.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 05/16] net/sched: act_ife: validate the control action inside init()
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
@ 2019-03-01 16:33 ` Vlad Buslov
0 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2019-03-01 16:33 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, pabeni, netdev
On Wed 27 Feb 2019 at 17:40, Davide Caratti <dcaratti@redhat.com> wrote:
> the following script:
>
> # tc qdisc add dev crash0 clsact
> # tc filter add dev crash0 egress matchall \
> > action ife encode allow mark pass index 90
> # tc actions replace action ife \
> > encode allow mark goto chain 42 index 90 cookie c1a0c1a0
> # tc action show action ife
>
> had the following output:
>
> IFE type 0xED3E
> IFE type 0xED3E
> Error: Failed to init TC action chain.
> We have an error talking to the kernel
> total acts 1
>
> action order 0: ife encode action goto chain 42 type 0XED3E
> allow mark
> index 90 ref 2 bind 1
> cookie c1a0c1a0
>
> Then, the first packet transmitted by crash0 made the kernel crash:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> #PF error: [normal kernel read fault]
> PGD 800000007b4e7067 P4D 800000007b4e7067 PUD 7b4e6067 PMD 0
> Oops: 0000 [#1] SMP PTI
> CPU: 2 PID: 164 Comm: kworker/2:1 Not tainted 5.0.0-rc4.gotochain_crash+ #533
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Workqueue: ipv6_addrconf addrconf_dad_work
> RIP: 0010:tcf_action_exec+0xb8/0x100
> Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
> RSP: 0018:ffffa6a7c0553ad0 EFLAGS: 00010246
> RAX: 000000002000002a RBX: ffff9796ee1bbd00 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> RBP: ffffa6a7c0553b70 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: ffff9797385bb038 R12: ffff9796ead9d700
> R13: ffff9796ead9d708 R14: 0000000000000001 R15: ffff9796ead9d800
> FS: 0000000000000000(0000) GS:ffff97973db00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000007c41e006 CR4: 00000000001606e0
> Call Trace:
> tcf_classify+0x58/0x120
> __dev_queue_xmit+0x40a/0x890
> ? ndisc_next_option+0x50/0x50
> ? ___neigh_create+0x4d5/0x680
> ? ip6_finish_output2+0x1b5/0x590
> ip6_finish_output2+0x1b5/0x590
> ? ip6_output+0x68/0x110
> ip6_output+0x68/0x110
> ? nf_hook.constprop.28+0x79/0xc0
> ndisc_send_skb+0x248/0x2e0
> ndisc_send_ns+0xf8/0x200
> ? addrconf_dad_work+0x389/0x4b0
> addrconf_dad_work+0x389/0x4b0
> ? __switch_to_asm+0x34/0x70
> ? process_one_work+0x195/0x380
> ? addrconf_dad_completed+0x370/0x370
> process_one_work+0x195/0x380
> worker_thread+0x30/0x390
> ? process_one_work+0x380/0x380
> kthread+0x113/0x130
> ? kthread_park+0x90/0x90
> ret_from_fork+0x35/0x40
> Modules linked in: act_gact act_meta_mark act_ife dummy veth ip6table_filter ip6_tables iptable_filter binfmt_misc snd_hda_codec_generic ext4 snd_hda_intel snd_hda_codec crct10dif_pclmul mbcache crc32_pclmul jbd2 snd_hwdep snd_hda_core ghash_clmulni_intel snd_seq snd_seq_device snd_pcm snd_timer aesni_intel crypto_simd snd cryptd glue_helper virtio_balloon joydev pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl virtio_net drm_kms_helper virtio_blk net_failover syscopyarea failover sysfillrect virtio_console sysimgblt fb_sys_fops ttm drm crc32c_intel serio_raw ata_piix virtio_pci virtio_ring libata virtio floppy dm_mirror dm_region_hash dm_log dm_mod [last unloaded: act_ife]
> CR2: 0000000000000000
>
> Validating the control action within tcf_ife_init() proved to fix the
> above issue. A TDC selftest is added to verify the correct behavior.
>
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_ife.c | 32 ++++++++++++-------
> .../tc-testing/tc-tests/actions/ife.json | 25 +++++++++++++++
> 2 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
> index 9b2eb941e093..4480edb386d7 100644
> --- a/net/sched/act_ife.c
> +++ b/net/sched/act_ife.c
> @@ -29,6 +29,7 @@
> #include <net/net_namespace.h>
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> +#include <net/pkt_cls.h>
> #include <uapi/linux/tc_act/tc_ife.h>
> #include <net/tc_act/tc_ife.h>
> #include <linux/etherdevice.h>
> @@ -472,6 +473,7 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, ife_net_id);
> + struct tcf_chain *newchain = NULL, *oldchain;
> struct nlattr *tb[TCA_IFE_MAX + 1];
> struct nlattr *tb2[IFE_META_MAX + 1];
> struct tcf_ife_params *p;
> @@ -531,6 +533,10 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> }
>
> ife = to_ife(*a);
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
> + if (err)
> + goto release_idr;
> +
> p->flags = parm->flags;
>
> if (parm->flags & IFE_ENCODE) {
> @@ -563,13 +569,8 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> if (tb[TCA_IFE_METALST]) {
> err = nla_parse_nested(tb2, IFE_META_MAX, tb[TCA_IFE_METALST],
> NULL, NULL);
> - if (err) {
> -metadata_parse_err:
> - tcf_idr_release(*a, bind);
> - kfree(p);
> - return err;
> - }
> -
> + if (err)
> + goto metadata_parse_err;
> err = populate_metalist(ife, tb2, exists, rtnl_held);
> if (err)
> goto metadata_parse_err;
> @@ -581,15 +582,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> * going to bail out
> */
> err = use_all_metadata(ife, exists);
> - if (err) {
> - tcf_idr_release(*a, bind);
> - kfree(p);
> - return err;
> - }
> + if (err)
> + goto metadata_parse_err;
> }
>
> if (exists)
> spin_lock_bh(&ife->tcf_lock);
> + oldchain = ife->tcf_goto_chain;
> + ife->tcf_goto_chain = newchain;
> ife->tcf_action = parm->action;
> /* protected by tcf_lock when modifying existing action */
> rcu_swap_protected(ife->params, p, 1);
> @@ -603,6 +603,14 @@ static int tcf_ife_init(struct net *net, struct nlattr *nla,
> tcf_idr_insert(tn, *a);
>
oldchain is not released before return.
> return ret;
> +
> +metadata_parse_err:
> + if (newchain)
> + tcf_chain_put_by_act(newchain);
> +release_idr:
> + kfree(p);
> + tcf_idr_release(*a, bind);
> + return err;
> }
>
> static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> index 0da3545cabdb..c13a68b98fc7 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/ife.json
> @@ -1060,5 +1060,30 @@
> "matchPattern": "action order [0-9]*: ife encode action pipe.*allow prio.*index 4",
> "matchCount": "0",
> "teardown": []
> + },
> + {
> + "id": "a0e2",
> + "name": "Replace ife encode action with invalid goto chain control",
> + "category": [
> + "actions",
> + "ife"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action ife",
> + 0,
> + 1,
> + 255
> + ],
> + "$TC actions add action ife encode allow mark pass index 90"
> + ],
> + "cmdUnderTest": "$TC actions replace action ife encode allow mark goto chain 42 index 90 cookie c1a0c1a0",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions get action ife index 90",
> + "matchPattern": "action order [0-9]*: ife encode action pass.*type 0[xX]ED3E .*allow mark.*index 90 ref",
> + "matchCount": "1",
> + "teardown": [
> + "$TC actions flush action ife"
> + ]
> }
> ]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 07/16] net/sched: act_connmark: validate the control action inside init()
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
@ 2019-03-01 16:35 ` Vlad Buslov
0 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2019-03-01 16:35 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, pabeni, netdev
On Wed 27 Feb 2019 at 17:40, Davide Caratti <dcaratti@redhat.com> wrote:
> the following script:
>
> # tc qdisc add dev crash0 clsact
> # tc filter add dev crash0 egress matchall \
> > action connmark pass index 90
> # tc actions replace action connmark \
> > goto chain 42 index 90 cookie c1a0c1a0
> # tc actions show action connmark
>
> had the following output:
>
> Error: Failed to init TC action chain.
> We have an error talking to the kernel
> total acts 1
>
> action order 0: connmark zone 0 goto chain 42
> index 90 ref 2 bind 1
> cookie c1a0c1a0
>
> Then, the first packet transmitted by crash0 made the kernel crash:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> #PF error: [normal kernel read fault]
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 0 PID: 302 Comm: kworker/0:2 Not tainted 5.0.0-rc4.gotochain_crash+ #533
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Workqueue: ipv6_addrconf addrconf_dad_work
> RIP: 0010:tcf_action_exec+0xb8/0x100
> Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
> RSP: 0018:ffff9bea406c3ad0 EFLAGS: 00010246
> RAX: 000000002000002a RBX: ffff8c5dfc009f00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff9bea406c3a80 RDI: ffff8c5dfb9d6ec0
> RBP: ffff9bea406c3b70 R08: ffff8c5dfda222a0 R09: ffffffff90933c3c
> R10: 0000000000000000 R11: 0000000092793f7d R12: ffff8c5df48b3c00
> R13: ffff8c5df48b3c08 R14: 0000000000000001 R15: ffff8c5dfb9d6e40
> FS: 0000000000000000(0000) GS:ffff8c5dfda00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000062e0e006 CR4: 00000000001606f0
> Call Trace:
> tcf_classify+0x58/0x120
> __dev_queue_xmit+0x40a/0x890
> ? ndisc_next_option+0x50/0x50
> ? ___neigh_create+0x4d5/0x680
> ? ip6_finish_output2+0x1b5/0x590
> ip6_finish_output2+0x1b5/0x590
> ? ip6_output+0x68/0x110
> ip6_output+0x68/0x110
> ? nf_hook.constprop.28+0x79/0xc0
> ndisc_send_skb+0x248/0x2e0
> ndisc_send_ns+0xf8/0x200
> ? addrconf_dad_work+0x389/0x4b0
> addrconf_dad_work+0x389/0x4b0
> ? __switch_to_asm+0x34/0x70
> ? process_one_work+0x195/0x380
> ? addrconf_dad_completed+0x370/0x370
> process_one_work+0x195/0x380
> worker_thread+0x30/0x390
> ? process_one_work+0x380/0x380
> kthread+0x113/0x130
> ? kthread_park+0x90/0x90
> ret_from_fork+0x35/0x40
> Modules linked in: act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 veth ip6table_filter ip6_tables iptable_filter binfmt_misc ext4 crct10dif_pclmul mbcache crc32_pclmul jbd2 snd_hda_codec_generic ghash_clmulni_intel snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core snd_seq snd_seq_device snd_pcm aesni_intel snd_timer crypto_simd cryptd snd glue_helper joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper virtio_net net_failover syscopyarea virtio_blk failover virtio_console sysfillrect sysimgblt fb_sys_fops ttm drm ata_piix crc32c_intel serio_raw libata virtio_pci virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
> CR2: 0000000000000000
>
> Validating the control action within tcf_connmark_init() proved to fix the
> above issue. A TDC selftest is added to verify the correct behavior.
>
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_connmark.c | 24 +++++++++++++++++-
> .../tc-testing/tc-tests/actions/connmark.json | 25 +++++++++++++++++++
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index 30c4c109c80c..5e02f9f6850a 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -21,6 +21,7 @@
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> #include <net/act_api.h>
> +#include <net/pkt_cls.h>
> #include <uapi/linux/tc_act/tc_connmark.h>
> #include <net/tc_act/tc_connmark.h>
>
> @@ -101,10 +102,11 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, connmark_net_id);
> + struct tcf_chain *newchain = NULL, *oldchain;
> struct nlattr *tb[TCA_CONNMARK_MAX + 1];
> struct tcf_connmark_info *ci;
> struct tc_connmark *parm;
> - int ret = 0;
> + int ret = 0, err;
>
> if (!nla)
> return -EINVAL;
> @@ -129,6 +131,12 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> }
>
> ci = to_connmark(*a);
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
> + extack);
> + if (err)
> + goto release_idr;
> + oldchain = ci->tcf_goto_chain;
> + ci->tcf_goto_chain = newchain;
> ci->tcf_action = parm->action;
> ci->net = net;
> ci->zone = parm->zone;
> @@ -143,15 +151,29 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> tcf_idr_release(*a, bind);
> return -EEXIST;
> }
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
> + extack);
> + if (err)
> + goto release_idr;
> /* replacing action and zone */
> spin_lock_bh(&ci->tcf_lock);
> + oldchain = ci->tcf_goto_chain;
> + ci->tcf_goto_chain = newchain;
> ci->tcf_action = parm->action;
> ci->zone = parm->zone;
> spin_unlock_bh(&ci->tcf_lock);
> ret = 0;
> + } else {
> + goto end;
Maybe just initialize oldchain to NULL to avoid this goto?
> }
>
> + if (oldchain)
> + tcf_chain_put_by_act(oldchain);
> +end:
> return ret;
> +release_idr:
> + tcf_idr_release(*a, bind);
> + return err;
> }
>
> static inline int tcf_connmark_dump(struct sk_buff *skb, struct tc_action *a,
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
> index 13147a1f5731..cadde8f41fcd 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/connmark.json
> @@ -287,5 +287,30 @@
> "teardown": [
> "$TC actions flush action connmark"
> ]
> + },
> + {
> + "id": "c506",
> + "name": "Replace connmark with invalid goto chain control",
> + "category": [
> + "actions",
> + "connmark"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action connmark",
> + 0,
> + 1,
> + 255
> + ],
> + "$TC actions add action connmark pass index 90"
> + ],
> + "cmdUnderTest": "$TC actions replace action connmark goto chain 42 index 90 cookie c1a0c1a0",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions get action connmark index 90",
> + "matchPattern": "action order [0-9]+: connmark zone 0 pass.*index 90 ref",
> + "matchCount": "1",
> + "teardown": [
> + "$TC actions flush action connmark"
> + ]
> }
> ]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 12/16] net/sched: act_simple: validate the control action inside init()
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
@ 2019-03-01 16:39 ` Vlad Buslov
0 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2019-03-01 16:39 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, pabeni, netdev
On Wed 27 Feb 2019 at 17:40, Davide Caratti <dcaratti@redhat.com> wrote:
> the following script:
>
> # tc qdisc add dev crash0 clsact
> # tc filter add dev crash0 egress matchall \
> > action simple sdata hello pass index 90
> # tc actions replace action simple \
> > sdata world goto chain 42 index 90 cookie c1a0c1a0
> # tc action show action simple
>
> had the following output:
>
> Error: Failed to init TC action chain.
> We have an error talking to the kernel
> total acts 1
>
> action order 0: Simple <world>
> index 90 ref 2 bind 1
> cookie c1a0c1a0
>
> Then, the first packet transmitted by crash0 made the kernel crash:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> #PF error: [normal kernel read fault]
> PGD 800000006a6fb067 P4D 800000006a6fb067 PUD 6aed6067 PMD 0
> Oops: 0000 [#1] SMP PTI
> CPU: 2 PID: 3241 Comm: kworker/2:0 Not tainted 5.0.0-rc4.gotochain_crash+ #536
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Workqueue: ipv6_addrconf addrconf_dad_work
> RIP: 0010:tcf_action_exec+0xb8/0x100
> Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
> RSP: 0018:ffffbe6781763ad0 EFLAGS: 00010246
> RAX: 000000002000002a RBX: ffff9e59bdb80e00 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff9e59b4716738 RDI: ffff9e59ab12d140
> RBP: ffffbe6781763b70 R08: 0000000000000234 R09: 0000000000aaaaaa
> R10: 0000000000000000 R11: ffff9e59b247cd50 R12: ffff9e59b112f100
> R13: ffff9e59b112f108 R14: 0000000000000001 R15: ffff9e59ab12d0c0
> FS: 0000000000000000(0000) GS:ffff9e59b4700000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000006af92004 CR4: 00000000001606e0
> Call Trace:
> tcf_classify+0x58/0x120
> __dev_queue_xmit+0x40a/0x890
> ? ndisc_next_option+0x50/0x50
> ? ___neigh_create+0x4d5/0x680
> ? ip6_finish_output2+0x1b5/0x590
> ip6_finish_output2+0x1b5/0x590
> ? ip6_output+0x68/0x110
> ip6_output+0x68/0x110
> ? nf_hook.constprop.28+0x79/0xc0
> ndisc_send_skb+0x248/0x2e0
> ndisc_send_ns+0xf8/0x200
> ? addrconf_dad_work+0x389/0x4b0
> addrconf_dad_work+0x389/0x4b0
> ? __switch_to_asm+0x34/0x70
> ? process_one_work+0x195/0x380
> ? addrconf_dad_completed+0x370/0x370
> process_one_work+0x195/0x380
> worker_thread+0x30/0x390
> ? process_one_work+0x380/0x380
> kthread+0x113/0x130
> ? kthread_park+0x90/0x90
> ret_from_fork+0x35/0x40
> Modules linked in: act_simple veth ip6table_filter ip6_tables iptable_filter binfmt_misc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel ext4 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep mbcache snd_hda_core jbd2 snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd snd_timer glue_helper snd joydev virtio_balloon pcspkr soundcore i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops virtio_net ttm net_failover virtio_console virtio_blk failover drm crc32c_intel serio_raw floppy ata_piix libata virtio_pci virtio_ring virtio dm_mirror dm_region_hash dm_log dm_mod
> CR2: 0000000000000000
>
> Validating the control action within tcf_simple_init() proved to fix the
> above issue. A TDC selftest is added to verify the correct behavior.
>
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_simple.c | 50 +++++++++++++++----
> .../tc-testing/tc-tests/actions/simple.json | 25 ++++++++++
> 2 files changed, 65 insertions(+), 10 deletions(-)
>
> diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
> index 640ee5b785dc..ca8ef8378c33 100644
> --- a/net/sched/act_simple.c
> +++ b/net/sched/act_simple.c
> @@ -18,6 +18,7 @@
> #include <linux/rtnetlink.h>
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> +#include <net/pkt_cls.h>
>
> #define TCA_ACT_SIMP 22
>
> @@ -62,14 +63,28 @@ static int alloc_defdata(struct tcf_defact *d, const struct nlattr *defdata)
> return 0;
> }
>
> -static void reset_policy(struct tcf_defact *d, const struct nlattr *defdata,
> - struct tc_defact *p)
> +static int reset_policy(struct tcf_defact *d, const struct nlattr *defdata,
> + struct tc_defact *p, struct tcf_proto *tp,
> + struct netlink_ext_ack *extack)
> {
> + struct tcf_chain *newchain = NULL, *oldchain;
> + int err;
> +
> + err = tcf_action_check_ctrlact(p->action, tp, &newchain, extack);
> + if (err)
> + return err;
> +
> spin_lock_bh(&d->tcf_lock);
> + oldchain = d->tcf_goto_chain;
> + d->tcf_goto_chain = newchain;
> d->tcf_action = p->action;
> memset(d->tcfd_defdata, 0, SIMP_MAX_DATA);
> nla_strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
> spin_unlock_bh(&d->tcf_lock);
> +
> + if (oldchain)
> + tcf_chain_put_by_act(oldchain);
> + return err;
> }
>
> static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
> @@ -83,6 +98,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
> struct tcf_proto *tp, struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, simp_net_id);
> + struct tcf_chain *newchain = NULL, *oldchain;
> struct nlattr *tb[TCA_DEF_MAX + 1];
> struct tc_defact *parm;
> struct tcf_defact *d;
> @@ -124,27 +140,41 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
> }
>
> d = to_defact(*a);
> - ret = alloc_defdata(d, tb[TCA_DEF_DATA]);
> - if (ret < 0) {
> - tcf_idr_release(*a, bind);
> - return ret;
> - }
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain,
> + extack);
> + if (err < 0)
> + goto release_idr;
> +
> + err = alloc_defdata(d, tb[TCA_DEF_DATA]);
> + if (err < 0)
> + goto release_idr;
This should jump to error handler that releases newchain besides idr.
> +
> + oldchain = d->tcf_goto_chain;
> + d->tcf_goto_chain = newchain;
> d->tcf_action = parm->action;
> + if (oldchain)
> + tcf_chain_put_by_act(oldchain);
> ret = ACT_P_CREATED;
> } else {
> d = to_defact(*a);
>
> if (!ovr) {
> - tcf_idr_release(*a, bind);
> - return -EEXIST;
> + err = -EEXIST;
> + goto release_idr;
> }
>
> - reset_policy(d, tb[TCA_DEF_DATA], parm);
> + err = reset_policy(d, tb[TCA_DEF_DATA], parm, tp, extack);
> + if (err)
> + goto release_idr;
> }
>
> if (ret == ACT_P_CREATED)
> tcf_idr_insert(tn, *a);
> return ret;
> +
> +release_idr:
> + tcf_idr_release(*a, bind);
> + return err;
> }
>
> static int tcf_simp_dump(struct sk_buff *skb, struct tc_action *a,
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
> index e89a7aa4012d..8e8c1ae12260 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/simple.json
> @@ -126,5 +126,30 @@
> "teardown": [
> ""
> ]
> + },
> + {
> + "id": "b776",
> + "name": "Replace simple action with invalid goto chain control",
> + "category": [
> + "actions",
> + "simple"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action simple",
> + 0,
> + 1,
> + 255
> + ],
> + "$TC actions add action simple sdata \"hello\" pass index 90"
> + ],
> + "cmdUnderTest": "$TC actions replace action simple sdata \"world\" goto chain 42 index 90 cookie c1a0c1a0",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions list action simple",
> + "matchPattern": "action order [0-9]*: Simple <hello>.*index 90 ref",
> + "matchCount": "1",
> + "teardown": [
> + "$TC actions flush action simple"
> + ]
> }
> ]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 13/16] net/sched: act_skbedit: validate the control action inside init()
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
@ 2019-03-01 16:43 ` Vlad Buslov
0 siblings, 0 replies; 30+ messages in thread
From: Vlad Buslov @ 2019-03-01 16:43 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko, pabeni, netdev
On Wed 27 Feb 2019 at 17:40, Davide Caratti <dcaratti@redhat.com> wrote:
> the following script:
>
> # tc qdisc add dev crash0 clsact
> # tc filter add dev crash0 egress matchall \
> > action skbedit ptype host pass index 90
> # tc actions replace action skbedit \
> > ptype host goto chain 42 index 90 cookie c1a0c1a0
> # tc actions show action skbedit
>
> had the following output:
>
> Error: Failed to init TC action chain.
> We have an error talking to the kernel
> total acts 1
>
> action order 0: skbedit ptype host goto chain 42
> index 90 ref 2 bind 1
> cookie c1a0c1a0
>
> Then, the first packet transmitted by crash0 made the kernel crash:
>
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> #PF error: [normal kernel read fault]
> PGD 0 P4D 0
> Oops: 0000 [#1] SMP PTI
> CPU: 3 PID: 3467 Comm: kworker/3:3 Not tainted 5.0.0-rc4.gotochain_crash+ #536
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Workqueue: ipv6_addrconf addrconf_dad_work
> RIP: 0010:tcf_action_exec+0xb8/0x100
> Code: 00 00 00 20 74 1d 83 f8 03 75 09 49 83 c4 08 4d 39 ec 75 bc 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3 49 8b 97 a8 00 00 00 <48> 8b 12 48 89 55 00 48 83 c4 10 5b 5d 41 5c 41 5d 41 5e 41 5f c3
> RSP: 0018:ffffb50a81e1fad0 EFLAGS: 00010246
> RAX: 000000002000002a RBX: ffff9aa47ba4ea00 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: ffff9aa469eeb3c0 RDI: ffff9aa47ba4ea00
> RBP: ffffb50a81e1fb70 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: ffff9aa47bce0638 R12: ffff9aa4793b0c00
> R13: ffff9aa4793b0c08 R14: 0000000000000001 R15: ffff9aa469eeb3c0
> FS: 0000000000000000(0000) GS:ffff9aa474780000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000007360e005 CR4: 00000000001606e0
> Call Trace:
> tcf_classify+0x58/0x120
> __dev_queue_xmit+0x40a/0x890
> ? ndisc_next_option+0x50/0x50
> ? ___neigh_create+0x4d5/0x680
> ? ip6_finish_output2+0x1b5/0x590
> ip6_finish_output2+0x1b5/0x590
> ? ip6_output+0x68/0x110
> ip6_output+0x68/0x110
> ? nf_hook.constprop.28+0x79/0xc0
> ndisc_send_skb+0x248/0x2e0
> ndisc_send_ns+0xf8/0x200
> ? addrconf_dad_work+0x389/0x4b0
> addrconf_dad_work+0x389/0x4b0
> ? __switch_to_asm+0x34/0x70
> ? process_one_work+0x195/0x380
> ? addrconf_dad_completed+0x370/0x370
> process_one_work+0x195/0x380
> worker_thread+0x30/0x390
> ? process_one_work+0x380/0x380
> kthread+0x113/0x130
> ? kthread_park+0x90/0x90
> ret_from_fork+0x35/0x40
> Modules linked in: act_skbedit veth ip6table_filter ip6_tables iptable_filter binfmt_misc crct10dif_pclmul crc32_pclmul ghash_clmulni_intel ext4 snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep mbcache snd_hda_core jbd2 snd_seq snd_seq_device snd_pcm aesni_intel crypto_simd cryptd snd_timer glue_helper snd joydev soundcore pcspkr virtio_balloon i2c_piix4 nfsd auth_rpcgss nfs_acl lockd grace sunrpc ip_tables xfs ata_generic pata_acpi qxl drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm virtio_net net_failover drm failover virtio_blk virtio_console ata_piix virtio_pci crc32c_intel serio_raw libata virtio_ring virtio floppy dm_mirror dm_region_hash dm_log dm_mod
> CR2: 0000000000000000
>
> Validating the control action within tcf_skbedit_init() proved to fix the
> above issue. A TDC selftest is added to verify the correct behavior.
>
> Fixes: db50514f9a9c ("net: sched: add termination action to allow goto chain")
> Fixes: 97763dc0f401 ("net_sched: reject unknown tcfa_action values")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
> net/sched/act_skbedit.c | 22 +++++++++++++---
> .../tc-testing/tc-tests/actions/skbedit.json | 25 +++++++++++++++++++
> 2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index 9a8a0f2d4418..5a0d35b2dac3 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -26,6 +26,7 @@
> #include <net/ip.h>
> #include <net/ipv6.h>
> #include <net/dsfield.h>
> +#include <net/pkt_cls.h>
>
> #include <linux/tc_act/tc_skbedit.h>
> #include <net/tc_act/tc_skbedit.h>
> @@ -100,6 +101,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> struct netlink_ext_ack *extack)
> {
> struct tc_action_net *tn = net_generic(net, skbedit_net_id);
> + struct tcf_chain *newchain = NULL, *oldchain;
> struct tcf_skbedit_params *params_new;
> struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
> struct tc_skbedit *parm;
> @@ -187,12 +189,13 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> return -EEXIST;
> }
> }
> + err = tcf_action_check_ctrlact(parm->action, tp, &newchain, extack);
> + if (err)
> + goto release_idr;
>
> params_new = kzalloc(sizeof(*params_new), GFP_KERNEL);
> - if (unlikely(!params_new)) {
> - tcf_idr_release(*a, bind);
> - return -ENOMEM;
> - }
> + if (unlikely(!params_new))
> + goto put_chain;
Set err=-ENOMEM before goto.
>
> params_new->flags = flags;
> if (flags & SKBEDIT_F_PRIORITY)
> @@ -209,6 +212,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> params_new->mask = *mask;
>
> spin_lock_bh(&d->tcf_lock);
> + oldchain = d->tcf_goto_chain;
> + d->tcf_goto_chain = newchain;
> d->tcf_action = parm->action;
> rcu_swap_protected(d->params, params_new,
> lockdep_is_held(&d->tcf_lock));
> @@ -218,7 +223,16 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
>
> if (ret == ACT_P_CREATED)
> tcf_idr_insert(tn, *a);
> + if (oldchain)
> + tcf_chain_put_by_act(oldchain);
> return ret;
> +
> +put_chain:
> + if (newchain)
> + tcf_chain_put_by_act(newchain);
> +release_idr:
> + tcf_idr_release(*a, bind);
> + return err;
> }
>
> static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json b/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
> index 5aaf593b914a..ecd96eda7f6a 100644
> --- a/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/skbedit.json
> @@ -484,5 +484,30 @@
> "teardown": [
> "$TC actions flush action skbedit"
> ]
> + },
> + {
> + "id": "1b2b",
> + "name": "Replace skbedit action with invalid goto_chain control",
> + "category": [
> + "actions",
> + "skbedit"
> + ],
> + "setup": [
> + [
> + "$TC actions flush action skbedit",
> + 0,
> + 1,
> + 255
> + ],
> + "$TC actions add action skbedit ptype host pass index 90"
> + ],
> + "cmdUnderTest": "$TC actions replace action skbedit ptype host goto chain 42 index 90 cookie c1a0c1a0",
> + "expExitCode": "255",
> + "verifyCmd": "$TC actions list action skbedit",
> + "matchPattern": "action order [0-9]*: skbedit ptype host pass.*index 90 ref",
> + "matchCount": "1",
> + "teardown": [
> + "$TC actions flush action skbedit"
> + ]
> }
> ]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action
2019-02-28 1:28 ` Cong Wang
@ 2019-03-01 17:59 ` Davide Caratti
0 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-03-01 17:59 UTC (permalink / raw)
To: Cong Wang
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
hello Cong, thanks for reviewing.
On Wed, 2019-02-27 at 17:28 -0800, Cong Wang wrote:
> On Wed, Feb 27, 2019 at 9:41 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > +int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
> > + struct tcf_chain **handle,
>
> Please use a better name than 'handle'. 'handle' is usually used
> for a hex numeric ID. Here you just want to save the allocated
> tcf_chain to this address.
ok, understood. I will use 'newchain', like done elsewhere.
> + *handle = tcf_chain_get_by_act(tp->chain->block, chain_index);
> > + if (!*handle) {
> > + ret = -ENOMEM;
>
> Is -ENOMEM okay here? I feel like it should be -ENOSPC or whatever
> tcf_chain_get_by_act() says.
tcf_chain_by_act() calls __tcf_chain_get() with 'create' equal to true.
So, if a chain with the given index does not exist, tcf_chain_create()
will try to create it - and there, the only possible failure I see is
kzalloc().
That's why I think -ENOMEM is ok, at least for the current net/sched code.
--
davide
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-02-28 1:50 ` Cong Wang
@ 2019-03-01 18:02 ` Davide Caratti
2019-03-02 0:04 ` Cong Wang
0 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-03-01 18:02 UTC (permalink / raw)
To: Cong Wang
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > + if (oldchain)
> > + tcf_chain_put_by_act(oldchain);
>
> Do we need to respect RCU grace period here?
if I well understand the question, you are worried about
tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
decrease refcounts and eventually call kfree(oldchain).
But this would result in a use-after-free only in case the chain has only
refcount held by 1 action (the one we are overwriting), and 0 filters: is
this a condition where packets can go through this action's data plane?
In every other case, the chain is refcounted at least by 1 filter.
So, normally the worst case would be a packet routed on the wrong chain,
which is not much different than what already happens now when a valid
'goto chain' rule is overwritten with another valid 'goto chain' rule.
Am I missing something?
thank you in advance,
--
davide
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-03-01 18:02 ` Davide Caratti
@ 2019-03-02 0:04 ` Cong Wang
2019-03-07 13:56 ` Davide Caratti
0 siblings, 1 reply; 30+ messages in thread
From: Cong Wang @ 2019-03-02 0:04 UTC (permalink / raw)
To: Davide Caratti
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti <dcaratti@redhat.com> wrote:
>
> On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > + if (oldchain)
> > > + tcf_chain_put_by_act(oldchain);
> >
> > Do we need to respect RCU grace period here?
>
> if I well understand the question, you are worried about
> tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
> overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
> decrease refcounts and eventually call kfree(oldchain).
>
> But this would result in a use-after-free only in case the chain has only
> refcount held by 1 action (the one we are overwriting), and 0 filters: is
> this a condition where packets can go through this action's data plane?
Hmm? Isn't goto chain can be arbitrary? Packets can be routed
from this action to any filter chain, so even if the target chain has 0
filter this action still has traffic as long as itself is not on the same
chain?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-03-02 0:04 ` Cong Wang
@ 2019-03-07 13:56 ` Davide Caratti
2019-03-07 14:51 ` Vlad Buslov
0 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-03-07 13:56 UTC (permalink / raw)
To: Cong Wang
Cc: David S. Miller, Jamal Hadi Salim, Jiri Pirko, Vlad Buslov,
Paolo Abeni, Linux Kernel Network Developers
On Fri, 2019-03-01 at 16:04 -0800, Cong Wang wrote:
> > On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti <dcaratti@redhat.com> wrote:
> >
> > if I well understand the question, you are worried about
> > tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
> > overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
> > decrease refcounts and eventually call kfree(oldchain).
> >
> > But this would result in a use-after-free only in case the chain has only
> > refcount held by 1 action (the one we are overwriting), and 0 filters: is
> > this a condition where packets can go through this action's data plane?
>
> Hmm? Isn't goto chain can be arbitrary? Packets can be routed
> from this action to any filter chain, so even if the target chain has 0
> filter this action still has traffic as long as itself is not on the same
> chain?
hi,
sorry for the delay: it took some time to verify experimentally if we
really need this or not. So, we want to ensure the control path doesn't do
tcf_csum_init(..., ovr=1, ...)
tcf_chain_put_by_act(oldchain)
tcf_chain_destroy(oldchain, ...)
kfree(oldchain);
while the traffic path of the action is doing
res->goto_tp = rcu_dereference_bh(oldchain->filter_chain);
I iterated this script many times on a KASAN kernel,
# tc chain add dev crash0 chain 42 ingress protocol ip flower ip_proto udp action pass
# tc filter add dev crash0 ingress protocol ip flower ip_proto udp action csum udp goto chain 42 index 66
# tc chain del dev crash0 chain 42 ingress
(start netperf traffic)
# tc action replace action csum udp pass index 66
and reproduced twice the following splat:
==================================================================
BUG: KASAN: null-ptr-deref in tcf_action_exec+0x181/0x200
Read of size 8 at addr 0000000000000000 by task netperf/5777
CPU: 0 PID: 5777 Comm: netperf Not tainted 5.0.0-rc7.goto_chain_fix+ #551
Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
Call Trace:
<IRQ>
dump_stack+0xc7/0x15b
? show_regs_print_info+0x5/0x5
? _raw_read_lock_irq+0x40/0x40
? tcf_action_exec+0x181/0x200
? tcf_action_exec+0x181/0x200
kasan_report+0x176/0x192
? tcf_action_exec+0x181/0x200
tcf_action_exec+0x181/0x200
? find_dump_kind+0x170/0x170
? rcu_irq_exit+0x153/0x210
fl_classify+0x81a/0x830
so, I think that the answer to your question:
On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > > + if (oldchain)
> > > > + tcf_chain_put_by_act(oldchain);
> > >
> > > Do we need to respect RCU grace period here?
is a "yes, we do".
Now I'm trying something similar to what's done in tcf_bpf_init(), to
release the bpf program on 'replace' operations:
365 if (res == ACT_P_CREATED) {
366 tcf_idr_insert(tn, *act);
367 } else {
368 /* make sure the program being replaced is no longer executing */
369 synchronize_rcu();
370 tcf_bpf_cfg_cleanup(&old);
371 }
do you think it's worth going in this direction?
thank you in advance!
--
davide
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-03-07 13:56 ` Davide Caratti
@ 2019-03-07 14:51 ` Vlad Buslov
2019-03-07 16:56 ` Davide Caratti
0 siblings, 1 reply; 30+ messages in thread
From: Vlad Buslov @ 2019-03-07 14:51 UTC (permalink / raw)
To: Davide Caratti
Cc: Cong Wang, David S. Miller, Jamal Hadi Salim, Jiri Pirko,
Paolo Abeni, Linux Kernel Network Developers
On Thu 07 Mar 2019 at 15:56, Davide Caratti <dcaratti@redhat.com> wrote:
> On Fri, 2019-03-01 at 16:04 -0800, Cong Wang wrote:
>> > On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti <dcaratti@redhat.com> wrote:
>> >
>> > if I well understand the question, you are worried about
>> > tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
>> > overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
>> > decrease refcounts and eventually call kfree(oldchain).
>> >
>> > But this would result in a use-after-free only in case the chain has only
>> > refcount held by 1 action (the one we are overwriting), and 0 filters: is
>> > this a condition where packets can go through this action's data plane?
>>
>> Hmm? Isn't goto chain can be arbitrary? Packets can be routed
>> from this action to any filter chain, so even if the target chain has 0
>> filter this action still has traffic as long as itself is not on the same
>> chain?
>
> hi,
>
> sorry for the delay: it took some time to verify experimentally if we
> really need this or not. So, we want to ensure the control path doesn't do
>
> tcf_csum_init(..., ovr=1, ...)
> tcf_chain_put_by_act(oldchain)
> tcf_chain_destroy(oldchain, ...)
> kfree(oldchain);
>
> while the traffic path of the action is doing
>
> res->goto_tp = rcu_dereference_bh(oldchain->filter_chain);
>
> I iterated this script many times on a KASAN kernel,
>
> # tc chain add dev crash0 chain 42 ingress protocol ip flower ip_proto udp action pass
> # tc filter add dev crash0 ingress protocol ip flower ip_proto udp action csum udp goto chain 42 index 66
> # tc chain del dev crash0 chain 42 ingress
> (start netperf traffic)
> # tc action replace action csum udp pass index 66
>
> and reproduced twice the following splat:
>
> ==================================================================
> BUG: KASAN: null-ptr-deref in tcf_action_exec+0x181/0x200
> Read of size 8 at addr 0000000000000000 by task netperf/5777
>
> CPU: 0 PID: 5777 Comm: netperf Not tainted 5.0.0-rc7.goto_chain_fix+ #551
> Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> Call Trace:
> <IRQ>
> dump_stack+0xc7/0x15b
> ? show_regs_print_info+0x5/0x5
> ? _raw_read_lock_irq+0x40/0x40
> ? tcf_action_exec+0x181/0x200
> ? tcf_action_exec+0x181/0x200
> kasan_report+0x176/0x192
> ? tcf_action_exec+0x181/0x200
> tcf_action_exec+0x181/0x200
> ? find_dump_kind+0x170/0x170
> ? rcu_irq_exit+0x153/0x210
> fl_classify+0x81a/0x830
>
> so, I think that the answer to your question:
>
> On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
>> > > > + if (oldchain)
>> > > > + tcf_chain_put_by_act(oldchain);
>> > >
>> > > Do we need to respect RCU grace period here?
>
> is a "yes, we do".
> Now I'm trying something similar to what's done in tcf_bpf_init(), to
> release the bpf program on 'replace' operations:
>
> 365 if (res == ACT_P_CREATED) {
> 366 tcf_idr_insert(tn, *act);
> 367 } else {
> 368 /* make sure the program being replaced is no longer executing */
> 369 synchronize_rcu();
> 370 tcf_bpf_cfg_cleanup(&old);
> 371 }
>
> do you think it's worth going in this direction?
> thank you in advance!
Hi Davide,
Using synchronize_rcu() will impact rule update rate performance and I
don't think we really need it. I don't see any reason why we can't just
update chain to be rcu-friendly. Data path is already rcu_read
protected, in fact it only needs chain to read rcu-pointer to tp list
when jumping to chain. So it should be enough to do the following:
1) Update tcf_chain_destroy() to free chain after rcu grace period.
2) Convert tc_action->goto_chain to be a proper rcu pointer. (mark it
with "__rcu", assign with rcu_assign_pointer(), read it with
rcu_dereference{_bh}(), etc.)
Am I missing anything?
Regards,
Vlad
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-03-07 14:51 ` Vlad Buslov
@ 2019-03-07 16:56 ` Davide Caratti
2019-03-08 17:47 ` Davide Caratti
0 siblings, 1 reply; 30+ messages in thread
From: Davide Caratti @ 2019-03-07 16:56 UTC (permalink / raw)
To: Vlad Buslov
Cc: Cong Wang, David S. Miller, Jamal Hadi Salim, Jiri Pirko,
Paolo Abeni, Linux Kernel Network Developers
On Thu, 2019-03-07 at 14:51 +0000, Vlad Buslov wrote:
[...]
> On Thu 07 Mar 2019 at 15:56, Davide Caratti <dcaratti@redhat.com> wrote:
> > so, I think that the answer to your question:
> >
> > On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > > > > + if (oldchain)
> > > > > > + tcf_chain_put_by_act(oldchain);
> > > > >
> > > > > Do we need to respect RCU grace period here?
> >
> > is a "yes, we do".
> > Now I'm trying something similar to what's done in tcf_bpf_init(), to
> > release the bpf program on 'replace' operations:
> >
> > 365 if (res == ACT_P_CREATED) {
> > 366 tcf_idr_insert(tn, *act);
> > 367 } else {
> > 368 /* make sure the program being replaced is no longer executing */
> > 369 synchronize_rcu();
> > 370 tcf_bpf_cfg_cleanup(&old);
> > 371 }
> >
> > do you think it's worth going in this direction?
> > thank you in advance!
>
> Hi Davide,
>
> Using synchronize_rcu() will impact rule update rate performance and I
> don't think we really need it.
Ok; consider that, on current kernel, chains are not being freed/de-
refcounted at all when TC actions are updated. So, the update rate
performance is going to drop anyway - because of the weight of
tcf_chain_put_by_act() we are forgetting to call now.
Only if synchronize_rcu() takes a number of cycles which is comparable (or
much greater than) tcf_chain_put_by_act(), then it makes sense to RCU-ify
a->tcf_goto_chain.
> I don't see any reason why we can't just
> update chain to be rcu-friendly. Data path is already rcu_read
> protected, in fact it only needs chain to read rcu-pointer to tp list
> when jumping to chain. So it should be enough to do the following:
>
> 1) Update tcf_chain_destroy() to free chain after rcu grace period.
>
> 2) Convert tc_action->goto_chain to be a proper rcu pointer. (mark it
> with "__rcu", assign with rcu_assign_pointer(), read it with
> rcu_dereference{_bh}(), etc.)
it seems feasible, with some attention points:
1) replacing the 'goto chain' in the init() function will then become
rcu_swap_protected(p->tcf_goto_chain, newchain,
lockdep_is_held(&p->tcf_lock));
with p->tcf_lock held, and we will have to do this unconditionally also on
non-update paths (it should have the same cost in CPU cycles as the rcu
init / assign code).
Unlike the synchronize_rcu(), that would only happen only in the update
path of goto_chain actions, this is a fee that we pay in every path
2) in tcf_action_goto_chain_exec(), we would have two "cascaded"
rcu_dereference(), action->chain and chain->filter. Is this design
acceptable?
thanks,
--
davide
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()
2019-03-07 16:56 ` Davide Caratti
@ 2019-03-08 17:47 ` Davide Caratti
0 siblings, 0 replies; 30+ messages in thread
From: Davide Caratti @ 2019-03-08 17:47 UTC (permalink / raw)
To: Vlad Buslov
Cc: Cong Wang, David S. Miller, Jamal Hadi Salim, Jiri Pirko,
Paolo Abeni, Linux Kernel Network Developers
On Thu, 2019-03-07 at 17:56 +0100, Davide Caratti wrote:
> On Thu, 2019-03-07 at 14:51 +0000, Vlad Buslov wrote:
>
> [...]
hi Vlad,
> > On Thu 07 Mar 2019 at 15:56, Davide Caratti <dcaratti@redhat.com> wrote:
> > > so, I think that the answer to your question:
> > >
> > > On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > > > > > + if (oldchain)
> > > > > > > + tcf_chain_put_by_act(oldchain);
> > > > > >
> > > > > > Do we need to respect RCU grace period here?
> > >
> > > is a "yes, we do".
> > > Now I'm trying something similar to what's done in tcf_bpf_init(), to
> > > release the bpf program on 'replace' operations:
> > >
> > > 365 if (res == ACT_P_CREATED) {
> > > 366 tcf_idr_insert(tn, *act);
> > > 367 } else {
> > > 368 /* make sure the program being replaced is no longer executing */
> > > 369 synchronize_rcu();
> > > 370 tcf_bpf_cfg_cleanup(&old);
> > > 371 }
> > >
> > > do you think it's worth going in this direction?
> > > thank you in advance!
> >
> > Hi Davide,
> >
> > Using synchronize_rcu() will impact rule update rate performance and I
> > don't think we really need it.
... and moreover it doesn't seem to fix anything with my KASAN kernel when
the action is replaced with netperf running. I still see the same splat in
tcf_action_goto_chain_exec().
Probably the reason is that the "bad pointer" is chain, not chain->filter.
I will experiment rcu-ifiying 'goto_chain', like you proposed below.
> > I don't see any reason why we can't just
> > update chain to be rcu-friendly. Data path is already rcu_read
> > protected, in fact it only needs chain to read rcu-pointer to tp list
> > when jumping to chain. So it should be enough to do the following:
> >
> > 1) Update tcf_chain_destroy() to free chain after rcu grace period.
> >
> > 2) Convert tc_action->goto_chain to be a proper rcu pointer. (mark it
> > with "__rcu", assign with rcu_assign_pointer(), read it with
> > rcu_dereference{_bh}(), etc.)
>
> it seems feasible, with some attention points:
>
> 1) replacing the 'goto chain' in the init() function will then become
>
> rcu_swap_protected(p->tcf_goto_chain, newchain,
> lockdep_is_held(&p->tcf_lock));
>
> with p->tcf_lock held, and we will have to do this unconditionally also on
> non-update paths (it should have the same cost in CPU cycles as the rcu
> init / assign code).
> Unlike the synchronize_rcu(), that would only happen only in the update
> path of goto_chain actions, this is a fee that we pay in every path
>
> 2) in tcf_action_goto_chain_exec(), we would have two "cascaded"
> rcu_dereference(), action->chain and chain->filter. Is this design
> acceptable?
thanks!
--
davide
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-03-08 17:47 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 17:40 [PATCH net 00/16] validate the control action with all the other parameters Davide Caratti
2019-02-27 17:40 ` [PATCH net 01/16] net/sched: prepare TC actions to properly validate the control action Davide Caratti
2019-02-28 1:28 ` Cong Wang
2019-03-01 17:59 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 02/16] net/sched: act_bpf: validate the control action inside init() Davide Caratti
2019-02-27 17:40 ` [PATCH net 03/16] net/sched: act_csum: " Davide Caratti
2019-02-28 1:50 ` Cong Wang
2019-03-01 18:02 ` Davide Caratti
2019-03-02 0:04 ` Cong Wang
2019-03-07 13:56 ` Davide Caratti
2019-03-07 14:51 ` Vlad Buslov
2019-03-07 16:56 ` Davide Caratti
2019-03-08 17:47 ` Davide Caratti
2019-02-27 17:40 ` [PATCH net 04/16] net/sched: act_gact: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 05/16] net/sched: act_ife: " Davide Caratti
2019-03-01 16:33 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 06/16] net/sched: act_mirred: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 07/16] net/sched: act_connmark: " Davide Caratti
2019-03-01 16:35 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 08/16] net/sched: act_nat: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 09/16] net/sched: act_pedit: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 10/16] net/sched: act_police: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 11/16] net/sched: act_sample: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 12/16] net/sched: act_simple: " Davide Caratti
2019-03-01 16:39 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 13/16] net/sched: act_skbedit: " Davide Caratti
2019-03-01 16:43 ` Vlad Buslov
2019-02-27 17:40 ` [PATCH net 14/16] net/sched: act_skbmod: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 15/16] net/sched: act_tunnel_key: " Davide Caratti
2019-02-27 17:40 ` [PATCH net 16/16] net/sched: act_vlan: " Davide Caratti
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.