[PATCH v2 0/5] netlink: nested policy validation

[PATCH v2 0/5] netlink: nested policy validation

[Johannes Berg]

Ok, I should've tried the idea that David came up with first - it does
in fact make the code quite a bit simpler, and indeed removes the need
for the previously introduced "**error_msg" argument that I hadn't
really liked anyway.

So, changes here are:
 * move setting the bad attr pointer/message into validate_nla()
 * remove the recursion patch since that's no longer needed
 * simply skip the generic bad attr pointer/message setting in
   case of nested nla_validate() failing since that could fail
   only due to validate_nla() failing inside, which already sets
   the extack information

johannes

[PATCH v2 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This isn't used anywhere, so we might as well get rid of it.

Reviewed-by: David Ahern <dsahern-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/net/netlink.h |  2 --
 lib/nlattr.c          | 11 -----------
 2 files changed, 13 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 318b1ded3833..b680fe365e91 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,7 +172,6 @@ enum {
 	NLA_FLAG,
 	NLA_MSECS,
 	NLA_NESTED,
-	NLA_NESTED_COMPAT,
 	NLA_NUL_STRING,
 	NLA_BINARY,
 	NLA_S8,
@@ -203,7 +202,6 @@ enum {
  *    NLA_BINARY           Maximum length of attribute payload
  *    NLA_NESTED           Don't use `len' field -- length verification is
  *                         done by checking len of nested header (or empty)
- *    NLA_NESTED_COMPAT    Minimum length of structure payload
  *    NLA_U8, NLA_U16,
  *    NLA_U32, NLA_U64,
  *    NLA_S8, NLA_S16,
diff --git a/lib/nlattr.c b/lib/nlattr.c
index bb6fe5ed4ecf..120ad569e13d 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -140,17 +140,6 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			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.
-- 
2.14.4

RE: [PATCH v2 1/5] netlink: remove NLA_NESTED_COMPAT

[David Laight]

From: Johannes Berg
> Sent: 19 September 2018 20:49
> This isn't used anywhere, so we might as well get rid of it.
> 
> Reviewed-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/netlink.h |  2 --
>  lib/nlattr.c          | 11 -----------
>  2 files changed, 13 deletions(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 318b1ded3833..b680fe365e91 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -172,7 +172,6 @@ enum {
>  	NLA_FLAG,
>  	NLA_MSECS,
>  	NLA_NESTED,
> -	NLA_NESTED_COMPAT,
>  	NLA_NUL_STRING,
>  	NLA_BINARY,
>  	NLA_S8,
...

Is it safe to remove an item from this emun ?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

>> @@ -172,7 +172,6 @@ enum {
>>  	NLA_FLAG,
>>  	NLA_MSECS,
>>  	NLA_NESTED,
>> -	NLA_NESTED_COMPAT,
>>  	NLA_NUL_STRING,
>>  	NLA_BINARY,
>>  	NLA_S8,
>...
>
>Is it safe to remove an item from this emun ?

I believe it is, since it's not part of uapi. At least as long as you recompile all netlink policies afterwards.

johannes 
-- 
Sent from my phone. 

Re: [PATCH v2 1/5] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

On Thu, 2018-09-20 at 16:56 +0200, Johannes Berg wrote:
> 
> 
> > > @@ -172,7 +172,6 @@ enum {
> > >  	NLA_FLAG,
> > >  	NLA_MSECS,
> > >  	NLA_NESTED,
> > > -	NLA_NESTED_COMPAT,
> > >  	NLA_NUL_STRING,
> > >  	NLA_BINARY,
> > >  	NLA_S8,
> > 
> > ...
> > 
> > Is it safe to remove an item from this emun ?
> 
> I believe it is, since it's not part of uapi. At least as long as you
> recompile all netlink policies afterwards.

That came out confusing. It isn't UAPI, so the renumbering doesn't
matter, at least as long as you don't try to load a module compiled with
one version of the enum into a kernel compiled with the other, or
something strange like that.

johannes

[PATCH v2 2/5] netlink: make validation_data const

[Johannes Berg]

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

The validation data is only used within the policy that
should usually already be const, and isn't changed in any
code that uses it. Therefore, make the validation_data
pointer const.

While at it, remove the duplicate variable in the bitfield
validation that I'd otherwise have to change to const.

Reviewed-by: David Ahern <dsahern-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/net/netlink.h | 2 +-
 lib/nlattr.c          | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index b680fe365e91..0d698215d4d9 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -237,7 +237,7 @@ enum {
 struct nla_policy {
 	u16		type;
 	u16		len;
-	void            *validation_data;
+	const void     *validation_data;
 };
 
 #define NLA_POLICY_EXACT_LEN(_len)	{ .type = NLA_EXACT_LEN, .len = _len }
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 120ad569e13d..e2e5b38394d5 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -45,12 +45,11 @@ static const u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
 };
 
 static int validate_nla_bitfield32(const struct nlattr *nla,
-				   u32 *valid_flags_allowed)
+				   const u32 *valid_flags_mask)
 {
 	const struct nla_bitfield32 *bf = nla_data(nla);
-	u32 *valid_flags_mask = valid_flags_allowed;
 
-	if (!valid_flags_allowed)
+	if (!valid_flags_mask)
 		return -EINVAL;
 
 	/*disallow invalid bit selector */
-- 
2.14.4

[PATCH v2 3/5] netlink: move extack setting into validate_nla()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This unifies the code between nla_parse() which sets the bad
attribute pointer and an error message, and nla_validate()
which only sets the bad attribute pointer.

It also cleans up the code for NLA_REJECT and paves the way
for nested policy validation, as it will allow us to easily
skip setting the "generic" message without any extra args
like the **error_msg now, just passing the extack through is
now enough.

While at it, remove the unnecessary label in nla_parse().

Suggested-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 63 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e2e5b38394d5..ff6d48d7c19a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,10 +69,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
-			const char **error_msg)
+			struct netlink_ext_ack *extack)
 {
 	const struct nla_policy *pt;
 	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+	int err = -ERANGE;
 
 	if (type <= 0 || type > maxtype)
 		return 0;
@@ -90,24 +91,28 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 	switch (pt->type) {
 	case NLA_EXACT_LEN:
 		if (attrlen != pt->len)
-			return -ERANGE;
+			goto out_err;
 		break;
 
 	case NLA_REJECT:
-		if (pt->validation_data && error_msg)
-			*error_msg = pt->validation_data;
+		NL_SET_BAD_ATTR(extack, nla);
+		if (extack && pt->validation_data)
+			 extack->_msg = pt->validation_data;
 		return -EINVAL;
 
 	case NLA_FLAG:
 		if (attrlen > 0)
-			return -ERANGE;
+			goto out_err;
 		break;
 
 	case NLA_BITFIELD32:
 		if (attrlen != sizeof(struct nla_bitfield32))
-			return -ERANGE;
+			goto out_err;
 
-		return validate_nla_bitfield32(nla, pt->validation_data);
+		err = validate_nla_bitfield32(nla, pt->validation_data);
+		if (err)
+			goto out_err;
+		break;
 
 	case NLA_NUL_STRING:
 		if (pt->len)
@@ -115,13 +120,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		else
 			minlen = attrlen;
 
-		if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL)
-			return -EINVAL;
+		if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) {
+			err = -EINVAL;
+			goto out_err;
+		}
 		/* fall through */
 
 	case NLA_STRING:
 		if (attrlen < 1)
-			return -ERANGE;
+			goto out_err;
 
 		if (pt->len) {
 			char *buf = nla_data(nla);
@@ -130,13 +137,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 				attrlen--;
 
 			if (attrlen > pt->len)
-				return -ERANGE;
+				goto out_err;
 		}
 		break;
 
 	case NLA_BINARY:
 		if (pt->len && attrlen > pt->len)
-			return -ERANGE;
+			goto out_err;
 		break;
 
 	case NLA_NESTED:
@@ -152,10 +159,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			minlen = nla_attr_minlen[pt->type];
 
 		if (attrlen < minlen)
-			return -ERANGE;
+			goto out_err;
 	}
 
 	return 0;
+out_err:
+	NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
+	return err;
 }
 
 /**
@@ -180,12 +190,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 	int rem;
 
 	nla_for_each_attr(nla, head, len, rem) {
-		int err = validate_nla(nla, maxtype, policy, NULL);
+		int err = validate_nla(nla, maxtype, policy, extack);
 
-		if (err < 0) {
-			NL_SET_BAD_ATTR(extack, nla);
+		if (err < 0)
 			return err;
-		}
 	}
 
 	return 0;
@@ -241,7 +249,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 	      struct netlink_ext_ack *extack)
 {
 	const struct nlattr *nla;
-	int rem, err;
+	int rem;
 
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
@@ -249,17 +257,12 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 		u16 type = nla_type(nla);
 
 		if (type > 0 && type <= maxtype) {
-			static const char _msg[] = "Attribute failed policy validation";
-			const char *msg = _msg;
-
 			if (policy) {
-				err = validate_nla(nla, maxtype, policy, &msg);
-				if (err < 0) {
-					NL_SET_BAD_ATTR(extack, nla);
-					if (extack)
-						extack->_msg = msg;
-					goto errout;
-				}
+				int err = validate_nla(nla, maxtype, policy,
+						       extack);
+
+				if (err < 0)
+					return err;
 			}
 
 			tb[type] = (struct nlattr *)nla;
@@ -270,9 +273,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 		pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
 				    rem, current->comm);
 
-	err = 0;
-errout:
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(nla_parse);
 
-- 
2.14.4

Re: [PATCH v2 3/5] netlink: move extack setting into validate_nla()

[Johannes Berg]

On Wed, 2018-09-19 at 21:49 +0200, Johannes Berg wrote:
> 
>  	case NLA_REJECT:
> -		if (pt->validation_data && error_msg)
> -			*error_msg = pt->validation_data;
> +		NL_SET_BAD_ATTR(extack, nla);
> +		if (extack && pt->validation_data)
> +			 extack->_msg = pt->validation_data;
>  		return -EINVAL;

Damn. This of course needs to happen only if pt->validation_data is set,
otherwise it needs to "goto out_err" to set the default message.

I'll respin another day with that fixed, and any other comments that
come in until then addressed.

Sorry for the noise :-(

johannes

[PATCH v2 4/5] netlink: allow NLA_NESTED to specify nested policy to validate

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 13 +++++++++++--
 lib/nlattr.c          | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0d698215d4d9..91907852da1c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
  *    NLA_NUL_STRING       Maximum length of string (excluding NUL)
  *    NLA_FLAG             Unused
  *    NLA_BINARY           Maximum length of attribute payload
- *    NLA_NESTED           Don't use `len' field -- length verification is
- *                         done by checking len of nested header (or empty)
+ *    NLA_NESTED           Length verification is done by checking len of
+ *                         nested header (or empty); len field is used if
+ *                         validation_data is also used, for the max attr
+ *                         number in the nested policy.
  *    NLA_U8, NLA_U16,
  *    NLA_U32, NLA_U64,
  *    NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
  *    NLA_REJECT           This attribute is always rejected and validation data
  *                         may point to a string to report as the error instead
  *                         of the generic one in extended ACK.
+ *    NLA_NESTED           Points to a nested policy to validate, must also set
+ *                         `len' to the max attribute number.
+ *                         Note that nla_parse() will validate, but of course not
+ *                         parse, the nested sub-policies.
  *    All other            Unused
  *
  * Example:
@@ -247,6 +253,9 @@ struct nla_policy {
 #define NLA_POLICY_ETH_ADDR		NLA_POLICY_EXACT_LEN(ETH_ALEN)
 #define NLA_POLICY_ETH_ADDR_COMPAT	NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
 
+#define NLA_POLICY_NESTED(maxattr, policy) \
+	{ .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+
 /**
  * struct nl_info - netlink source information
  * @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index ff6d48d7c19a..4cbfc64f6d10 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -152,6 +152,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		 */
 		if (attrlen == 0)
 			break;
+		if (attrlen < NLA_HDRLEN)
+			goto out_err;
+		if (pt->validation_data) {
+			err = nla_validate(nla_data(nla), nla_len(nla), pt->len,
+					   pt->validation_data, extack);
+			if (err < 0) {
+				/*
+				 * return directly to preserve the inner
+				 * error message/attribute pointer
+				 */
+				return err;
+			}
+		}
+		break;
 	default:
 		if (pt->len)
 			minlen = pt->len;
-- 
2.14.4

[PATCH v2 5/5] netlink: add nested array policy validation

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Sometimes nested netlink attributes are just used as arrays, with
the nla_type() of each not being used; we have this in nl80211 and
e.g. NFTA_SET_ELEM_LIST_ELEMENTS.

Add the ability to validate this type of message directly in the
policy, by adding the type NLA_NESTED_ARRAY which does exactly
this: require a first level of nesting but ignore the attribute
type, and then inside each require a second level of nested and
validate those attributes against a given policy (if present).

Note that some nested array types actually require that all of
the entries have the same index, this is possible to express in
a nested policy already, apart from the validation that only the
one allowed type is used.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/netlink.h | 12 +++++++++++-
 lib/nlattr.c          | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 91907852da1c..3698ca8ff92c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,6 +172,7 @@ enum {
 	NLA_FLAG,
 	NLA_MSECS,
 	NLA_NESTED,
+	NLA_NESTED_ARRAY,
 	NLA_NUL_STRING,
 	NLA_BINARY,
 	NLA_S8,
@@ -200,7 +201,8 @@ enum {
  *    NLA_NUL_STRING       Maximum length of string (excluding NUL)
  *    NLA_FLAG             Unused
  *    NLA_BINARY           Maximum length of attribute payload
- *    NLA_NESTED           Length verification is done by checking len of
+ *    NLA_NESTED,
+ *    NLA_NESTED_ARRAY     Length verification is done by checking len of
  *                         nested header (or empty); len field is used if
  *                         validation_data is also used, for the max attr
  *                         number in the nested policy.
@@ -230,6 +232,12 @@ enum {
  *                         `len' to the max attribute number.
  *                         Note that nla_parse() will validate, but of course not
  *                         parse, the nested sub-policies.
+ *    NLA_NESTED_ARRAY     Points to a nested policy to validate, must also set
+ *                         `len' to the max attribute number. The difference to
+ *                         NLA_NESTED is the structure - NLA_NESTED has the
+ *                         nested attributes directly inside, while an array has
+ *                         the nested attributes at another level down and the
+ *                         attributes directly in the nesting don't matter.
  *    All other            Unused
  *
  * Example:
@@ -255,6 +263,8 @@ struct nla_policy {
 
 #define NLA_POLICY_NESTED(maxattr, policy) \
 	{ .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+#define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \
+	{ .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr }
 
 /**
  * struct nl_info - netlink source information
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 4cbfc64f6d10..9d8e1dc71680 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -67,6 +67,34 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 	return 0;
 }
 
+static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack)
+{
+	const struct nlattr *entry;
+	int rem;
+
+	nla_for_each_attr(entry, head, len, rem) {
+		int ret;
+
+		if (nla_len(entry) == 0)
+			continue;
+
+		if (nla_len(entry) < NLA_HDRLEN) {
+			NL_SET_ERR_MSG_ATTR(extack, entry,
+					    "Array element too short");
+			return -ERANGE;
+		}
+
+		ret = nla_validate(nla_data(entry), nla_len(entry),
+				   maxtype, policy, extack);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
 			struct netlink_ext_ack *extack)
@@ -166,6 +194,29 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			}
 		}
 		break;
+	case NLA_NESTED_ARRAY:
+		/* a nested array attribute is allowed to be empty; if its not,
+		 * it must have a size of at least NLA_HDRLEN.
+		 */
+		if (attrlen == 0)
+			break;
+		if (attrlen < NLA_HDRLEN)
+			goto out_err;
+		if (pt->validation_data) {
+			int err;
+
+			err = nla_validate_array(nla_data(nla), nla_len(nla),
+						 pt->len, pt->validation_data,
+						 extack);
+			if (err < 0) {
+				/*
+				 * return directly to preserve the inner
+				 * error message/attribute pointer
+				 */
+				return err;
+			}
+		}
+		break;
 	default:
 		if (pt->len)
 			minlen = pt->len;
-- 
2.14.4