nexthop: More fine-grained policies for netlink message validation

PATCH Applied Petr Machata 3 participants 8 messages Jan 20, 2021 – Jan 21, 2021

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Petr Machata

author

2021-01-20 17:27 UTC #0 lore raw

v2 cover nexthop: More fine-grained policies for netlink message validation

There is currently one policy that covers all attributes for next hop object management. Actual validation is then done in code, which makes it unobvious which attributes are acceptable when, and indeed that everything is rejected as necessary.

In this series, split rtm_nh_policy to several policies that cover various aspects of the next hop object configuration, and instead of open-coding the validation, defer to nlmsg_parse(). This should make extending the next hop code simpler as well, which will be relevant in near future for resilient hashing implementation.

This was tested by running tools/testing/selftests/net/fib_nexthops.sh. Additionally iproute2 was tweaked to issue "nexthop list id" as an RTM_GETNEXTHOP dump request, instead of a straight get to test that unexpected attributes are indeed rejected.

In patch #1, convert attribute validation in nh_valid_get_del_req().

In patch #2, convert nh_valid_dump_req().

In patch #3, rtm_nh_policy is cleaned up and renamed to rtm_nh_policy_new, because after the above two patches, that is the only context that it is used in.

v2:
- Patches #1, #2 and #3:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX
- Patch #2:
- Convert manual setting of true to nla_get_flag().

Petr Machata (3):
nexthop: Use a dedicated policy for nh_valid_get_del_req()
nexthop: Use a dedicated policy for nh_valid_dump_req()
nexthop: Specialize rtm_nh_policy

 net/ipv4/nexthop.c | 105 +++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 62 deletions(-)

Signature · 1 line

2.26.2

Petr Machata

author

2021-01-20 17:27 UTC #1 lore raw

PATCH v2 1/3 nexthop: Use a dedicated policy for nh_valid_get_del_req()

This function uses the global nexthop policy only to then bounce all arguments except for NHA_ID. Instead, just create a new policy that only includes the one allowed attribute.

Signed-off-by Petr Machata <[email protected]>

---

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX

 net/ipv4/nexthop.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

net/ipv4/nexthop.c +9 ~~-17~~

index e53e43aef785..391079ff1bb5 100644
@@ -36,6 +36,10 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };
 
+static const struct nla_policy rtm_nh_policy_get[] = {
+	[NHA_ID]		= { .type = NLA_U32 },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;

@@ -1842,28 +1846,16 @@ static int nh_valid_get_del_req(struct nlmsghdr *nlh, u32 *id,
 				struct netlink_ext_ack *extack)
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
-	struct nlattr *tb[NHA_MAX + 1];
-	int err, i;
+	struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_get)];
+	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
-			  extack);
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb,
+			  ARRAY_SIZE(rtm_nh_policy_get) - 1,
+			  rtm_nh_policy_get, extack);
 	if (err < 0)
 		return err;
 
 	err = -EINVAL;
-	for (i = 0; i < __NHA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
-		switch (i) {
-		case NHA_ID:
-			break;
-		default:
-			NL_SET_ERR_MSG_ATTR(extack, tb[i],
-					    "Unexpected attribute in request");
-			goto out;
-		}
-	}
 	if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) {
 		NL_SET_ERR_MSG(extack, "Invalid values in header");
 		goto out;

Signature · 1 line

2.26.2

David Ahern

Reviewed-by on v2 1/3

2021-01-21 04:27 UTC #2 lore raw

Re: [PATCH net-next v2 1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()

Quoted 7 lines · Petr Machata

This function uses the global nexthop policy only to then bounce all arguments except for NHA_ID. Instead, just create a new policy that only includes the one allowed attribute.
Signed-off-by Petr Machata <[email protected]>
---

Quoted 7 lines

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX
 net/ipv4/nexthop.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

Reviewed-by David Ahern <[email protected]>

Petr Machata

author

2021-01-20 17:27 UTC #3 lore raw

PATCH v2 2/3 nexthop: Use a dedicated policy for nh_valid_dump_req()

In reply to Petr Machata

This function uses the global nexthop policy, but only accepts four particular attributes. Create a new policy that only includes the four supported attributes, and use it. Convert the loop to a series of ifs.

Signed-off-by Petr Machata <[email protected]>

---

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX
- Convert manual setting of true to nla_get_flag().

 net/ipv4/nexthop.c | 60 +++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

net/ipv4/nexthop.c +27 ~~-33~~

index 391079ff1bb5..bbea78ea4870 100644
@@ -40,6 +40,13 @@ static const struct nla_policy rtm_nh_policy_get[] = {
 	[NHA_ID]		= { .type = NLA_U32 },
 };
 
+static const struct nla_policy rtm_nh_policy_dump[] = {
+	[NHA_OIF]		= { .type = NLA_U32 },
+	[NHA_GROUPS]		= { .type = NLA_FLAG },
+	[NHA_MASTER]		= { .type = NLA_U32 },
+	[NHA_FDB]		= { .type = NLA_FLAG },
+};
+
 static bool nexthop_notifiers_is_empty(struct net *net)
 {
 	return !net->nexthop.notifier_chain.head;

@@ -1983,48 +1990,35 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh, int *dev_idx,
 			     bool *fdb_filter, struct netlink_callback *cb)
 {
 	struct netlink_ext_ack *extack = cb->extack;
-	struct nlattr *tb[NHA_MAX + 1];
+	struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_dump)];
 	struct nhmsg *nhm;
-	int err, i;
+	int err;
 	u32 idx;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
-			  NULL);
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb,
+			  ARRAY_SIZE(rtm_nh_policy_dump) - 1,
+			  rtm_nh_policy_dump, NULL);
 	if (err < 0)
 		return err;
 
-	for (i = 0; i <= NHA_MAX; ++i) {
-		if (!tb[i])
-			continue;
-
-		switch (i) {
-		case NHA_OIF:
-			idx = nla_get_u32(tb[i]);
-			if (idx > INT_MAX) {
-				NL_SET_ERR_MSG(extack, "Invalid device index");
-				return -EINVAL;
-			}
-			*dev_idx = idx;
-			break;
-		case NHA_MASTER:
-			idx = nla_get_u32(tb[i]);
-			if (idx > INT_MAX) {
-				NL_SET_ERR_MSG(extack, "Invalid master device index");
-				return -EINVAL;
-			}
-			*master_idx = idx;
-			break;
-		case NHA_GROUPS:
-			*group_filter = true;
-			break;
-		case NHA_FDB:
-			*fdb_filter = true;
-			break;
-		default:
-			NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
+	if (tb[NHA_OIF]) {
+		idx = nla_get_u32(tb[NHA_OIF]);
+		if (idx > INT_MAX) {
+			NL_SET_ERR_MSG(extack, "Invalid device index");
+			return -EINVAL;
+		}
+		*dev_idx = idx;
+	}
+	if (tb[NHA_MASTER]) {
+		idx = nla_get_u32(tb[NHA_MASTER]);
+		if (idx > INT_MAX) {
+			NL_SET_ERR_MSG(extack, "Invalid master device index");
 			return -EINVAL;
 		}
+		*master_idx = idx;
 	}
+	*group_filter = nla_get_flag(tb[NHA_GROUPS]);
+	*fdb_filter = nla_get_flag(tb[NHA_FDB]);
 
 	nhm = nlmsg_data(nlh);
 	if (nhm->nh_protocol || nhm->resvd || nhm->nh_scope || nhm->nh_flags) {

Signature · 1 line

2.26.2

David Ahern

Reviewed-by on v2 2/3

2021-01-21 04:30 UTC #4 lore raw

Re: [PATCH net-next v2 2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()

Quoted 7 lines · Petr Machata

This function uses the global nexthop policy, but only accepts four particular attributes. Create a new policy that only includes the four supported attributes, and use it. Convert the loop to a series of ifs.
Signed-off-by Petr Machata <[email protected]>
---

Quoted 9 lines

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX
- Convert manual setting of true to nla_get_flag().
 net/ipv4/nexthop.c | 60 +++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

Reviewed-by David Ahern <[email protected]>

Petr Machata

author

2021-01-20 17:27 UTC #5 lore raw

PATCH v2 3/3 nexthop: Specialize rtm_nh_policy

In reply to Petr Machata

This policy is currently only used for creation of new next hops and new next hop groups. Rename it accordingly and remove the two attributes that are not valid in that context: NHA_GROUPS and NHA_MASTER.

For consistency with other policies, do not mention policy array size in the declarator, and replace NHA_MAX for ARRAY_SIZE as appropriate.

Note that with this commit, NHA_MAX and __NHA_MAX are not used anymore. Leave them in purely as a user API.

Signed-off-by Petr Machata <[email protected]>

---

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX

 net/ipv4/nexthop.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

net/ipv4/nexthop.c +9 ~~-14~~

index bbea78ea4870..e6dfca426242 100644
@@ -22,7 +22,7 @@ static void remove_nexthop(struct net *net, struct nexthop *nh,
 #define NH_DEV_HASHBITS  8
 #define NH_DEV_HASHSIZE (1U << NH_DEV_HASHBITS)
 
-static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
+static const struct nla_policy rtm_nh_policy_new[] = {
 	[NHA_ID]		= { .type = NLA_U32 },
 	[NHA_GROUP]		= { .type = NLA_BINARY },
 	[NHA_GROUP_TYPE]	= { .type = NLA_U16 },

@@ -31,8 +31,6 @@ static const struct nla_policy rtm_nh_policy[NHA_MAX + 1] = {
 	[NHA_GATEWAY]		= { .type = NLA_BINARY },
 	[NHA_ENCAP_TYPE]	= { .type = NLA_U16 },
 	[NHA_ENCAP]		= { .type = NLA_NESTED },
-	[NHA_GROUPS]		= { .type = NLA_FLAG },
-	[NHA_MASTER]		= { .type = NLA_U32 },
 	[NHA_FDB]		= { .type = NLA_FLAG },
 };

@@ -576,7 +574,8 @@ static int nh_check_attr_fdb_group(struct nexthop *nh, u8 *nh_family,
 	return 0;
 }
 
-static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
+static int nh_check_attr_group(struct net *net,
+			       struct nlattr *tb[], size_t tb_size,
 			       struct netlink_ext_ack *extack)
 {
 	unsigned int len = nla_len(tb[NHA_GROUP]);

@@ -635,7 +634,7 @@ static int nh_check_attr_group(struct net *net, struct nlattr *tb[],
 			return -EINVAL;
 		}
 	}
-	for (i = NHA_GROUP_TYPE + 1; i < __NHA_MAX; ++i) {
+	for (i = NHA_GROUP_TYPE + 1; i < tb_size; ++i) {
 		if (!tb[i])
 			continue;
 		if (i == NHA_FDB)

@@ -1654,11 +1653,12 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 			    struct netlink_ext_ack *extack)
 {
 	struct nhmsg *nhm = nlmsg_data(nlh);
-	struct nlattr *tb[NHA_MAX + 1];
+	struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_new)];
 	int err;
 
-	err = nlmsg_parse(nlh, sizeof(*nhm), tb, NHA_MAX, rtm_nh_policy,
-			  extack);
+	err = nlmsg_parse(nlh, sizeof(*nhm), tb,
+			  ARRAY_SIZE(rtm_nh_policy_new) - 1,
+			  rtm_nh_policy_new, extack);
 	if (err < 0)
 		return err;

@@ -1685,11 +1685,6 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 		goto out;
 	}
 
-	if (tb[NHA_GROUPS] || tb[NHA_MASTER]) {
-		NL_SET_ERR_MSG(extack, "Invalid attributes in request");
-		goto out;
-	}
-
 	memset(cfg, 0, sizeof(*cfg));
 	cfg->nlflags = nlh->nlmsg_flags;
 	cfg->nlinfo.portid = NETLINK_CB(skb).portid;

@@ -1731,7 +1726,7 @@ static int rtm_to_nh_config(struct net *net, struct sk_buff *skb,
 			NL_SET_ERR_MSG(extack, "Invalid group type");
 			goto out;
 		}
-		err = nh_check_attr_group(net, tb, extack);
+		err = nh_check_attr_group(net, tb, ARRAY_SIZE(tb), extack);
 
 		/* no other attributes should be set */
 		goto out;

Signature · 1 line

2.26.2

David Ahern

Reviewed-by on v2 3/3

2021-01-21 04:31 UTC #6 lore raw

Re: [PATCH net-next v2 3/3] nexthop: Specialize rtm_nh_policy

Quoted 13 lines · Petr Machata

This policy is currently only used for creation of new next hops and new next hop groups. Rename it accordingly and remove the two attributes that are not valid in that context: NHA_GROUPS and NHA_MASTER.
For consistency with other policies, do not mention policy array size in the declarator, and replace NHA_MAX for ARRAY_SIZE as appropriate.
Note that with this commit, NHA_MAX and __NHA_MAX are not used anymore. Leave them in purely as a user API.
Signed-off-by Petr Machata <[email protected]>
---

Quoted 8 lines

Notes:
v2:
- Do not specify size of the policy array. Use ARRAY_SIZE instead of NHA_MAX
 net/ipv4/nexthop.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Reviewed-by David Ahern <[email protected]>

[email protected]

CI Applied

2021-01-21 05:18 UTC #7 lore raw

In reply to Petr Machata

This series was applied to netdev/net-next.git (refs/heads/master):

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

Quoted 5 lines · On Wed, 20 Jan 2021 16:44:09 +0100 you wrote:

There is currently one policy that covers all attributes for next hop object management. Actual validation is then done in code, which makes it unobvious which attributes are acceptable when, and indeed that everything is rejected as necessary.

Quoted 7 lines

In this series, split rtm_nh_policy to several policies that cover various aspects of the next hop object configuration, and instead of open-coding the validation, defer to nlmsg_parse(). This should make extending the next hop code simpler as well, which will be relevant in near future for resilient hashing implementation.
[...]

Here is the summary with links:
- [net-next,v2,1/3] nexthop: Use a dedicated policy for nh_valid_get_del_req()

    https://git.kernel.org/netdev/net-next/c/60f5ad5e19c0
  - [net-next,v2,2/3] nexthop: Use a dedicated policy for nh_valid_dump_req()
    https://git.kernel.org/netdev/net-next/c/44551bff290d
  - [net-next,v2,3/3] nexthop: Specialize rtm_nh_policy
    https://git.kernel.org/netdev/net-next/c/643d0878e674

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html