From 5036034572b79daa6d6600338e8e8229e2a44b09 Mon Sep 17 00:00:00 2001 From: Pedro Tammela Date: Fri, 21 Apr 2023 18:25:13 -0300 Subject: [PATCH 1/5] net/sched: act_pedit: use NLA_POLICY for parsing 'ex' keys Transform two checks in the 'ex' key parsing into netlink policies removing extra if checks. Signed-off-by: Pedro Tammela Reviewed-by: Simon Horman Signed-off-by: David S. Miller --- net/sched/act_pedit.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 4559a1507ea5..45201f75e896 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -30,8 +30,9 @@ static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = { }; static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = { - [TCA_PEDIT_KEY_EX_HTYPE] = { .type = NLA_U16 }, - [TCA_PEDIT_KEY_EX_CMD] = { .type = NLA_U16 }, + [TCA_PEDIT_KEY_EX_HTYPE] = + NLA_POLICY_MAX(NLA_U16, TCA_PEDIT_HDR_TYPE_MAX), + [TCA_PEDIT_KEY_EX_CMD] = NLA_POLICY_MAX(NLA_U16, TCA_PEDIT_CMD_MAX), }; static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, @@ -81,12 +82,6 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, k->htype = nla_get_u16(tb[TCA_PEDIT_KEY_EX_HTYPE]); k->cmd = nla_get_u16(tb[TCA_PEDIT_KEY_EX_CMD]); - if (k->htype > TCA_PEDIT_HDR_TYPE_MAX || - k->cmd > TCA_PEDIT_CMD_MAX) { - err = -EINVAL; - goto err_out; - } - k++; } From 0c83c5210e18763ec54252b990d270748c99b01d Mon Sep 17 00:00:00 2001 From: Pedro Tammela Date: Fri, 21 Apr 2023 18:25:14 -0300 Subject: [PATCH 2/5] net/sched: act_pedit: use extack in 'ex' parsing errors We have extack available when parsing 'ex' keys, so pass it to tcf_pedit_keys_ex_parse and add more detailed error messages. Reviewed-by: Simon Horman Signed-off-by: Pedro Tammela Signed-off-by: David S. Miller --- net/sched/act_pedit.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 45201f75e896..24976cd4e4a2 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -36,7 +36,7 @@ static const struct nla_policy pedit_key_ex_policy[TCA_PEDIT_KEY_EX_MAX + 1] = { }; static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, - u8 n) + u8 n, struct netlink_ext_ack *extack) { struct tcf_pedit_key_ex *keys_ex; struct tcf_pedit_key_ex *k; @@ -57,12 +57,14 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, struct nlattr *tb[TCA_PEDIT_KEY_EX_MAX + 1]; if (!n) { + NL_SET_ERR_MSG_MOD(extack, "Can't parse more extended keys than requested"); err = -EINVAL; goto err_out; } n--; if (nla_type(ka) != TCA_PEDIT_KEY_EX) { + NL_SET_ERR_MSG_ATTR(extack, ka, "Unknown attribute, expected extended key"); err = -EINVAL; goto err_out; } @@ -73,8 +75,14 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, if (err) goto err_out; - if (!tb[TCA_PEDIT_KEY_EX_HTYPE] || - !tb[TCA_PEDIT_KEY_EX_CMD]) { + if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_HTYPE)) { + NL_SET_ERR_MSG(extack, "Missing required attribute"); + err = -EINVAL; + goto err_out; + } + + if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PEDIT_KEY_EX_CMD)) { + NL_SET_ERR_MSG(extack, "Missing required attribute"); err = -EINVAL; goto err_out; } @@ -86,6 +94,7 @@ static struct tcf_pedit_key_ex *tcf_pedit_keys_ex_parse(struct nlattr *nla, } if (n) { + NL_SET_ERR_MSG_MOD(extack, "Not enough extended keys to parse"); err = -EINVAL; goto err_out; } @@ -217,7 +226,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, } nparms->tcfp_keys_ex = - tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys); + tcf_pedit_keys_ex_parse(tb[TCA_PEDIT_KEYS_EX], parm->nkeys, extack); if (IS_ERR(nparms->tcfp_keys_ex)) { ret = PTR_ERR(nparms->tcfp_keys_ex); goto out_free; From e1201bc781c28766720e78a5e099ffa568be4d74 Mon Sep 17 00:00:00 2001 From: Pedro Tammela Date: Fri, 21 Apr 2023 18:25:15 -0300 Subject: [PATCH 3/5] net/sched: act_pedit: check static offsets a priori Static key offsets should always be on 32 bit boundaries. Validate them on create/update time for static offsets and move the datapath validation for runtime offsets only. iproute2 already errors out if a given offset and data size cannot be packed to a 32 bit boundary. This change will make sure users which create/update pedit instances directly via netlink also error out, instead of finding out when packets are traversing. Reviewed-by: Jamal Hadi Salim Reviewed-by: Simon Horman Signed-off-by: Pedro Tammela Signed-off-by: David S. Miller --- net/sched/act_pedit.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 24976cd4e4a2..cc4dfb01c6c7 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -251,8 +251,16 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, memcpy(nparms->tcfp_keys, parm->keys, ksize); for (i = 0; i < nparms->tcfp_nkeys; ++i) { + u32 offmask = nparms->tcfp_keys[i].offmask; u32 cur = nparms->tcfp_keys[i].off; + /* The AT option can be added to static offsets in the datapath */ + if (!offmask && cur % 4) { + NL_SET_ERR_MSG_MOD(extack, "Offsets must be on 32bit boundaries"); + ret = -EINVAL; + goto put_chain; + } + /* sanitize the shift value for any later use */ nparms->tcfp_keys[i].shift = min_t(size_t, BITS_PER_TYPE(int) - 1, @@ -261,7 +269,7 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla, /* The AT option can read a single byte, we can bound the actual * value with uchar max. */ - cur += (0xff & nparms->tcfp_keys[i].offmask) >> nparms->tcfp_keys[i].shift; + cur += (0xff & offmask) >> nparms->tcfp_keys[i].shift; /* Each key touches 4 bytes starting from the computed offset */ nparms->tcfp_off_max_hint = @@ -411,12 +419,12 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, sizeof(_d), &_d); if (!d) goto bad; - offset += (*d & tkey->offmask) >> tkey->shift; - } - if (offset % 4) { - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); - goto bad; + offset += (*d & tkey->offmask) >> tkey->shift; + if (offset % 4) { + pr_info("tc action pedit offset must be on 32 bit boundaries\n"); + goto bad; + } } if (!offset_valid(skb, hoffset + offset)) { From 577140180ba28d0d37bc898c7bd6702c83aa106f Mon Sep 17 00:00:00 2001 From: Pedro Tammela Date: Fri, 21 Apr 2023 18:25:16 -0300 Subject: [PATCH 4/5] net/sched: act_pedit: remove extra check for key type The netlink parsing already validates the key 'htype'. Remove the datapath check as it's redundant. Reviewed-by: Jamal Hadi Salim Reviewed-by: Simon Horman Signed-off-by: Pedro Tammela Signed-off-by: David S. Miller --- net/sched/act_pedit.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index cc4dfb01c6c7..2fec4473d800 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -325,37 +325,28 @@ static bool offset_valid(struct sk_buff *skb, int offset) return true; } -static int pedit_skb_hdr_offset(struct sk_buff *skb, - enum pedit_header_type htype, int *hoffset) +static void pedit_skb_hdr_offset(struct sk_buff *skb, + enum pedit_header_type htype, int *hoffset) { - int ret = -EINVAL; - + /* 'htype' is validated in the netlink parsing */ switch (htype) { case TCA_PEDIT_KEY_EX_HDR_TYPE_ETH: - if (skb_mac_header_was_set(skb)) { + if (skb_mac_header_was_set(skb)) *hoffset = skb_mac_offset(skb); - ret = 0; - } break; case TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK: case TCA_PEDIT_KEY_EX_HDR_TYPE_IP4: case TCA_PEDIT_KEY_EX_HDR_TYPE_IP6: *hoffset = skb_network_offset(skb); - ret = 0; break; case TCA_PEDIT_KEY_EX_HDR_TYPE_TCP: case TCA_PEDIT_KEY_EX_HDR_TYPE_UDP: - if (skb_transport_header_was_set(skb)) { + if (skb_transport_header_was_set(skb)) *hoffset = skb_transport_offset(skb); - ret = 0; - } break; default: - ret = -EINVAL; break; } - - return ret; } TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, @@ -388,10 +379,9 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, for (i = parms->tcfp_nkeys; i > 0; i--, tkey++) { int offset = tkey->off; + int hoffset = 0; u32 *ptr, hdata; - int hoffset; u32 val; - int rc; if (tkey_ex) { htype = tkey_ex->htype; @@ -400,12 +390,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, tkey_ex++; } - rc = pedit_skb_hdr_offset(skb, htype, &hoffset); - if (rc) { - pr_info("tc action pedit bad header type specified (0x%x)\n", - htype); - goto bad; - } + pedit_skb_hdr_offset(skb, htype, &hoffset); if (tkey->offmask) { u8 *d, _d; From e3c9673e2f6e1b3aa4bb87c570336e10f364c28a Mon Sep 17 00:00:00 2001 From: Pedro Tammela Date: Fri, 21 Apr 2023 18:25:17 -0300 Subject: [PATCH 5/5] net/sched: act_pedit: rate limit datapath messages Unbounded info messages in the pedit datapath can flood the printk ring buffer quite easily depending on the action created. As these messages are informational, usually printing some, not all, is enough to bring attention to the real issue. Reviewed-by: Jamal Hadi Salim Reviewed-by: Simon Horman Signed-off-by: Pedro Tammela Signed-off-by: David S. Miller --- net/sched/act_pedit.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c index 2fec4473d800..fb93d4c1faca 100644 --- a/net/sched/act_pedit.c +++ b/net/sched/act_pedit.c @@ -396,8 +396,8 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, u8 *d, _d; if (!offset_valid(skb, hoffset + tkey->at)) { - pr_info("tc action pedit 'at' offset %d out of bounds\n", - hoffset + tkey->at); + pr_info_ratelimited("tc action pedit 'at' offset %d out of bounds\n", + hoffset + tkey->at); goto bad; } d = skb_header_pointer(skb, hoffset + tkey->at, @@ -407,14 +407,13 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, offset += (*d & tkey->offmask) >> tkey->shift; if (offset % 4) { - pr_info("tc action pedit offset must be on 32 bit boundaries\n"); + pr_info_ratelimited("tc action pedit offset must be on 32 bit boundaries\n"); goto bad; } } if (!offset_valid(skb, hoffset + offset)) { - pr_info("tc action pedit offset %d out of bounds\n", - hoffset + offset); + pr_info_ratelimited("tc action pedit offset %d out of bounds\n", hoffset + offset); goto bad; } @@ -431,8 +430,7 @@ TC_INDIRECT_SCOPE int tcf_pedit_act(struct sk_buff *skb, val = (*ptr + tkey->val) & ~tkey->mask; break; default: - pr_info("tc action pedit bad command (%d)\n", - cmd); + pr_info_ratelimited("tc action pedit bad command (%d)\n", cmd); goto bad; }