[PATCH v2 net-next] net: netlink: Update attr validation to require exact length for some types

[PATCH v2 net-next] net: netlink: Update attr validation to require exact length for some types

[David Ahern]

Attributes using NLA_U* and NLA_S* (where * is 8, 16,32 and 64) are
expected to be an exact length. Split these data types from
nla_attr_minlen into nla_attr_len and update validate_nla to require
the attribute to have exact length for them.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
v2
- fix check in nla_policy_len - reported by kernel test robot

 lib/nlattr.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 3d8295c85505..8bf78b4b78f0 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -15,19 +15,23 @@
 #include <linux/types.h>
 #include <net/netlink.h>
 
-static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+/* for these data types attribute length must be exactly given size */
+static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
 	[NLA_U8]	= sizeof(u8),
 	[NLA_U16]	= sizeof(u16),
 	[NLA_U32]	= sizeof(u32),
 	[NLA_U64]	= sizeof(u64),
-	[NLA_MSECS]	= sizeof(u64),
-	[NLA_NESTED]	= NLA_HDRLEN,
 	[NLA_S8]	= sizeof(s8),
 	[NLA_S16]	= sizeof(s16),
 	[NLA_S32]	= sizeof(s32),
 	[NLA_S64]	= sizeof(s64),
 };
 
+static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
+	[NLA_MSECS]	= sizeof(u64),
+	[NLA_NESTED]	= NLA_HDRLEN,
+};
+
 static int validate_nla_bitfield32(const struct nlattr *nla,
 				   u32 *valid_flags_allowed)
 {
@@ -65,6 +69,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 
 	BUG_ON(pt->type > NLA_TYPE_MAX);
 
+	/* for data types NLA_U* and NLA_S* require exact length */
+	if (nla_attr_len[pt->type]) {
+		if (attrlen != nla_attr_len[pt->type])
+			return -ERANGE;
+		return 0;
+	}
+
 	switch (pt->type) {
 	case NLA_FLAG:
 		if (attrlen > 0)
@@ -191,6 +202,8 @@ nla_policy_len(const struct nla_policy *p, int n)
 	for (i = 0; i < n; i++, p++) {
 		if (p->len)
 			len += nla_total_size(p->len);
+		else if (nla_attr_len[p->type])
+			len += nla_total_size(nla_attr_len[p->type]);
 		else if (nla_attr_minlen[p->type])
 			len += nla_total_size(nla_attr_minlen[p->type]);
 	}
-- 
2.1.4

Re: [PATCH v2 net-next] net: netlink: Update attr validation to require exact length for some types

[David Miller]

From: David Ahern <dsahern@gmail.com>
Date: Tue,  7 Nov 2017 21:59:40 -0800

> Attributes using NLA_U* and NLA_S* (where * is 8, 16,32 and 64) are
> expected to be an exact length. Split these data types from
> nla_attr_minlen into nla_attr_len and update validate_nla to require
> the attribute to have exact length for them.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> v2
> - fix check in nla_policy_len - reported by kernel test robot

Applied, thanks David.

Re: [PATCH v2 net-next] net: netlink: Update attr validation to require exact length for some types

[Jouni Malinen]

Well.. This did not go well with gmail defaults and the mailing list..
Sending this with something safer and plaintext only version to get this
on the mailing list as well:

On Wed, Nov 8, 2017 at 7:59 AM, David Ahern <dsahern@gmail.com> wrote:

> Attributes using NLA_U* and NLA_S* (where * is 8, 16,32 and 64) are
> expected to be an exact length. Split these data types from
> nla_attr_minlen into nla_attr_len and update validate_nla to require
> the attribute to have exact length for them.

While I understand and support this change in general, I have to note that
this resulted in some unfortunate user space regressions that came apparent
when testing Wi-Fi with Linux 4.15-rc1. When a new nl80211 attribute was
added for controlling SMPS modes in 2014 the kernel contribution added this
with NLA_U8 policy while the user space contribution to hostapd used
NLA_PUT_U32. This has apparently been unnoticed until now since the first
byte contained the appropriate value on little endian devices (no one
testing this on big endian hosts?)..

I'll obviously fix the encoding of this attribute in hostapd, but it should
be noted that Linux 4.15 will result in significant functionality issues if
the kernel is updated without a user space fix going in first.

-- 
Jouni Malinen                                            PGP id EFC895FA