Commit 85d0966f authored by Davide Caratti's avatar Davide Caratti Committed by David S. Miller
Browse files

net/sched: prepare TC actions to properly validate the control action 



- pass a pointer to struct tcf_proto in each actions's init() handler,
  to allow validating the control action, checking whether the chain
  exists and (eventually) refcounting it.
- 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 NULL goto_chain pointer at
  the same time.
- add tcf_action_check_ctrlact(), that will validate the control action
  and eventually allocate the action 'goto_chain' within the init()
  handler.
- add tcf_action_set_ctrlact(), that will assign the control action and
  swap the current 'goto_chain' pointer with the new given one.

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 calling tcf_action_set_ctrlact()
to assign '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: db50514f ("net: sched: add termination action to allow goto chain")
Fixes: 97763dc0 ("net_sched: reject unknown tcfa_action values")
Signed-off-by: default avatarDavide Caratti <dcaratti@redhat.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent cd5afa91

include/net/act_api.h

+6 −1
Original line number Diff line number Diff line
@@ -90,7 +90,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 +181,11 @@ 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 *newchain);

struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,

					 struct tcf_chain *newchain);

#endif /* CONFIG_NET_CLS_ACT */



static inline void tcf_action_stats_update(struct tc_action *a, u64 bytes,

net/sched/act_api.c

+56 −41
Original line number Diff line number Diff line
@@ -28,23 +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);

}



static void tcf_action_goto_chain_exec(const struct tc_action *a,

				       struct tcf_result *res)

{
@@ -71,6 +54,53 @@ 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 **newchain,

			     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 || !newchain) {

			ret = -EINVAL;

			NL_SET_ERR_MSG(extack,

				       "can't goto NULL proto/chain");

			goto end;

		}

		*newchain = tcf_chain_get_by_act(tp->chain->block, chain_index);

		if (!*newchain) {

			ret = -ENOMEM;

			NL_SET_ERR_MSG(extack,

				       "can't allocate goto_chain");

		}

	}

end:

	return ret;

}

EXPORT_SYMBOL(tcf_action_check_ctrlact);



struct tcf_chain *tcf_action_set_ctrlact(struct tc_action *a, int action,

					 struct tcf_chain *newchain)

{

	struct tcf_chain *oldchain = a->goto_chain;



	a->tcfa_action = action;

	a->goto_chain = newchain;

	return oldchain;

}

EXPORT_SYMBOL(tcf_action_set_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
@@ -78,13 +108,15 @@ static void tcf_set_action_cookie(struct tc_cookie __rcu **old_cookie, 
 */

static void free_tcf(struct tc_action *p)

{

	struct tcf_chain *chain = p->goto_chain;



	free_percpu(p->cpu_bstats);

	free_percpu(p->cpu_bstats_hw);

	free_percpu(p->cpu_qstats);



	tcf_set_action_cookie(&p->act_cookie, NULL);

	if (p->goto_chain)

		tcf_action_goto_chain_fini(p);

	if (chain)

		tcf_chain_put_by_act(chain);



	kfree(p);

}
@@ -800,15 +832,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 +913,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,18 +930,10 @@ 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) {

		tcf_action_destroy_1(a, bind);

		NL_SET_ERR_MSG(extack, "Invalid control action value");

		NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");

		return ERR_PTR(-EINVAL);

	}

net/sched/act_bpf.c

+1 −1
Original line number Diff line number Diff line
@@ -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];

net/sched/act_connmark.c

+1 −0
Original line number Diff line number Diff line
@@ -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);

net/sched/act_csum.c

+1 −1
Original line number Diff line number Diff line
@@ -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);

CRA Git | Maintained and supported by SUSTech CRA and CCSE