Re: [PATCH net-next v3 2/5] net/sched: user-space can't set unknown tcfa_action values

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jul 24, 2018 at 10:06:40PM CEST, pabeni@redhat.com wrote:
>Currently, when initializing an action, the user-space can specify
>and use arbitrary values for the tcfa_action field. If the value
>is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC.
>
>This change explicitly checks for unknown values at action creation
>time, and explicitly convert them to TC_ACT_UNSPEC. No functional
>changes are introduced, but this will allow introducing tcfa_action
>values not exposed to user-space in a later patch.
>
>Note: we can't use the above to hide TC_ACT_REDIRECT from user-space,
>as the latter is already part of uAPI.
>
>Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>---
> include/uapi/linux/pkt_cls.h |  6 ++++--
> net/sched/act_api.c          | 10 +++++++++-
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
>diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
>index c4262d911596..c8a24861d4c8 100644
>--- a/include/uapi/linux/pkt_cls.h
>+++ b/include/uapi/linux/pkt_cls.h
>@@ -45,6 +45,7 @@ enum {
> 				   * the skb and act like everything
> 				   * is alright.
> 				   */
>+#define TC_ACT_VALUE_MAX	TC_ACT_TRAP
> 
> /* There is a special kind of actions called "extended actions",
>  * which need a value parameter. These have a local opcode located in
>@@ -55,11 +56,12 @@ enum {
> #define __TC_ACT_EXT_SHIFT 28
> #define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT)
> #define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1)
>-#define TC_ACT_EXT_CMP(combined, opcode) \
>-	(((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode)
>+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK))
>+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode)
> 
> #define TC_ACT_JUMP __TC_ACT_EXT(1)
> #define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
>+#define TC_ACT_EXT_OPCODE_MAX	TC_ACT_GOTO_CHAIN
> 
> /* Action type identifiers*/
> enum {
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 24b5534967fe..5044f4809b37 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> 	char act_name[IFNAMSIZ];
> 	struct nlattr *tb[TCA_ACT_MAX + 1];
> 	struct nlattr *kind;
>+	int opcode;
> 	int err;
> 
> 	if (name == NULL) {
>@@ -884,7 +885,8 @@ 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)) {
>+	opcode = TC_ACT_EXT_OPCODE(a->tcfa_action);
>+	if (opcode == TC_ACT_GOTO_CHAIN) {
> 		err = tcf_action_goto_chain_init(a, tp);
> 		if (err) {
> 			struct tc_action *actions[] = { a, NULL };
>@@ -898,6 +900,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
> 	if (a->tcfa_action == TC_ACT_REDIRECT) {
> 		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");
> 		a->tcfa_action = TC_ACT_UNSPEC;
>+	} else if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) ||
>+		   (opcode && opcode > TC_ACT_EXT_OPCODE_MAX &&
>+		    a->tcfa_action != TC_ACT_UNSPEC)) {
>+		net_warn_ratelimited("invalid %d action value",
>+				     a->tcfa_action);
>+		a->tcfa_action = TC_ACT_UNSPEC;

Maybe this could be a separate helper function?

Also, the warn might go along with extack to user too.

Otherwise, this looks fine to me.


> 	}
> 
> 	return a;
>-- 
>2.17.1
>