[nf PATCH] netfilter: nf_tables: Fix netdev hook sanity checks

Wildcard interface names were confused with non-wildcard ones or those
matching the wildcard's prefix. Fix for odd behaviour when updating
flowtable or netdev chain hook devices.

Unify behaviour between flowtable and netdev chain parsers by having
both call the same parsing/validating code, also with old NFTA_HOOK_DEV
attribute (which is still used for single hook updates by user space).

The validating code must distinguish between hooks being added or
deleted (duplicates are ignored in the former case). Transport this info
in a 'bool add' parameter, but rename the existing one with same name
first since that actually reflects whether the code is parsing hooks for
a new object or an existing one.

Fixes: 6d07a289504a ("netfilter: nf_tables: Support wildcard netdev hook specs")
Signed-off-by: Phil Sutter <[email protected]>
---
This is a follow-up to Fernando's "netfilter: nf_tables: fix device name
and prefix match in hook lookup", sorting odd behaviour in similar,
other cases, too. With his v4 and this patch in place, the new nftables
shell tests pass for me.
---
 net/netfilter/nf_tables_api.c | 102 +++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 31 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 500ef483e485..8fac88f07d74 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2450,10 +2450,44 @@ static struct nft_hook *nft_hook_list_find(struct list_head *hook_list,
 	return NULL;
 }
 
+static struct nft_hook *
+nf_tables_parse_netdev_hook(struct net *net,
+			    const struct nlattr *tmp,
+			    struct list_head *hook_list,
+			    struct list_head *old_hook_list,
+			    bool prefix, bool add)
+{
+	struct nft_hook *hook;
+
+	hook = nft_netdev_hook_alloc(net, tmp, prefix);
+	if (IS_ERR(hook))
+		return hook;
+
+	/* Reject new hooks which overlap */
+	if (nft_hook_list_find(hook_list, hook, false)) {
+		nft_netdev_hook_free(hook);
+		return ERR_PTR(-EEXIST);
+	}
+	/* Skip this exact duplicate when adding */
+	if (nft_hook_list_find(old_hook_list, hook, true)) {
+		if (add) {
+			nft_netdev_hook_free(hook);
+			return NULL;
+		}
+	/* Reject partial overlaps in either case */
+	} else if (nft_hook_list_find(old_hook_list, hook, false)) {
+		nft_netdev_hook_free(hook);
+		return ERR_PTR(add ? -EEXIST : -ENOENT);
+	}
+	return hook;
+}
+
 static int nf_tables_parse_netdev_hooks(struct net *net,
 					const struct nlattr *attr,
 					struct list_head *hook_list,
-					struct netlink_ext_ack *extack)
+					struct list_head *old_hook_list,
+					struct netlink_ext_ack *extack,
+					bool add)
 {
 	struct nft_hook *hook, *next;
 	const struct nlattr *tmp;
@@ -2473,18 +2507,17 @@ static int nf_tables_parse_netdev_hooks(struct net *net,
 			goto err_hook;
 		}
 
-		hook = nft_netdev_hook_alloc(net, tmp, prefix);
+		hook = nf_tables_parse_netdev_hook(net, tmp, hook_list,
+						   old_hook_list, prefix, add);
+		if (!hook)
+			continue;
+
 		if (IS_ERR(hook)) {
 			NL_SET_BAD_ATTR(extack, tmp);
 			err = PTR_ERR(hook);
 			goto err_hook;
 		}
-		if (nft_hook_list_find(hook_list, hook, false)) {
-			NL_SET_BAD_ATTR(extack, tmp);
-			nft_netdev_hook_free(hook);
-			err = -EEXIST;
-			goto err_hook;
-		}
+
 		list_add_tail(&hook->list, hook_list);
 		n++;
 
@@ -2513,13 +2546,17 @@ struct nft_chain_hook {
 
 static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
 				  struct list_head *hook_list,
-				  struct netlink_ext_ack *extack, u32 flags)
+				  struct list_head *old_hook_list,
+				  struct netlink_ext_ack *extack, u32 flags,
+				  bool add)
 {
 	struct nft_hook *hook;
 	int err;
 
 	if (tb[NFTA_HOOK_DEV]) {
-		hook = nft_netdev_hook_alloc(net, tb[NFTA_HOOK_DEV], false);
+		hook = nf_tables_parse_netdev_hook(net, tb[NFTA_HOOK_DEV],
+						   hook_list, old_hook_list,
+						   false, false);
 		if (IS_ERR(hook)) {
 			NL_SET_BAD_ATTR(extack, tb[NFTA_HOOK_DEV]);
 			return PTR_ERR(hook);
@@ -2528,7 +2565,8 @@ static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
 		list_add_tail(&hook->list, hook_list);
 	} else if (tb[NFTA_HOOK_DEVS]) {
 		err = nf_tables_parse_netdev_hooks(net, tb[NFTA_HOOK_DEVS],
-						   hook_list, extack);
+						   hook_list, old_hook_list,
+						   extack, add);
 		if (err < 0)
 			return err;
 
@@ -2544,8 +2582,10 @@ static int nft_chain_parse_netdev(struct net *net, struct nlattr *tb[],
 static int nft_chain_parse_hook(struct net *net,
 				struct nft_base_chain *basechain,
 				const struct nlattr * const nla[],
-				struct nft_chain_hook *hook, u8 family,
-				u32 flags, struct netlink_ext_ack *extack)
+				struct nft_chain_hook *hook,
+				struct list_head *old_hook_list, u8 family,
+				u32 flags, struct netlink_ext_ack *extack,
+				bool add)
 {
 	struct nftables_pernet *nft_net = nft_pernet(net);
 	struct nlattr *ha[NFTA_HOOK_MAX + 1];
@@ -2623,7 +2663,8 @@ static int nft_chain_parse_hook(struct net *net,
 
 	INIT_LIST_HEAD(&hook->list);
 	if (nft_base_chain_netdev(family, hook->num)) {
-		err = nft_chain_parse_netdev(net, ha, &hook->list, extack, flags);
+		err = nft_chain_parse_netdev(net, ha, &hook->list,
+					     old_hook_list, extack, flags, add);
 		if (err < 0) {
 			module_put(type->owner);
 			return err;
@@ -2756,6 +2797,7 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 policy,
 	if (nla[NFTA_CHAIN_HOOK]) {
 		struct nft_stats __percpu *stats = NULL;
 		struct nft_chain_hook hook = {};
+		LIST_HEAD(empty);
 
 		if (table->flags & __NFT_TABLE_F_UPDATE)
 			return -EINVAL;
@@ -2763,8 +2805,8 @@ static int nf_tables_addchain(struct nft_ctx *ctx, u8 family, u8 policy,
 		if (flags & NFT_CHAIN_BINDING)
 			return -EOPNOTSUPP;
 
-		err = nft_chain_parse_hook(net, NULL, nla, &hook, family, flags,
-					   extack);
+		err = nft_chain_parse_hook(net, NULL, nla, &hook, &empty,
+					   family, flags, extack, true);
 		if (err < 0)
 			return err;
 
@@ -2916,7 +2958,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 
 		basechain = nft_base_chain(chain);
 		err = nft_chain_parse_hook(ctx->net, basechain, nla, &hook,
-					   ctx->family, flags, extack);
+					   &basechain->hook_list,
+					   ctx->family, flags, extack, true);
 		if (err < 0)
 			return err;
 
@@ -2936,7 +2979,7 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
 					ops->hook	= basechain->ops.hook;
 				}
 
-				if (nft_hook_list_find(&basechain->hook_list, h, false)) {
+				if (nft_hook_list_find(&basechain->hook_list, h, true)) {
 					list_del(&h->list);
 					nft_netdev_hook_free(h);
 					continue;
@@ -3246,7 +3289,8 @@ static int nft_delchain_hook(struct nft_ctx *ctx,
 		return -EOPNOTSUPP;
 
 	err = nft_chain_parse_hook(ctx->net, basechain, nla, &chain_hook,
-				   ctx->family, chain->flags, extack);
+				   &basechain->hook_list, ctx->family,
+				   chain->flags, extack, false);
 	if (err < 0)
 		return err;
 
@@ -8866,7 +8910,8 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
 				    const struct nlattr * const nla[],
 				    struct nft_flowtable_hook *flowtable_hook,
 				    struct nft_flowtable *flowtable,
-				    struct netlink_ext_ack *extack, bool add)
+				    struct netlink_ext_ack *extack,
+				    bool new, bool add)
 {
 	struct nlattr *tb[NFTA_FLOWTABLE_HOOK_MAX + 1];
 	struct nf_hook_ops *ops;
@@ -8882,7 +8927,7 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
-	if (add) {
+	if (new) {
 		if (!tb[NFTA_FLOWTABLE_HOOK_NUM] ||
 		    !tb[NFTA_FLOWTABLE_HOOK_PRIORITY]) {
 			NL_SET_BAD_ATTR(extack, nla[NFTA_FLOWTABLE_NAME]);
@@ -8918,7 +8963,8 @@ static int nft_flowtable_parse_hook(const struct nft_ctx *ctx,
 		err = nf_tables_parse_netdev_hooks(ctx->net,
 						   tb[NFTA_FLOWTABLE_HOOK_DEVS],
 						   &flowtable_hook->list,
-						   extack);
+						   &flowtable->hook_list,
+						   extack, add);
 		if (err < 0)
 			return err;
 	}
@@ -9112,17 +9158,11 @@ static int nft_flowtable_update(struct nft_ctx *ctx, const struct nlmsghdr *nlh,
 	int err;
 
 	err = nft_flowtable_parse_hook(ctx, nla, &flowtable_hook, flowtable,
-				       extack, false);
+				       extack, false, true);
 	if (err < 0)
 		return err;
 
 	list_for_each_entry_safe(hook, next, &flowtable_hook.list, list) {
-		if (nft_hook_list_find(&flowtable->hook_list, hook, false)) {
-			list_del(&hook->list);
-			nft_netdev_hook_free(hook);
-			continue;
-		}
-
 		nft_net = nft_pernet(ctx->net);
 		list_for_each_entry(trans, &nft_net->commit_list, list) {
 			if (trans->msg_type != NFT_MSG_NEWFLOWTABLE ||
@@ -9279,7 +9319,7 @@ static int nf_tables_newflowtable(struct sk_buff *skb,
 		goto err3;
 
 	err = nft_flowtable_parse_hook(&ctx, nla, &flowtable_hook, flowtable,
-				       extack, true);
+				       extack, true, true);
 	if (err < 0)
 		goto err_flowtable_parse_hooks;
 
@@ -9345,7 +9385,7 @@ static int nft_delflowtable_hook(struct nft_ctx *ctx,
 	int err;
 
 	err = nft_flowtable_parse_hook(ctx, nla, &flowtable_hook, flowtable,
-				       extack, false);
+				       extack, false, false);
 	if (err < 0)
 		return err;
 
-- 
2.54.0