Re: [net] netlink: Relax attr validation for fixed length types

[Johannes Berg <johannes@sipsolutions.net>]

On Tue, 2017-12-05 at 12:55 -0700, David Ahern wrote:
> @@ -70,10 +78,9 @@ 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 */

You should update the comment now :-)

And the comment on nla_attr_len as well.

With the comments fixed, this looks fine to me.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>


Since we already have two tables now and may want to add attribute
types with exact enforcement in the future (like the BITFIELD32 one),
I'd actually do things a bit more data driven, but I haven't tested it
right now, and it's better done in net-next after this fix.


diff --git a/lib/nlattr.c b/lib/nlattr.c
index 8bf78b4b78f0..e65eb5400a1a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -15,21 +15,67 @@
 #include <linux/types.h>
 #include <net/netlink.h>
 
-/* 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_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,
+/* netlink validation flags */
+#define NLVF_WARN_WRONG_LEN	BIT(0)
+#define NLVF_REJECT_WRONG_LEN	BIT(1)
+
+static const struct {
+	u8 len, flags;
+} nla_attr_val[NLA_TYPE_MAX + 1] = {
+	[NLA_FLAG] = {
+		.len = 0,
+		.flags = NLVF_REJECT_WRONG_LEN,
+	},
+	[NLA_BITFIELD32] = {
+		/* further checks below */
+		.len = sizeof(struct nla_bitfield32),
+		.flags = NLVF_REJECT_WRONG_LEN,
+	},
+	[NLA_U8] = {
+		.len = sizeof(u8),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_U16] = {
+		.len = sizeof(u16),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_U32] = {
+		.len = sizeof(u32),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_U64] = {
+		.len = sizeof(u64),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_S8] = {
+		.len = sizeof(s8),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_S16] = {
+		.len = sizeof(s16),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_S32] = {
+		.len = sizeof(s32),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_S64] = {
+		.len = sizeof(s64),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_MSECS] = {
+		.len = sizeof(s64),
+		.flags = NLVF_WARN_WRONG_LEN,
+	},
+	[NLA_NESTED] = {
+		/* minimum length */
+		.len = NLA_HDRLEN,
+	},
+	[NLA_STRING] = {
+		/* minimum length, further checks below */
+		.len = 1,
+	},
+	/* others have .len = 0 and no flags */
 };
 
 static int validate_nla_bitfield32(const struct nlattr *nla,
@@ -60,7 +106,7 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy)
 {
 	const struct nla_policy *pt;
-	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+	int minlen, attrlen = nla_len(nla), type = nla_type(nla);
 
 	if (type <= 0 || type > maxtype)
 		return 0;
@@ -69,23 +115,51 @@ 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])
+	switch (pt->type) {
+	case NLA_BINARY:
+		if (pt->len && attrlen > pt->len)
 			return -ERANGE;
-		return 0;
-	}
+		break;
 
-	switch (pt->type) {
-	case NLA_FLAG:
-		if (attrlen > 0)
+	case NLA_NESTED_COMPAT:
+		if (attrlen < pt->len)
+			return -ERANGE;
+		if (attrlen < NLA_ALIGN(pt->len))
+			break;
+		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
+			return -ERANGE;
+		nla = nla_data(nla) + NLA_ALIGN(pt->len);
+		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
 			return -ERANGE;
 		break;
 
-	case NLA_BITFIELD32:
-		if (attrlen != sizeof(struct nla_bitfield32))
+	case NLA_NESTED:
+		/* a nested attributes is allowed to be empty; if its not,
+		 * it must have a size of at least NLA_HDRLEN.
+		 */
+		if (attrlen == 0)
+			break;
+		/* fall through */
+	default:
+		minlen = nla_attr_val[pt->type].len;
+
+		if (nla_attr_val[pt->type].flags & NLVF_REJECT_WRONG_LEN &&
+		    minlen != attrlen)
+			return -ERANGE;
+		if (nla_attr_val[pt->type].flags & NLVF_WARN_WRONG_LEN &&
+		    minlen != attrlen)
+			pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
+					    current->comm, type);
+
+		if (pt->len)
+			minlen = pt->len;
+
+		if (attrlen < minlen)
 			return -ERANGE;
+	}
 
+	switch (pt->type) {
+	case NLA_BITFIELD32:
 		return validate_nla_bitfield32(nla, pt->validation_data);
 
 	case NLA_NUL_STRING:
@@ -99,9 +173,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		/* fall through */
 
 	case NLA_STRING:
-		if (attrlen < 1)
-			return -ERANGE;
-
 		if (pt->len) {
 			char *buf = nla_data(nla);
 
@@ -112,37 +183,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 				return -ERANGE;
 		}
 		break;
-
-	case NLA_BINARY:
-		if (pt->len && attrlen > pt->len)
-			return -ERANGE;
-		break;
-
-	case NLA_NESTED_COMPAT:
-		if (attrlen < pt->len)
-			return -ERANGE;
-		if (attrlen < NLA_ALIGN(pt->len))
-			break;
-		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN)
-			return -ERANGE;
-		nla = nla_data(nla) + NLA_ALIGN(pt->len);
-		if (attrlen < NLA_ALIGN(pt->len) + NLA_HDRLEN + nla_len(nla))
-			return -ERANGE;
-		break;
-	case NLA_NESTED:
-		/* a nested attributes is allowed to be empty; if its not,
-		 * it must have a size of at least NLA_HDRLEN.
-		 */
-		if (attrlen == 0)
-			break;
-	default:
-		if (pt->len)
-			minlen = pt->len;
-		else if (pt->type != NLA_UNSPEC)
-			minlen = nla_attr_minlen[pt->type];
-
-		if (attrlen < minlen)
-			return -ERANGE;
 	}
 
 	return 0;

johannes