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

Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
exact length for some types") requires attributes using types NLA_U* and
NLA_S* to have an exact length. This change is exposing bugs in various
userspace commands that are sending attributes with an invalid length
(e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
the commands are clearly broken and need to be fixed, users are arguing
that the sudden change in enforcement is breaking older commands on
newer kernels for use cases that otherwise "worked".

Relax the validation to print a warning mesage similar to what is done
for messages containing extra bytes after parsing.

Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
Signed-off-by: David Ahern <dsahern@gmail.com>
---
 lib/nlattr.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

From: David Ahern <dsahern@gmail.com>
Date: Tue,  5 Dec 2017 12:55:40 -0700

> Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
> exact length for some types") requires attributes using types NLA_U* and
> NLA_S* to have an exact length. This change is exposing bugs in various
> userspace commands that are sending attributes with an invalid length
> (e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
> the commands are clearly broken and need to be fixed, users are arguing
> that the sudden change in enforcement is breaking older commands on
> newer kernels for use cases that otherwise "worked".
> 
> Relax the validation to print a warning mesage similar to what is done
> for messages containing extra bytes after parsing.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: David Ahern <dsahern@gmail.com>

Johannes, please review.

> ---
>  lib/nlattr.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 8bf78b4b78f0..6122662906c8 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -28,8 +28,16 @@ static const u8 nla_attr_len[NLA_TYPE_MAX+1] = {
>  };
>  
>  static const u8 nla_attr_minlen[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 int validate_nla_bitfield32(const struct nlattr *nla,
> @@ -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 */
> -	if (nla_attr_len[pt->type]) {
> -		if (attrlen != nla_attr_len[pt->type])
> -			return -ERANGE;
> -		return 0;
> +	if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
> +		pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
> +				    current->comm, type);
>  	}
>  
>  	switch (pt->type) {
> -- 
> 2.11.0
>

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

From: David Ahern <dsahern@gmail.com>
Date: Tue,  5 Dec 2017 12:55:40 -0700

> Commit 28033ae4e0f5 ("net: netlink: Update attr validation to require
> exact length for some types") requires attributes using types NLA_U* and
> NLA_S* to have an exact length. This change is exposing bugs in various
> userspace commands that are sending attributes with an invalid length
> (e.g., attribute has type NLA_U8 and userspace sends NLA_U32). While
> the commands are clearly broken and need to be fixed, users are arguing
> that the sudden change in enforcement is breaking older commands on
> newer kernels for use cases that otherwise "worked".
> 
> Relax the validation to print a warning mesage similar to what is done
> for messages containing extra bytes after parsing.
> 
> Fixes: 28033ae4e0f5 ("net: netlink: Update attr validation to require exact length for some types")
> Signed-off-by: David Ahern <dsahern@gmail.com>

David, please address Johannes's feedback and I'll apply this.

Thank you.