[PATCH net-next 0/2] netlink: add range checks for network byte integers

[PATCH net-next 0/2] netlink: add range checks for network byte integers

[Florian Westphal]

NLA_POLICY_MAX() can be used to let netlink core validate that the given
integer attribute is within the given min-max interval.

Add NLA_POLICY_MAX_BE to allow similar range check on unsigned integers
when those are in network byte order (big endian).

First patch adds the netlink change, second patch adds one user.

Florian Westphal (2):
  netlink: introduce NLA_POLICY_MAX_BE
  netfilter: nft_payload: reject out-of-range attributes via policy

 include/net/netlink.h       |  9 +++++++++
 lib/nlattr.c                | 31 +++++++++++++++++++++++++++----
 net/netfilter/nft_payload.c |  6 +++---
 3 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.35.1

[PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Florian Westphal]

netlink allows to specify allowed ranges for integer types.
Unfortunately, nfnetlink passes integers in big endian, so the existing
NLA_POLICY_MAX() cannot be used.

At the moment, nfnetlink users, such as nf_tables, need to resort to
programmatic checking via helpers such as nft_parse_u32_check().

This is both cumbersome and error prone.  This adds NLA_POLICY_MAX_BE
which adds range check support for BE16, BE32 and BE64 integers.

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

diff --git a/include/net/netlink.h b/include/net/netlink.h
index e658d18afa67..4418b1981e31 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -325,6 +325,7 @@ struct nla_policy {
 		struct netlink_range_validation_signed *range_signed;
 		struct {
 			s16 min, max;
+			u8 network_byte_order:1;
 		};
 		int (*validate)(const struct nlattr *attr,
 				struct netlink_ext_ack *extack);
@@ -418,6 +419,14 @@ struct nla_policy {
 	.type = NLA_ENSURE_INT_OR_BINARY_TYPE(tp),	\
 	.validation_type = NLA_VALIDATE_MAX,		\
 	.max = _max,					\
+	.network_byte_order = 0,			\
+}
+
+#define NLA_POLICY_MAX_BE(tp, _max) {			\
+	.type = NLA_ENSURE_UINT_TYPE(tp),		\
+	.validation_type = NLA_VALIDATE_MAX,		\
+	.max = _max,					\
+	.network_byte_order = 1,			\
 }
 
 #define NLA_POLICY_MASK(tp, _mask) {			\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 86029ad5ead4..40f22b177d69 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -159,6 +159,31 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
 	}
 }
 
+static u64 nla_get_attr_bo(const struct nla_policy *pt,
+			   const struct nlattr *nla)
+{
+	switch (pt->type) {
+	case NLA_U16:
+		if (pt->network_byte_order)
+			return ntohs(nla_get_be16(nla));
+
+		return nla_get_u16(nla);
+	case NLA_U32:
+		if (pt->network_byte_order)
+			return ntohl(nla_get_be32(nla));
+
+		return nla_get_u32(nla);
+	case NLA_U64:
+		if (pt->network_byte_order)
+			return be64_to_cpu(nla_get_be64(nla));
+
+		return nla_get_u64(nla);
+	}
+
+	WARN_ON_ONCE(1);
+	return 0;
+}
+
 static int nla_validate_range_unsigned(const struct nla_policy *pt,
 				       const struct nlattr *nla,
 				       struct netlink_ext_ack *extack,
@@ -172,12 +197,10 @@ static int nla_validate_range_unsigned(const struct nla_policy *pt,
 		value = nla_get_u8(nla);
 		break;
 	case NLA_U16:
-		value = nla_get_u16(nla);
-		break;
 	case NLA_U32:
-		value = nla_get_u32(nla);
-		break;
 	case NLA_U64:
+		value = nla_get_attr_bo(pt, nla);
+		break;
 	case NLA_MSECS:
 		value = nla_get_u64(nla);
 		break;
-- 
2.35.1

[PATCH net-next 2/2] netfilter: nft_payload: reject out-of-range attributes via policy

[Florian Westphal]

Now that nla_policy allows range checks for bigendian data make use of
this to reject such attributes.  At this time, reject happens later
from the init or select_ops callbacks, but its prone to errors.

In the future, new attributes can be handled via NLA_POLICY_MAX_BE
and exiting ones can be converted one by one.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_payload.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index eb0e40c29712..088244f9d838 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -173,10 +173,10 @@ static const struct nla_policy nft_payload_policy[NFTA_PAYLOAD_MAX + 1] = {
 	[NFTA_PAYLOAD_SREG]		= { .type = NLA_U32 },
 	[NFTA_PAYLOAD_DREG]		= { .type = NLA_U32 },
 	[NFTA_PAYLOAD_BASE]		= { .type = NLA_U32 },
-	[NFTA_PAYLOAD_OFFSET]		= { .type = NLA_U32 },
-	[NFTA_PAYLOAD_LEN]		= { .type = NLA_U32 },
+	[NFTA_PAYLOAD_OFFSET]		= NLA_POLICY_MAX_BE(NLA_U32, 255),
+	[NFTA_PAYLOAD_LEN]		= NLA_POLICY_MAX_BE(NLA_U32, 255),
 	[NFTA_PAYLOAD_CSUM_TYPE]	= { .type = NLA_U32 },
-	[NFTA_PAYLOAD_CSUM_OFFSET]	= { .type = NLA_U32 },
+	[NFTA_PAYLOAD_CSUM_OFFSET]	= NLA_POLICY_MAX_BE(NLA_U32, 255),
 	[NFTA_PAYLOAD_CSUM_FLAGS]	= { .type = NLA_U32 },
 };
 
-- 
2.35.1

Re: [PATCH net-next 0/2] netlink: add range checks for network byte integers

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon,  5 Sep 2022 12:09:35 +0200 you wrote:
> NLA_POLICY_MAX() can be used to let netlink core validate that the given
> integer attribute is within the given min-max interval.
> 
> Add NLA_POLICY_MAX_BE to allow similar range check on unsigned integers
> when those are in network byte order (big endian).
> 
> First patch adds the netlink change, second patch adds one user.
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] netlink: introduce NLA_POLICY_MAX_BE
    https://git.kernel.org/netdev/net-next/c/08724ef69907
  - [net-next,2/2] netfilter: nft_payload: reject out-of-range attributes via policy
    https://git.kernel.org/netdev/net-next/c/e7af210e6dd0

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

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Jakub Kicinski]

On Mon,  5 Sep 2022 12:09:36 +0200 Florian Westphal wrote:
>  		struct {
>  			s16 min, max;
> +			u8 network_byte_order:1;
>  		};

This makes the union 64bit even on 32bit systems.
Do we care? Should we accept that and start using
full 64bits in other validation members?

We can quite easily steal a bit elsewhere, which
I reckon may be the right thing to do, but I thought
I'd ask.

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Johannes Berg]

On Thu, 2022-10-27 at 13:31 -0700, Jakub Kicinski wrote:
> On Mon,  5 Sep 2022 12:09:36 +0200 Florian Westphal wrote:
> >  		struct {
> >  			s16 min, max;
> > +			u8 network_byte_order:1;
> >  		};
> 
> This makes the union 64bit even on 32bit systems.
> Do we care? Should we accept that and start using
> full 64bits in other validation members?
> 
> We can quite easily steal a bit elsewhere, which
> I reckon may be the right thing to do, but I thought
> I'd ask.

Personally, I guess I might have preferred to steal a bit out of the
type or validation_type. We have a lot of these structures... and I'd
guess 32-bit systems are typically more memory constrained.

In fact we could easily just have three extra types NLA_BE16, NLA_BE32
and NLA_BE64 types without even stealing a bit? We already have
NLA_MSECS which is basically the same as NLA_U64 but just with the
additional semantic information, for example.

johannes

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Florian Westphal]

Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2022-10-27 at 13:31 -0700, Jakub Kicinski wrote:
> > On Mon,  5 Sep 2022 12:09:36 +0200 Florian Westphal wrote:
> > >  		struct {
> > >  			s16 min, max;
> > > +			u8 network_byte_order:1;
> > >  		};
> > 
> > This makes the union 64bit even on 32bit systems.
> > Do we care? Should we accept that and start using
> > full 64bits in other validation members?
> > 
> > We can quite easily steal a bit elsewhere, which
> > I reckon may be the right thing to do, but I thought
> > I'd ask.

I'm fine with scraping the marker elsewhere.

> In fact we could easily just have three extra types NLA_BE16, NLA_BE32
> and NLA_BE64 types without even stealing a bit?

Sure, I can make a patch if there is consensus that new types are the
way to go.

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Jakub Kicinski]

On Fri, 28 Oct 2022 01:35:00 +0200 Florian Westphal wrote:
> > In fact we could easily just have three extra types NLA_BE16, NLA_BE32
> > and NLA_BE64 types without even stealing a bit?  
> 
> Sure, I can make a patch if there is consensus that new types are the
> way to go.

The NLA_BE* idea seems appealing, but if the implementation gets
tedious either way works for me.

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Florian Westphal]

Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri, 28 Oct 2022 01:35:00 +0200 Florian Westphal wrote:
> > > In fact we could easily just have three extra types NLA_BE16, NLA_BE32
> > > and NLA_BE64 types without even stealing a bit?  
> > 
> > Sure, I can make a patch if there is consensus that new types are the
> > way to go.
> 
> The NLA_BE* idea seems appealing, but if the implementation gets
> tedious either way works for me.

Doesn't look too bad.  I plan to do a formal submission once I'm back
home.

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 4418b1981e31..a843c8eb75cc 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -181,6 +181,8 @@ enum {
 	NLA_S64,
 	NLA_BITFIELD32,
 	NLA_REJECT,
+	NLA_BE16,
+	NLA_BE32,
 	__NLA_TYPE_MAX,
 };
 
@@ -231,6 +233,7 @@ enum nla_policy_validation {
  *    NLA_U32, NLA_U64,
  *    NLA_S8, NLA_S16,
  *    NLA_S32, NLA_S64,
+ *    NLA_BE16, NLA_BE32,
  *    NLA_MSECS            Leaving the length field zero will verify the
  *                         given type fits, using it verifies minimum length
  *                         just like "All other"
@@ -261,6 +264,8 @@ enum nla_policy_validation {
  *    NLA_U16,
  *    NLA_U32,
  *    NLA_U64,
+ *    NLA_BE16,
+ *    NLA_BE32,
  *    NLA_S8,
  *    NLA_S16,
  *    NLA_S32,
@@ -325,7 +330,6 @@ struct nla_policy {
 		struct netlink_range_validation_signed *range_signed;
 		struct {
 			s16 min, max;
-			u8 network_byte_order:1;
 		};
 		int (*validate)(const struct nlattr *attr,
 				struct netlink_ext_ack *extack);
@@ -369,6 +373,8 @@ struct nla_policy {
 	(tp == NLA_U8 || tp == NLA_U16 || tp == NLA_U32 || tp == NLA_U64)
 #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)			\
@@ -382,6 +388,7 @@ 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)		\
@@ -389,6 +396,8 @@ 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),	\
@@ -419,14 +428,6 @@ struct nla_policy {
 	.type = NLA_ENSURE_INT_OR_BINARY_TYPE(tp),	\
 	.validation_type = NLA_VALIDATE_MAX,		\
 	.max = _max,					\
-	.network_byte_order = 0,			\
-}
-
-#define NLA_POLICY_MAX_BE(tp, _max) {			\
-	.type = NLA_ENSURE_UINT_TYPE(tp),		\
-	.validation_type = NLA_VALIDATE_MAX,		\
-	.max = _max,					\
-	.network_byte_order = 1,			\
 }
 
 #define NLA_POLICY_MASK(tp, _mask) {			\
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 40f22b177d69..b67a53e29b8f 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -124,10 +124,12 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
 		range->max = U8_MAX;
 		break;
 	case NLA_U16:
+	case NLA_BE16:
 	case NLA_BINARY:
 		range->max = U16_MAX;
 		break;
 	case NLA_U32:
+	case NLA_BE32:
 		range->max = U32_MAX;
 		break;
 	case NLA_U64:
@@ -159,31 +161,6 @@ void nla_get_range_unsigned(const struct nla_policy *pt,
 	}
 }
 
-static u64 nla_get_attr_bo(const struct nla_policy *pt,
-			   const struct nlattr *nla)
-{
-	switch (pt->type) {
-	case NLA_U16:
-		if (pt->network_byte_order)
-			return ntohs(nla_get_be16(nla));
-
-		return nla_get_u16(nla);
-	case NLA_U32:
-		if (pt->network_byte_order)
-			return ntohl(nla_get_be32(nla));
-
-		return nla_get_u32(nla);
-	case NLA_U64:
-		if (pt->network_byte_order)
-			return be64_to_cpu(nla_get_be64(nla));
-
-		return nla_get_u64(nla);
-	}
-
-	WARN_ON_ONCE(1);
-	return 0;
-}
-
 static int nla_validate_range_unsigned(const struct nla_policy *pt,
 				       const struct nlattr *nla,
 				       struct netlink_ext_ack *extack,
@@ -197,9 +174,13 @@ static int nla_validate_range_unsigned(const struct nla_policy *pt,
 		value = nla_get_u8(nla);
 		break;
 	case NLA_U16:
+		value = nla_get_u16(nla);
+		break;
 	case NLA_U32:
+		value = nla_get_u32(nla);
+		break;
 	case NLA_U64:
-		value = nla_get_attr_bo(pt, nla);
+		value = nla_get_u64(nla);
 		break;
 	case NLA_MSECS:
 		value = nla_get_u64(nla);
@@ -207,6 +188,12 @@ static int nla_validate_range_unsigned(const struct nla_policy *pt,
 	case NLA_BINARY:
 		value = nla_len(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;
 	}
@@ -334,6 +321,8 @@ static int nla_validate_int_range(const struct nla_policy *pt,
 	case NLA_U64:
 	case NLA_MSECS:
 	case NLA_BINARY:
+	case NLA_BE16:
+	case NLA_BE32:
 		return nla_validate_range_unsigned(pt, nla, extack, validate);
 	case NLA_S8:
 	case NLA_S16:
diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index 088244f9d838..4edd899aeb9b 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -173,10 +173,10 @@ static const struct nla_policy nft_payload_policy[NFTA_PAYLOAD_MAX + 1] = {
 	[NFTA_PAYLOAD_SREG]		= { .type = NLA_U32 },
 	[NFTA_PAYLOAD_DREG]		= { .type = NLA_U32 },
 	[NFTA_PAYLOAD_BASE]		= { .type = NLA_U32 },
-	[NFTA_PAYLOAD_OFFSET]		= NLA_POLICY_MAX_BE(NLA_U32, 255),
-	[NFTA_PAYLOAD_LEN]		= NLA_POLICY_MAX_BE(NLA_U32, 255),
+	[NFTA_PAYLOAD_OFFSET]		= NLA_POLICY_MAX(NLA_BE32, 255),
+	[NFTA_PAYLOAD_LEN]		= NLA_POLICY_MAX(NLA_BE32, 255),
 	[NFTA_PAYLOAD_CSUM_TYPE]	= { .type = NLA_U32 },
-	[NFTA_PAYLOAD_CSUM_OFFSET]	= NLA_POLICY_MAX_BE(NLA_U32, 255),
+	[NFTA_PAYLOAD_CSUM_OFFSET]	= NLA_POLICY_MAX(NLA_BE32, 255),
 	[NFTA_PAYLOAD_CSUM_FLAGS]	= { .type = NLA_U32 },
 };
 

Re: [PATCH net-next 1/2] netlink: introduce NLA_POLICY_MAX_BE

[Jakub Kicinski]

On Fri, 28 Oct 2022 12:16:13 +0200 Florian Westphal wrote:
> > The NLA_BE* idea seems appealing, but if the implementation gets
> > tedious either way works for me.  
> 
> Doesn't look too bad.  I plan to do a formal submission once I'm back
> home.

Neat, FWIW:

Reviewed-by: Jakub Kicinski <kuba@kernel.org>