| 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
| ||
|
Message-ID: <Y/o0loqC1TLzqQm8@corigine.com>
Date: Sat, 25 Feb 2023 17:17:26 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Pedro Tammela <pctammela@...atatu.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, amir@...ai.me,
dcaratti@...hat.com, willemb@...gle.com,
simon.horman@...ronome.com, john.hurley@...ronome.com,
yotamg@...lanox.com, ozsh@...dia.com, paulb@...dia.com
Subject: Re: [PATCH net 2/3] net/sched: act_mpls: fix action bind logic
On Fri, Feb 24, 2023 at 12:00:57PM -0300, Pedro Tammela wrote:
> The TC architecture allows filters and actions to be created independently.
> In filters the user can reference action objects using:
> tc action add action mpls ... index 1
> tc filter add ... action mpls index 1
>
> In the current code for act_mpls this is broken as it checks netlink
> attributes for create/update before actually checking if we are binding to an
> existing action.
>
> tdc results:
> 1..53
> ok 1 a933 - Add MPLS dec_ttl action with pipe opcode
> ok 2 08d1 - Add mpls dec_ttl action with pass opcode
> ok 3 d786 - Add mpls dec_ttl action with drop opcode
> ok 4 f334 - Add mpls dec_ttl action with reclassify opcode
> ok 5 29bd - Add mpls dec_ttl action with continue opcode
> ok 6 48df - Add mpls dec_ttl action with jump opcode
> ok 7 62eb - Add mpls dec_ttl action with trap opcode
> ok 8 09d2 - Add mpls dec_ttl action with opcode and cookie
> ok 9 c170 - Add mpls dec_ttl action with opcode and cookie of max length
> ok 10 9118 - Add mpls dec_ttl action with invalid opcode
> ok 11 6ce1 - Add mpls dec_ttl action with label (invalid)
> ok 12 352f - Add mpls dec_ttl action with tc (invalid)
> ok 13 fa1c - Add mpls dec_ttl action with ttl (invalid)
> ok 14 6b79 - Add mpls dec_ttl action with bos (invalid)
> ok 15 d4c4 - Add mpls pop action with ip proto
> ok 16 91fb - Add mpls pop action with ip proto and cookie
> ok 17 92fe - Add mpls pop action with mpls proto
> ok 18 7e23 - Add mpls pop action with no protocol (invalid)
> ok 19 6182 - Add mpls pop action with label (invalid)
> ok 20 6475 - Add mpls pop action with tc (invalid)
> ok 21 067b - Add mpls pop action with ttl (invalid)
> ok 22 7316 - Add mpls pop action with bos (invalid)
> ok 23 38cc - Add mpls push action with label
> ok 24 c281 - Add mpls push action with mpls_mc protocol
> ok 25 5db4 - Add mpls push action with label, tc and ttl
> ok 26 7c34 - Add mpls push action with label, tc ttl and cookie of max length
> ok 27 16eb - Add mpls push action with label and bos
> ok 28 d69d - Add mpls push action with no label (invalid)
> ok 29 e8e4 - Add mpls push action with ipv4 protocol (invalid)
> ok 30 ecd0 - Add mpls push action with out of range label (invalid)
> ok 31 d303 - Add mpls push action with out of range tc (invalid)
> ok 32 fd6e - Add mpls push action with ttl of 0 (invalid)
> ok 33 19e9 - Add mpls mod action with mpls label
> ok 34 1fde - Add mpls mod action with max mpls label
> ok 35 0c50 - Add mpls mod action with mpls label exceeding max (invalid)
> ok 36 10b6 - Add mpls mod action with mpls label of MPLS_LABEL_IMPLNULL (invalid)
> ok 37 57c9 - Add mpls mod action with mpls min tc
> ok 38 6872 - Add mpls mod action with mpls max tc
> ok 39 a70a - Add mpls mod action with mpls tc exceeding max (invalid)
> ok 40 6ed5 - Add mpls mod action with mpls ttl
> ok 41 77c1 - Add mpls mod action with mpls ttl and cookie
> ok 42 b80f - Add mpls mod action with mpls max ttl
> ok 43 8864 - Add mpls mod action with mpls min ttl
> ok 44 6c06 - Add mpls mod action with mpls ttl of 0 (invalid)
> ok 45 b5d8 - Add mpls mod action with mpls ttl exceeding max (invalid)
> ok 46 451f - Add mpls mod action with mpls max bos
> ok 47 a1ed - Add mpls mod action with mpls min bos
> ok 48 3dcf - Add mpls mod action with mpls bos exceeding max (invalid)
> ok 49 db7c - Add mpls mod action with protocol (invalid)
> ok 50 b070 - Replace existing mpls push action with new ID
> ok 51 95a9 - Replace existing mpls push action with new label, tc, ttl and cookie
> ok 52 6cce - Delete mpls pop action
> ok 53 d138 - Flush mpls actions
>
> Fixes: 2a2ea50870ba ("net: sched: add mpls manipulation actions to TC")
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
Minor not notwithstanding,
Reviewed-by: Simon Horman <simon.horman@...igine.com>
> ---
> net/sched/act_mpls.c | 66 +++++++++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 29 deletions(-)
>
> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> index 6b26bdb999d7..809f7928a1be 100644
> --- a/net/sched/act_mpls.c
> +++ b/net/sched/act_mpls.c
> @@ -190,40 +190,67 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> parm = nla_data(tb[TCA_MPLS_PARMS]);
> index = parm->index;
>
> + err = tcf_idr_check_alloc(tn, &index, a, bind);
> + if (err < 0)
> + return err;
> + exists = err;
> + if (exists && bind)
> + return 0;
> +
> + if (!exists) {
> + ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind,
> + true, flags);
> + if (ret) {
> + tcf_idr_cleanup(tn, index);
> + return ret;
> + }
> +
> + ret = ACT_P_CREATED;
> + } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
> + tcf_idr_release(*a, bind);
> + return -EEXIST;
nit: I think it would be more consistent to goto release_idr here.
> + }
> +
> /* Verify parameters against action type. */
> switch (parm->m_action) {
> case TCA_MPLS_ACT_POP:
> if (!tb[TCA_MPLS_PROTO]) {
> NL_SET_ERR_MSG_MOD(extack, "Protocol must be set for MPLS pop");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> if (!eth_proto_is_802_3(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
> NL_SET_ERR_MSG_MOD(extack, "Invalid protocol type for MPLS pop");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> if (tb[TCA_MPLS_LABEL] || tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] ||
> tb[TCA_MPLS_BOS]) {
> NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC or BOS cannot be used with MPLS pop");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> break;
> case TCA_MPLS_ACT_DEC_TTL:
> if (tb[TCA_MPLS_PROTO] || tb[TCA_MPLS_LABEL] ||
> tb[TCA_MPLS_TTL] || tb[TCA_MPLS_TC] || tb[TCA_MPLS_BOS]) {
> NL_SET_ERR_MSG_MOD(extack, "Label, TTL, TC, BOS or protocol cannot be used with MPLS dec_ttl");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> break;
> case TCA_MPLS_ACT_PUSH:
> case TCA_MPLS_ACT_MAC_PUSH:
> if (!tb[TCA_MPLS_LABEL]) {
> NL_SET_ERR_MSG_MOD(extack, "Label is required for MPLS push");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> if (tb[TCA_MPLS_PROTO] &&
> !eth_p_mpls(nla_get_be16(tb[TCA_MPLS_PROTO]))) {
> NL_SET_ERR_MSG_MOD(extack, "Protocol must be an MPLS type for MPLS push");
> - return -EPROTONOSUPPORT;
> + err = -EPROTONOSUPPORT;
> + goto release_idr;
> }
> /* Push needs a TTL - if not specified, set a default value. */
> if (!tb[TCA_MPLS_TTL]) {
> @@ -238,33 +265,14 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> case TCA_MPLS_ACT_MODIFY:
> if (tb[TCA_MPLS_PROTO]) {
> NL_SET_ERR_MSG_MOD(extack, "Protocol cannot be used with MPLS modify");
> - return -EINVAL;
> + err = -EINVAL;
> + goto release_idr;
> }
> break;
> default:
> NL_SET_ERR_MSG_MOD(extack, "Unknown MPLS action");
> - return -EINVAL;
> - }
> -
> - err = tcf_idr_check_alloc(tn, &index, a, bind);
> - if (err < 0)
> - return err;
> - exists = err;
> - if (exists && bind)
> - return 0;
> -
> - if (!exists) {
> - ret = tcf_idr_create(tn, index, est, a,
> - &act_mpls_ops, bind, true, flags);
> - if (ret) {
> - tcf_idr_cleanup(tn, index);
> - return ret;
> - }
> -
> - ret = ACT_P_CREATED;
> - } else if (!(flags & TCA_ACT_FLAGS_REPLACE)) {
> - tcf_idr_release(*a, bind);
> - return -EEXIST;
> + err = -EINVAL;
> + goto release_idr;
> }
>
> err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
> --
> 2.34.1
>
Powered by blists - more mailing lists