| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Mar 2018 19:36:16 +0100
From: Davide Caratti <dcaratti@...hat.com>
To: Stephen Hemminger <sthemmin@...rosoft.com>,
Jiri Pirko <jiri@...lanox.com>
Cc: netdev@...r.kernel.org, Wolfgang Bumiller <w.bumiller@...xmox.com>,
Michal Privoznik <mprivozn@...hat.com>
Subject: [PATCH iproute2] tc: fix parsing of the control action
If the user didn't specify any control action, don't pop the command line
arguments: otherwise, parsing of the next argument (tipically the 'index'
keyword) results in an error, causing the following 'tc-testing' failures:
Test a6d6: Add skbedit action with index
Test 38f3: Delete skbedit action
Test a568: Add action with ife type
Test b983: Add action without ife type
Test 7d50: Add skbmod action to set destination mac
Test 9b29: Add skbmod action to set source mac
Test e93a: Delete an skbmod action
Also, add missing parse for 'ok' control action to m_police, to fix the
following 'tc-testing' failure:
Test 8dd5: Add police action with control ok
tested with:
# ./tdc.py
test results:
all tests ok using kernel 4.16-rc2, except 9aa8 "Get a single skbmod
action from a list" (which is failing also before this commit)
Fixes: 3572e01a090a ("tc: util: Don't call NEXT_ARG_FWD() in __parse_action_control()")
Cc: Michal Privoznik <mprivozn@...hat.com>
Cc: Wolfgang Bumiller <w.bumiller@...xmox.com>
Signed-off-by: Davide Caratti <dcaratti@...hat.com>
---
Notes:
hello Stephen,
while running the tdc.py script in 'tc-testing' folder of latest kernel
tree, I observed some systematic failures. Most of them (not all TBH) are
caused by commit 3572e01a090a ("tc: util: Don't call NEXT_ARG_FWD() in
__parse_action_control()).
AFAIU, the current code wrongly assumes that the callers of this function
consume exactly one item of argv list: because of that, it ensures that a
successful call to parse_action_ctrl{,_dflt}() is followed by NEXT_ARG_FWD.
However, this assumption is not always true _ at least, it's not true for
parse_action_ctrl_dflt(), just because specifying an explicit control
action is optional in TC rules, most of the times.
Therefore, a call to NEXT_ARG_FWD() after parse_action_control{,_dflt}()
returned 0 is a fix for some m_police use cases, but it's breaking other
use cases at the same time.
I made a partial revert of the above commit, trying to ensure that the
helpers modify the argument list by the correct quantity, and this patch
is the result. But then, looking at linux-netdev archives, I observed
that both Michal and Wolfgang initially proposed to fix m_police in a
similar way [1][2] _ so, the result of this work is almost equivalent to
reverting Michal's patch, and applying his V1: that's why I'm asking
if you want to resume the whole discussion.
any feedback is appreciated, thank you in advance!
[1] https://www.spinics.net/lists/netdev/msg468479.html
[2] https://www.spinics.net/lists/netdev/msg481407.html
tc/m_bpf.c | 1 -
tc/m_connmark.c | 1 -
tc/m_csum.c | 1 -
tc/m_gact.c | 9 +++------
tc/m_ife.c | 1 -
tc/m_mirred.c | 5 ++---
tc/m_nat.c | 1 -
tc/m_pedit.c | 1 -
tc/m_police.c | 16 ++++++++++------
tc/m_sample.c | 1 -
tc/m_skbedit.c | 1 -
tc/m_skbmod.c | 1 -
tc/m_tunnel_key.c | 1 -
tc/m_vlan.c | 1 -
tc/tc_util.c | 6 +++++-
15 files changed, 20 insertions(+), 27 deletions(-)
diff --git a/tc/m_bpf.c b/tc/m_bpf.c
index 576f69cc..1c1f71cd 100644
--- a/tc/m_bpf.c
+++ b/tc/m_bpf.c
@@ -129,7 +129,6 @@ opt_bpf:
parse_action_control_dflt(&argc, &argv, &parm.action,
false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
diff --git a/tc/m_connmark.c b/tc/m_connmark.c
index 47c7a8c2..37d71854 100644
--- a/tc/m_connmark.c
+++ b/tc/m_connmark.c
@@ -82,7 +82,6 @@ parse_connmark(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
}
parse_action_control_dflt(&argc, &argv, &sel.action, false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
diff --git a/tc/m_csum.c b/tc/m_csum.c
index e1352c08..7b156734 100644
--- a/tc/m_csum.c
+++ b/tc/m_csum.c
@@ -124,7 +124,6 @@ parse_csum(struct action_util *a, int *argc_p,
}
parse_action_control_dflt(&argc, &argv, &sel.action, false, TC_ACT_OK);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
diff --git a/tc/m_gact.c b/tc/m_gact.c
index b30b0420..16c4413f 100644
--- a/tc/m_gact.c
+++ b/tc/m_gact.c
@@ -87,12 +87,10 @@ parse_gact(struct action_util *a, int *argc_p, char ***argv_p,
if (argc < 0)
return -1;
- if (matches(*argv, "gact") != 0 &&
- parse_action_control(&argc, &argv, &p.action, false) == -1) {
+ if (!matches(*argv, "gact"))
+ NEXT_ARG_FWD();
+ if (parse_action_control(&argc, &argv, &p.action, false))
usage(); /* does not return */
- }
-
- NEXT_ARG_FWD();
#ifdef CONFIG_GACT_PROB
if (argc > 0) {
@@ -113,7 +111,6 @@ parse_gact(struct action_util *a, int *argc_p, char ***argv_p,
if (parse_action_control(&argc, &argv,
&pp.paction, false) == -1)
usage();
- NEXT_ARG_FWD();
if (get_u16(&pp.pval, *argv, 10)) {
fprintf(stderr,
"Illegal probability val 0x%x\n",
diff --git a/tc/m_ife.c b/tc/m_ife.c
index 4647f6a6..205efc9f 100644
--- a/tc/m_ife.c
+++ b/tc/m_ife.c
@@ -159,7 +159,6 @@ static int parse_ife(struct action_util *a, int *argc_p, char ***argv_p,
parse_action_control_dflt(&argc, &argv, &p.action, false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_mirred.c b/tc/m_mirred.c
index aa7ce6d9..14e5c88d 100644
--- a/tc/m_mirred.c
+++ b/tc/m_mirred.c
@@ -103,6 +103,7 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
while (argc > 0) {
if (matches(*argv, "action") == 0) {
+ NEXT_ARG();
break;
} else if (!egress && matches(*argv, "egress") == 0) {
egress = 1;
@@ -202,10 +203,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
}
- if (p.eaction == TCA_EGRESS_MIRROR || p.eaction == TCA_INGRESS_MIRROR) {
+ if (p.eaction == TCA_EGRESS_MIRROR || p.eaction == TCA_INGRESS_MIRROR)
parse_action_control(&argc, &argv, &p.action, false);
- NEXT_ARG_FWD();
- }
if (argc) {
if (iok && matches(*argv, "index") == 0) {
diff --git a/tc/m_nat.c b/tc/m_nat.c
index f5de4d4c..1e4ff51f 100644
--- a/tc/m_nat.c
+++ b/tc/m_nat.c
@@ -116,7 +116,6 @@ parse_nat(struct action_util *a, int *argc_p, char ***argv_p, int tca_id, struct
parse_action_control_dflt(&argc, &argv, &sel.action, false, TC_ACT_OK);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index dc57f14a..26549eee 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -672,7 +672,6 @@ int parse_pedit(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
parse_action_control_dflt(&argc, &argv, &sel.sel.action, false, TC_ACT_OK);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_police.c b/tc/m_police.c
index ff1dcb7d..055b50ee 100644
--- a/tc/m_police.c
+++ b/tc/m_police.c
@@ -150,15 +150,18 @@ int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
matches(*argv, "shot") == 0 ||
matches(*argv, "continue") == 0 ||
matches(*argv, "pass") == 0 ||
+ matches(*argv, "ok") == 0 ||
matches(*argv, "pipe") == 0 ||
matches(*argv, "goto") == 0) {
- if (parse_action_control(&argc, &argv, &p.action, false))
- return -1;
+ if (!parse_action_control(&argc, &argv, &p.action, false))
+ goto action_ctrl_ok;
+ return -1;
} else if (strcmp(*argv, "conform-exceed") == 0) {
NEXT_ARG();
- if (parse_action_control_slash(&argc, &argv, &p.action,
- &presult, true))
- return -1;
+ if (!parse_action_control_slash(&argc, &argv, &p.action,
+ &presult, true))
+ goto action_ctrl_ok;
+ return -1;
} else if (matches(*argv, "overhead") == 0) {
NEXT_ARG();
if (get_u16(&overhead, *argv, 10)) {
@@ -174,8 +177,9 @@ int act_parse_police(struct action_util *a, int *argc_p, char ***argv_p,
} else {
break;
}
+ NEXT_ARG_FWD();
+action_ctrl_ok:
ok++;
- argc--; argv++;
}
if (!ok)
diff --git a/tc/m_sample.c b/tc/m_sample.c
index 31774c0e..ff5ee6bd 100644
--- a/tc/m_sample.c
+++ b/tc/m_sample.c
@@ -100,7 +100,6 @@ static int parse_sample(struct action_util *a, int *argc_p, char ***argv_p,
parse_action_control_dflt(&argc, &argv, &p.action, false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_skbedit.c b/tc/m_skbedit.c
index c41a7bb0..aa374fcb 100644
--- a/tc/m_skbedit.c
+++ b/tc/m_skbedit.c
@@ -123,7 +123,6 @@ parse_skbedit(struct action_util *a, int *argc_p, char ***argv_p, int tca_id,
parse_action_control_dflt(&argc, &argv, &sel.action,
false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_skbmod.c b/tc/m_skbmod.c
index bc268dfd..561b73fb 100644
--- a/tc/m_skbmod.c
+++ b/tc/m_skbmod.c
@@ -124,7 +124,6 @@ static int parse_skbmod(struct action_util *a, int *argc_p, char ***argv_p,
parse_action_control_dflt(&argc, &argv, &p.action, false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_tunnel_key.c b/tc/m_tunnel_key.c
index 2dc91879..1cdd0356 100644
--- a/tc/m_tunnel_key.c
+++ b/tc/m_tunnel_key.c
@@ -175,7 +175,6 @@ static int parse_tunnel_key(struct action_util *a, int *argc_p, char ***argv_p,
parse_action_control_dflt(&argc, &argv, &parm.action,
false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/m_vlan.c b/tc/m_vlan.c
index edae0d1e..161759fd 100644
--- a/tc/m_vlan.c
+++ b/tc/m_vlan.c
@@ -131,7 +131,6 @@ static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
parse_action_control_dflt(&argc, &argv, &parm.action,
false, TC_ACT_PIPE);
- NEXT_ARG_FWD();
if (argc) {
if (matches(*argv, "index") == 0) {
NEXT_ARG();
diff --git a/tc/tc_util.c b/tc/tc_util.c
index aceb0d94..8eadbbcf 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -588,6 +588,7 @@ static int __parse_action_control(int *argc_p, char ***argv_p, int *result_p,
}
result |= jump_cnt;
}
+ NEXT_ARG_FWD();
*argc_p = argc;
*argv_p = argv;
*result_p = result;
@@ -684,8 +685,8 @@ out:
int parse_action_control_slash(int *argc_p, char ***argv_p,
int *result1_p, int *result2_p, bool allow_num)
{
+ int result1, result2, argc = *argc_p;
char **argv = *argv_p;
- int result1, result2;
char *p = strchr(*argv, '/');
if (!p)
@@ -704,6 +705,9 @@ int parse_action_control_slash(int *argc_p, char ***argv_p,
*result1_p = result1;
*result2_p = result2;
+ NEXT_ARG_FWD();
+ *argc_p = argc;
+ *argv_p = argv;
return 0;
}
--
2.14.3
Powered by blists - more mailing lists