[PATCH nf-next 0/2] netfilter: nf_tables: use NLA_POLICY_MASK instead of manual checks

[PATCH nf-next 0/2] netfilter: nf_tables: use NLA_POLICY_MASK instead of manual checks

[Florian Westphal]

nf_tables still uses manual attribute validation in multiple places.
Make NLA_POLICY_MASK available with NLA_BE16/NLA_BE32 and then start
using it for flag attribute validation.

Florian Westphal (2):
  netlink: allow be16 and be32 types in all uint policy checks
  netfilter: nf_tables: use NLA_POLICY_MASK to test for valid flag
    options

 include/net/netlink.h      | 10 +++-------
 lib/nlattr.c               |  6 ++++++
 net/netfilter/nft_fib.c    | 13 +++++++------
 net/netfilter/nft_lookup.c |  6 ++----
 net/netfilter/nft_masq.c   |  8 +++-----
 net/netfilter/nft_nat.c    |  8 +++-----
 net/netfilter/nft_redir.c  |  8 +++-----
 7 files changed, 27 insertions(+), 32 deletions(-)

-- 
2.41.0

[PATCH nf-next 1/2] netlink: allow be16 and be32 types in all uint policy checks

[Florian Westphal]

__NLA_IS_BEINT_TYPE(tp) isn't useful.  NLA_BE16/32 are identical to
NLA_U16/32, the only difference is that it tells the netlink validation
functions that byteorder conversion might be needed before comparing
the value to the policy min/max ones.

After this change all policy macros that can be used with UINT types,
such as NLA_POLICY_MASK() can also be used with NLA_BE16/32.

This will be used to validate nf_tables flag attributes which
are in bigendian byte order.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netlink.h | 10 +++-------
 lib/nlattr.c          |  6 ++++++
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b12cd957abb4..8a7cd1170e1f 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -375,12 +375,11 @@ struct nla_policy {
 #define NLA_POLICY_BITFIELD32(valid) \
 	{ .type = NLA_BITFIELD32, .bitfield32_valid = valid }
 
-#define __NLA_IS_UINT_TYPE(tp)						\
-	(tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 || tp == NLA_U64)
+#define __NLA_IS_UINT_TYPE(tp)					\
+	(tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 ||	\
+	 tp == NLA_U64 || tp == NLA_BE16 || tp == NLA_BE32)
 #define __NLA_IS_SINT_TYPE(tp)						\
 	(tp == NLA_S8 || tp == NLA_S16 || tp == NLA_S32 || tp == NLA_S64)
-#define __NLA_IS_BEINT_TYPE(tp)						\
-	(tp == NLA_BE16 || tp == NLA_BE32)
 
 #define __NLA_ENSURE(condition) BUILD_BUG_ON_ZERO(!(condition))
 #define NLA_ENSURE_UINT_TYPE(tp)			\
@@ -394,7 +393,6 @@ struct nla_policy {
 #define NLA_ENSURE_INT_OR_BINARY_TYPE(tp)		\
 	(__NLA_ENSURE(__NLA_IS_UINT_TYPE(tp) ||		\
 		      __NLA_IS_SINT_TYPE(tp) ||		\
-		      __NLA_IS_BEINT_TYPE(tp) ||	\
 		      tp == NLA_MSECS ||		\
 		      tp == NLA_BINARY) + tp)
 #define NLA_ENSURE_NO_VALIDATION_PTR(tp)		\
@@ -402,8 +400,6 @@ struct nla_policy {
 		      tp != NLA_REJECT &&		\
 		      tp != NLA_NESTED &&		\
 		      tp != NLA_NESTED_ARRAY) + tp)
-#define NLA_ENSURE_BEINT_TYPE(tp)			\
-	(__NLA_ENSURE(__NLA_IS_BEINT_TYPE(tp)) + tp)
 
 #define NLA_POLICY_RANGE(tp, _min, _max) {		\
 	.type = NLA_ENSURE_INT_OR_BINARY_TYPE(tp),	\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 489e15bde5c1..7a2b6c38fd59 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -355,6 +355,12 @@ static int nla_validate_mask(const struct nla_policy *pt,
 	case NLA_U64:
 		value = nla_get_u64(nla);
 		break;
+	case NLA_BE16:
+		value = ntohs(nla_get_be16(nla));
+		break;
+	case NLA_BE32:
+		value = ntohl(nla_get_be32(nla));
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.41.0

[PATCH nf-next 2/2] netfilter: nf_tables: use NLA_POLICY_MASK to test for valid flag options

[Florian Westphal]

nf_tables relies on manual test of netlink attributes coming from userspace
even in cases where this could be handled via netlink policy.

Convert a bunch of 'flag' attributes to use NLA_POLICY_MASK checks.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_fib.c    | 13 +++++++------
 net/netfilter/nft_lookup.c |  6 ++----
 net/netfilter/nft_masq.c   |  8 +++-----
 net/netfilter/nft_nat.c    |  8 +++-----
 net/netfilter/nft_redir.c  |  8 +++-----
 5 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/nft_fib.c b/net/netfilter/nft_fib.c
index 6e049fd48760..601c9e09d07a 100644
--- a/net/netfilter/nft_fib.c
+++ b/net/netfilter/nft_fib.c
@@ -14,17 +14,18 @@
 #include <net/netfilter/nf_tables.h>
 #include <net/netfilter/nft_fib.h>
 
+#define NFTA_FIB_F_ALL (NFTA_FIB_F_SADDR | NFTA_FIB_F_DADDR | \
+			NFTA_FIB_F_MARK | NFTA_FIB_F_IIF | NFTA_FIB_F_OIF | \
+			NFTA_FIB_F_PRESENT)
+
 const struct nla_policy nft_fib_policy[NFTA_FIB_MAX + 1] = {
 	[NFTA_FIB_DREG]		= { .type = NLA_U32 },
 	[NFTA_FIB_RESULT]	= { .type = NLA_U32 },
-	[NFTA_FIB_FLAGS]	= { .type = NLA_U32 },
+	[NFTA_FIB_FLAGS]	=
+		NLA_POLICY_MASK(NLA_BE32, NFTA_FIB_F_ALL),
 };
 EXPORT_SYMBOL(nft_fib_policy);
 
-#define NFTA_FIB_F_ALL (NFTA_FIB_F_SADDR | NFTA_FIB_F_DADDR | \
-			NFTA_FIB_F_MARK | NFTA_FIB_F_IIF | NFTA_FIB_F_OIF | \
-			NFTA_FIB_F_PRESENT)
-
 int nft_fib_validate(const struct nft_ctx *ctx, const struct nft_expr *expr,
 		     const struct nft_data **data)
 {
@@ -77,7 +78,7 @@ int nft_fib_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 
 	priv->flags = ntohl(nla_get_be32(tb[NFTA_FIB_FLAGS]));
 
-	if (priv->flags == 0 || (priv->flags & ~NFTA_FIB_F_ALL))
+	if (priv->flags == 0)
 		return -EINVAL;
 
 	if ((priv->flags & (NFTA_FIB_F_SADDR | NFTA_FIB_F_DADDR)) ==
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 29ac48cdd6db..870e5b113d13 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -90,7 +90,8 @@ static const struct nla_policy nft_lookup_policy[NFTA_LOOKUP_MAX + 1] = {
 	[NFTA_LOOKUP_SET_ID]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_SREG]	= { .type = NLA_U32 },
 	[NFTA_LOOKUP_DREG]	= { .type = NLA_U32 },
-	[NFTA_LOOKUP_FLAGS]	= { .type = NLA_U32 },
+	[NFTA_LOOKUP_FLAGS]	=
+		NLA_POLICY_MASK(NLA_BE32, NFT_LOOKUP_F_INV),
 };
 
 static int nft_lookup_init(const struct nft_ctx *ctx,
@@ -120,9 +121,6 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
 	if (tb[NFTA_LOOKUP_FLAGS]) {
 		flags = ntohl(nla_get_be32(tb[NFTA_LOOKUP_FLAGS]));
 
-		if (flags & ~NFT_LOOKUP_F_INV)
-			return -EINVAL;
-
 		if (flags & NFT_LOOKUP_F_INV)
 			priv->invert = true;
 	}
diff --git a/net/netfilter/nft_masq.c b/net/netfilter/nft_masq.c
index b115d77fbbc7..8a14aaca93bb 100644
--- a/net/netfilter/nft_masq.c
+++ b/net/netfilter/nft_masq.c
@@ -20,7 +20,8 @@ struct nft_masq {
 };
 
 static const struct nla_policy nft_masq_policy[NFTA_MASQ_MAX + 1] = {
-	[NFTA_MASQ_FLAGS]		= { .type = NLA_U32 },
+	[NFTA_MASQ_FLAGS]		=
+		NLA_POLICY_MASK(NLA_BE32, NF_NAT_RANGE_MASK),
 	[NFTA_MASQ_REG_PROTO_MIN]	= { .type = NLA_U32 },
 	[NFTA_MASQ_REG_PROTO_MAX]	= { .type = NLA_U32 },
 };
@@ -47,11 +48,8 @@ static int nft_masq_init(const struct nft_ctx *ctx,
 	struct nft_masq *priv = nft_expr_priv(expr);
 	int err;
 
-	if (tb[NFTA_MASQ_FLAGS]) {
+	if (tb[NFTA_MASQ_FLAGS])
 		priv->flags = ntohl(nla_get_be32(tb[NFTA_MASQ_FLAGS]));
-		if (priv->flags & ~NF_NAT_RANGE_MASK)
-			return -EINVAL;
-	}
 
 	if (tb[NFTA_MASQ_REG_PROTO_MIN]) {
 		err = nft_parse_register_load(tb[NFTA_MASQ_REG_PROTO_MIN],
diff --git a/net/netfilter/nft_nat.c b/net/netfilter/nft_nat.c
index 5c29915ab028..583885ce7232 100644
--- a/net/netfilter/nft_nat.c
+++ b/net/netfilter/nft_nat.c
@@ -132,7 +132,8 @@ static const struct nla_policy nft_nat_policy[NFTA_NAT_MAX + 1] = {
 	[NFTA_NAT_REG_ADDR_MAX]	 = { .type = NLA_U32 },
 	[NFTA_NAT_REG_PROTO_MIN] = { .type = NLA_U32 },
 	[NFTA_NAT_REG_PROTO_MAX] = { .type = NLA_U32 },
-	[NFTA_NAT_FLAGS]	 = { .type = NLA_U32 },
+	[NFTA_NAT_FLAGS]	 =
+		NLA_POLICY_MASK(NLA_BE32, NF_NAT_RANGE_MASK),
 };
 
 static int nft_nat_validate(const struct nft_ctx *ctx,
@@ -246,11 +247,8 @@ static int nft_nat_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
 		priv->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 	}
 
-	if (tb[NFTA_NAT_FLAGS]) {
+	if (tb[NFTA_NAT_FLAGS])
 		priv->flags |= ntohl(nla_get_be32(tb[NFTA_NAT_FLAGS]));
-		if (priv->flags & ~NF_NAT_RANGE_MASK)
-			return -EOPNOTSUPP;
-	}
 
 	return nf_ct_netns_get(ctx->net, family);
 }
diff --git a/net/netfilter/nft_redir.c b/net/netfilter/nft_redir.c
index a70196ffcb1e..a58bd8d291ff 100644
--- a/net/netfilter/nft_redir.c
+++ b/net/netfilter/nft_redir.c
@@ -22,7 +22,8 @@ struct nft_redir {
 static const struct nla_policy nft_redir_policy[NFTA_REDIR_MAX + 1] = {
 	[NFTA_REDIR_REG_PROTO_MIN]	= { .type = NLA_U32 },
 	[NFTA_REDIR_REG_PROTO_MAX]	= { .type = NLA_U32 },
-	[NFTA_REDIR_FLAGS]		= { .type = NLA_U32 },
+	[NFTA_REDIR_FLAGS]		=
+		NLA_POLICY_MASK(NLA_BE32, NF_NAT_RANGE_MASK),
 };
 
 static int nft_redir_validate(const struct nft_ctx *ctx,
@@ -68,11 +69,8 @@ static int nft_redir_init(const struct nft_ctx *ctx,
 		priv->flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
 	}
 
-	if (tb[NFTA_REDIR_FLAGS]) {
+	if (tb[NFTA_REDIR_FLAGS])
 		priv->flags = ntohl(nla_get_be32(tb[NFTA_REDIR_FLAGS]));
-		if (priv->flags & ~NF_NAT_RANGE_MASK)
-			return -EINVAL;
-	}
 
 	return nf_ct_netns_get(ctx->net, ctx->family);
 }
-- 
2.41.0

Re: [PATCH nf-next 1/2] netlink: allow be16 and be32 types in all uint policy checks

[Jakub Kicinski]

On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote:
> __NLA_IS_BEINT_TYPE(tp) isn't useful.  NLA_BE16/32 are identical to
> NLA_U16/32, the only difference is that it tells the netlink validation
> functions that byteorder conversion might be needed before comparing
> the value to the policy min/max ones.
> 
> After this change all policy macros that can be used with UINT types,
> such as NLA_POLICY_MASK() can also be used with NLA_BE16/32.
> 
> This will be used to validate nf_tables flag attributes which
> are in bigendian byte order.

Semi-related, how well do we do with NLA_F_NET_BYTEORDER?
On a quick grep we were using it in the kernel -> user
direction but not validating on input. Is that right?
-- 
pw-bot: au

Re: [PATCH nf-next 1/2] netlink: allow be16 and be32 types in all uint policy checks

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote:
> > __NLA_IS_BEINT_TYPE(tp) isn't useful.  NLA_BE16/32 are identical to
> > NLA_U16/32, the only difference is that it tells the netlink validation
> > functions that byteorder conversion might be needed before comparing
> > the value to the policy min/max ones.
> > 
> > After this change all policy macros that can be used with UINT types,
> > such as NLA_POLICY_MASK() can also be used with NLA_BE16/32.
> > 
> > This will be used to validate nf_tables flag attributes which
> > are in bigendian byte order.
> 
> Semi-related, how well do we do with NLA_F_NET_BYTEORDER?

Looks incomplete at best.

> On a quick grep we were using it in the kernel -> user
> direction but not validating on input. Is that right?

Looks like ipset is the only user, it sets it for kernel->user
dir.

I see ipset userspace even sets it on user -> kernel dir but
like you say, its not checked and BE encoding is assumed on
kernel side.

From a quick glance in ipset all Uxx types are always treated as
bigendian, which would mean things should not fall apart if ipset
stops announcing NLA_F_NET_BYTEORDER.  Not sure its worth risking
any breakage though.

I suspect that in practice, given both producer and consumer need
to agree of the meaning of type "12345" anyway its easier to just
agree on the byte ordering as well.

Was there a specific reason for the question?

Re: [PATCH nf-next 1/2] netlink: allow be16 and be32 types in all uint policy checks

[Jakub Kicinski]

On Wed, 19 Jul 2023 04:53:23 +0200 Florian Westphal wrote:
> > On a quick grep we were using it in the kernel -> user
> > direction but not validating on input. Is that right?  
> 
> Looks like ipset is the only user, it sets it for kernel->user
> dir.
> 
> I see ipset userspace even sets it on user -> kernel dir but
> like you say, its not checked and BE encoding is assumed on
> kernel side.
> 
> From a quick glance in ipset all Uxx types are always treated as
> bigendian, which would mean things should not fall apart if ipset
> stops announcing NLA_F_NET_BYTEORDER.  Not sure its worth risking
> any breakage though.
> 
> I suspect that in practice, given both producer and consumer need
> to agree of the meaning of type "12345" anyway its easier to just
> agree on the byte ordering as well.
> 
> Was there a specific reason for the question?

I was wondering what we should do with it going forward. Looks like it
was added by 8f4c1f9b049d ("[NETLINK]: Introduce nested and byteorder
flag to netlink attribute") which says:

    The byte-order flag is yet unused, it's intended use is to
    allow automatic byte order convertions for all atomic types.

That idea clearly never gained traction. If nobody knows of any use of
the flag outside of ipset I'm tempted to send a patch to officially
deprecate it and possibly reuse that bit for something else later on?

Re: [PATCH nf-next 1/2] netlink: allow be16 and be32 types in all uint policy checks

[Jozsef Kadlecsik]

On Wed, 19 Jul 2023, Florian Westphal wrote:

> Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 18 Jul 2023 09:52:29 +0200 Florian Westphal wrote:
> > > __NLA_IS_BEINT_TYPE(tp) isn't useful.  NLA_BE16/32 are identical to
> > > NLA_U16/32, the only difference is that it tells the netlink validation
> > > functions that byteorder conversion might be needed before comparing
> > > the value to the policy min/max ones.
> > > 
> > > After this change all policy macros that can be used with UINT types,
> > > such as NLA_POLICY_MASK() can also be used with NLA_BE16/32.
> > > 
> > > This will be used to validate nf_tables flag attributes which
> > > are in bigendian byte order.
> > 
> > Semi-related, how well do we do with NLA_F_NET_BYTEORDER?
> 
> Looks incomplete at best.
> 
> > On a quick grep we were using it in the kernel -> user
> > direction but not validating on input. Is that right?
> 
> Looks like ipset is the only user, it sets it for kernel->user
> dir.
> 
> I see ipset userspace even sets it on user -> kernel dir but
> like you say, its not checked and BE encoding is assumed on
> kernel side.
> 
> From a quick glance in ipset all Uxx types are always treated as
> bigendian, which would mean things should not fall apart if ipset
> stops announcing NLA_F_NET_BYTEORDER.  Not sure its worth risking
> any breakage though.

Yes, ipset treats all uxx types as netorder.

It checks the presence of the NLA_F_NET_BYTEORDER, see the 
ip_set_attr_netorder() and ip_set_optattr_netorder() functions in 
include/linux/netfilter/ipset/ip_set.h which are then used at input 
validation.

The userspace tool also uses and checks the flag in lib/session.c, but it 
accepts hostorder as well.

> I suspect that in practice, given both producer and consumer need
> to agree of the meaning of type "12345" anyway its easier to just
> agree on the byte ordering as well.
> 
> Was there a specific reason for the question?
 
Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary