[PATCH 0/7] netlink recursive policy validation

[PATCH 0/7] netlink recursive policy validation

[Johannes Berg]

This series adds recursive policy validation, allowing you to specify
complex policies involving nested attributes, both
 * attributes that directly contain new nested attributes
   (NLA_NESTED)
 * attributes that have nested attributes used as an array, where the
   type of each inner attribute is irrelevant/ignored but each of them
   yet again contains nested attributes that should conform to a given
   policy (the new NLA_NESTED_ARRAY)

This is useful for a more compact representation of the policy for the
attributes, which - among other benefits - makes it more easily seen
when reading the code, requiring reading just the policy instead of
digging into all the usage/nested validation code.

In terms of code benefits, it means possibly some validation code can
be removed.

One thing to be aware of: retrofitting this to existing policies may
in fact break userspace - it might have been sending broken but ignored
attributes, which global enforcement of the (nested) policy would now
prevent.

johannes

[PATCH 1/7] netlink: remove NLA_NESTED_COMPAT

[Johannes Berg]

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

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,
@@ -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

[PATCH 2/7] netlink: make validation_data const

[Johannes Berg]

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

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.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 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

Re: [PATCH 2/7] netlink: make validation_data const

[David Ahern]

On 9/19/18 5:08 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> 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.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/netlink.h | 2 +-
>  lib/nlattr.c          | 5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

[PATCH 3/7] netlink: set extack error message in nla_validate()

[Johannes Berg]

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

In nla_parse() we already set this, but it makes sense to
also do it in nla_validate() which already also sets the
bad attribute pointer.

CC: David Ahern <dsahern@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e2e5b38394d5..33404745bec4 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -180,9 +180,13 @@ 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);
+		static const char _msg[] = "Attribute failed policy validation";
+		const char *msg = _msg;
+		int err = validate_nla(nla, maxtype, policy, &msg);
 
 		if (err < 0) {
+			if (extack)
+				extack->_msg = msg;
 			NL_SET_BAD_ATTR(extack, nla);
 			return err;
 		}
-- 
2.14.4

Re: [PATCH 3/7] netlink: set extack error message in nla_validate()

[David Ahern]

On 9/19/18 5:08 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> In nla_parse() we already set this, but it makes sense to
> also do it in nla_validate() which already also sets the
> bad attribute pointer.
> 
> CC: David Ahern <dsahern@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  lib/nlattr.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e2e5b38394d5..33404745bec4 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -180,9 +180,13 @@ 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);
> +		static const char _msg[] = "Attribute failed policy validation";
> +		const char *msg = _msg;
> +		int err = validate_nla(nla, maxtype, policy, &msg);
>  
>  		if (err < 0) {
> +			if (extack)
> +				extack->_msg = msg;
>  			NL_SET_BAD_ATTR(extack, nla);

msg, NL_SET_BAD_ATTR and extack handling all can be done in validate_nla
removing the need for the same message ("Attribute failed policy
validation") to be declared twice and simplifying the extack setting.

>  			return err;
>  		}
> 

Re: [PATCH 3/7] netlink: set extack error message in nla_validate()

[Johannes Berg]

On Wed, 2018-09-19 at 09:20 -0700, David Ahern wrote:

> >  	nla_for_each_attr(nla, head, len, rem) {
> > -		int err = validate_nla(nla, maxtype, policy, NULL);
> > +		static const char _msg[] = "Attribute failed policy validation";
> > +		const char *msg = _msg;
> > +		int err = validate_nla(nla, maxtype, policy, &msg);
> >  
> >  		if (err < 0) {
> > +			if (extack)
> > +				extack->_msg = msg;
> >  			NL_SET_BAD_ATTR(extack, nla);
> 
> msg, NL_SET_BAD_ATTR and extack handling all can be done in validate_nla
> removing the need for the same message ("Attribute failed policy
> validation") to be declared twice and simplifying the extack setting.

Yeah, perhaps I should take another look at that. I didn't want to do
that originally as validate_nla() has so many exit points, but perhaps
it's better to put a goto label there.

FWIW, in the next patch I'm getting rid of the duplication again - I
wanted to have the patch that has an effect (this one, setting the
message) more clearly separated.

johannes

[PATCH 4/7] netlink: combine validate/parse functions

[Johannes Berg]

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

The parse function of course contains validate, but it's
implemented a second time, sharing just the validation
of a single attribute.

Introduce nla_validate_parse() that can be used for both
parsing/validation and only validation, to share code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 76 +++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 33404745bec4..966cd3dcf31b 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -158,6 +158,37 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 	return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack,
+			      struct nlattr **tb)
+{
+	const struct nlattr *nla;
+	int rem;
+
+	nla_for_each_attr(nla, head, len, rem) {
+		static const char _msg[] = "Attribute failed policy validation";
+		const char *msg = _msg;
+		u16 type = nla_type(nla);
+
+		if (policy) {
+			int err = validate_nla(nla, maxtype, policy, &msg);
+
+			if (err < 0) {
+				if (extack)
+					extack->_msg = msg;
+				NL_SET_BAD_ATTR(extack, nla);
+				return err;
+			}
+		}
+
+		if (tb && type > 0 && type <= maxtype)
+			tb[type] = (struct nlattr *)nla;
+	}
+
+	return rem;
+}
+
 /**
  * nla_validate - Validate a stream of attributes
  * @head: head of attribute stream
@@ -176,21 +207,12 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 		 const struct nla_policy *policy,
 		 struct netlink_ext_ack *extack)
 {
-	const struct nlattr *nla;
 	int rem;
 
-	nla_for_each_attr(nla, head, len, rem) {
-		static const char _msg[] = "Attribute failed policy validation";
-		const char *msg = _msg;
-		int err = validate_nla(nla, maxtype, policy, &msg);
+	rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
 
-		if (err < 0) {
-			if (extack)
-				extack->_msg = msg;
-			NL_SET_BAD_ATTR(extack, nla);
-			return err;
-		}
-	}
+	if (rem < 0)
+		return rem;
 
 	return 0;
 }
@@ -244,39 +266,19 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 	      int len, const struct nla_policy *policy,
 	      struct netlink_ext_ack *extack)
 {
-	const struct nlattr *nla;
-	int rem, err;
+	int rem;
 
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
-	nla_for_each_attr(nla, head, len, rem) {
-		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;
-				}
-			}
-
-			tb[type] = (struct nlattr *)nla;
-		}
-	}
+	rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+	if (rem < 0)
+		return rem;
 
 	if (unlikely(rem > 0))
 		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

[PATCH 5/7] netlink: prepare validate extack setting for recursion

[Johannes Berg]

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

In one of my previous patches in this area I introduced code
to pass out just the error message to store in the extack, for
use in NLA_REJECT.

Change this code now to set both the error message and the bad
attribute pointer, and carry around a boolean indicating that
the values have been set.

This will be used in the next patch to allow recursive validation
of nested policies, while preserving the innermost error message
rather than overwriting it with a generic out-level message.

Note that this is a completely local change - code calling one
of nla_parse/nla_validate isn't affected, both functions continue
to overwrite any previously set message with an error generated
here, but in the next patch the message generated may come from
an inner call to nested attribute validation instead, and there
the outer (generic) message shouldn't overwrite the inner.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/nlattr.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 966cd3dcf31b..2b015e43b725 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,7 +69,7 @@ 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, bool *extack_set)
 {
 	const struct nla_policy *pt;
 	int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
@@ -94,8 +94,11 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		break;
 
 	case NLA_REJECT:
-		if (pt->validation_data && error_msg)
-			*error_msg = pt->validation_data;
+		if (pt->validation_data && extack && !*extack_set) {
+			*extack_set = true;
+			extack->_msg = pt->validation_data;
+			NL_SET_BAD_ATTR(extack, nla);
+		}
 		return -EINVAL;
 
 	case NLA_FLAG:
@@ -160,24 +163,25 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 
 static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
 			      const struct nla_policy *policy,
-			      struct netlink_ext_ack *extack,
+			      struct netlink_ext_ack *extack, bool *extack_set,
 			      struct nlattr **tb)
 {
 	const struct nlattr *nla;
 	int rem;
 
 	nla_for_each_attr(nla, head, len, rem) {
-		static const char _msg[] = "Attribute failed policy validation";
-		const char *msg = _msg;
 		u16 type = nla_type(nla);
 
 		if (policy) {
-			int err = validate_nla(nla, maxtype, policy, &msg);
+			int err = validate_nla(nla, maxtype, policy,
+					       extack, extack_set);
 
 			if (err < 0) {
-				if (extack)
-					extack->_msg = msg;
-				NL_SET_BAD_ATTR(extack, nla);
+				if (!*extack_set) {
+					*extack_set = true;
+					NL_SET_ERR_MSG_ATTR(extack, nla,
+							    "Attribute failed policy validation");
+				}
 				return err;
 			}
 		}
@@ -207,9 +211,11 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
 		 const struct nla_policy *policy,
 		 struct netlink_ext_ack *extack)
 {
+	bool extack_set = false;
 	int rem;
 
-	rem = nla_validate_parse(head, len, maxtype, policy, extack, NULL);
+	rem = nla_validate_parse(head, len, maxtype, policy,
+				 extack, &extack_set, NULL);
 
 	if (rem < 0)
 		return rem;
@@ -266,11 +272,13 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
 	      int len, const struct nla_policy *policy,
 	      struct netlink_ext_ack *extack)
 {
+	bool extack_set = false;
 	int rem;
 
 	memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
 
-	rem = nla_validate_parse(head, len, maxtype, policy, extack, tb);
+	rem = nla_validate_parse(head, len, maxtype, policy,
+				 extack, &extack_set, tb);
 	if (rem < 0)
 		return rem;
 
-- 
2.14.4

Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

[David Ahern]

On 9/19/18 5:08 AM, Johannes Berg wrote:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 966cd3dcf31b..2b015e43b725 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -69,7 +69,7 @@ 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, bool *extack_set)

extack_set arg is not needed if you handle the "Attribute failed policy
validation" message and NL_SET_BAD_ATTR here as well.

Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> On 9/19/18 5:08 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 966cd3dcf31b..2b015e43b725 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -69,7 +69,7 @@ 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, bool *extack_set)
> 
> extack_set arg is not needed if you handle the "Attribute failed policy
> validation" message and NL_SET_BAD_ATTR here as well.

I'm not sure that's true, but perhaps you have a better idea than me?

My thought would be to introduce an "error" label in validate_nla(),
that sets up the extack data.

Then we could skip over that if we have a separate message to report,
making the NLA_REJECT case easier.

However, if we do nested validation, I'm not sure it really is that much
easier? We still need to figure out if the nested validation was setting
the message (and bad attr), rather than it having been set before we
even get into this function.

So let's say we have

        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;
                if (attrlen < NLA_HDRLEN)
                        return -ERANGE;
                if (pt->validation_data) {
                        int err;

                        err = nla_validate_parse(nla_data(nla), nla_len(nla),
                                                 pt->len, pt->validation_data,
                                                 extack, extack_set, NULL);
                        if (err < 0)
                                return err;
                }
                break;

right now after all the patches.

The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
I'm not really sure we can cleanly handle the other case?

Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
return points that can fail outside of validate_nla(), or we set up the
extack data there as well, so that once we have a nested
nla_validate_parse() we know that it's been set.

Actually, we need to do that anyway so that we can move the setting into
validate_nla(), and then it should work.

Mechanics aside - I'll take a look later tonight or tomorrow - do you
think the goal/external interface of this makes sense?

johannes

Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

[David Ahern]

On 9/19/18 9:36 AM, Johannes Berg wrote:
> On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
>> On 9/19/18 5:08 AM, Johannes Berg wrote:
>>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>>> index 966cd3dcf31b..2b015e43b725 100644
>>> --- a/lib/nlattr.c
>>> +++ b/lib/nlattr.c
>>> @@ -69,7 +69,7 @@ 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, bool *extack_set)
>>
>> extack_set arg is not needed if you handle the "Attribute failed policy
>> validation" message and NL_SET_BAD_ATTR here as well.
> 
> I'm not sure that's true, but perhaps you have a better idea than me?
> 
> My thought would be to introduce an "error" label in validate_nla(),
> that sets up the extack data.
> 
> Then we could skip over that if we have a separate message to report,
> making the NLA_REJECT case easier.
> 
> However, if we do nested validation, I'm not sure it really is that much
> easier? We still need to figure out if the nested validation was setting
> the message (and bad attr), rather than it having been set before we
> even get into this function.
> 
> So let's say we have
> 
>         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;
>                 if (attrlen < NLA_HDRLEN)
>                         return -ERANGE;
>                 if (pt->validation_data) {
>                         int err;
> 
>                         err = nla_validate_parse(nla_data(nla), nla_len(nla),
>                                                  pt->len, pt->validation_data,
>                                                  extack, extack_set, NULL);
>                         if (err < 0)
>                                 return err;
>                 }
>                 break;
> 
> right now after all the patches.
> 
> The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
> I'm not really sure we can cleanly handle the other case?
> 
> Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
> return points that can fail outside of validate_nla(), or we set up the
> extack data there as well, so that once we have a nested
> nla_validate_parse() we know that it's been set.
> 
> Actually, we need to do that anyway so that we can move the setting into
> validate_nla(), and then it should work.
> 
> Mechanics aside - I'll take a look later tonight or tomorrow - do you
> think the goal/external interface of this makes sense?

If it fails and returns (nested and all) on the first failure it should
be fine. I was thinking something like this (whitespace damaged on paste):

diff --git a/lib/nlattr.c b/lib/nlattr.c
index e335bcafa9e4..f18f0ed3f1cd 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -73,6 +73,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
 {
        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;
@@ -89,7 +90,7 @@ static int validate_nla(const struct nlattr *nla, int
maxtype,
        switch (pt->type) {
        case NLA_FLAG:
                if (attrlen > 0)
-                       return -ERANGE;
+                       goto out_err;
                break;

        case NLA_BITFIELD32:

...
(similar for other error places. the one EINVAL needs to set err first)
...

@@ -156,6 +157,10 @@ static int validate_nla(const struct nlattr *nla,
int maxtype,
        }

        return 0;
+out_err:
+       NL_SET_ERR_MSG_ATTR(extack, nla,
+                           "Attribute failed policy validation");
+       return err;
 }

 /**

Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

[Marcelo Ricardo Leitner]

On Wed, Sep 19, 2018 at 09:44:37AM -0700, David Ahern wrote:
> On 9/19/18 9:36 AM, Johannes Berg wrote:
> > On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> >> On 9/19/18 5:08 AM, Johannes Berg wrote:
> >>> diff --git a/lib/nlattr.c b/lib/nlattr.c
> >>> index 966cd3dcf31b..2b015e43b725 100644
> >>> --- a/lib/nlattr.c
> >>> +++ b/lib/nlattr.c
> >>> @@ -69,7 +69,7 @@ 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, bool *extack_set)
> >>
> >> extack_set arg is not needed if you handle the "Attribute failed policy
> >> validation" message and NL_SET_BAD_ATTR here as well.
> > 
> > I'm not sure that's true, but perhaps you have a better idea than me?
> > 
> > My thought would be to introduce an "error" label in validate_nla(),
> > that sets up the extack data.
> > 
> > Then we could skip over that if we have a separate message to report,
> > making the NLA_REJECT case easier.
> > 
> > However, if we do nested validation, I'm not sure it really is that much
> > easier? We still need to figure out if the nested validation was setting
> > the message (and bad attr), rather than it having been set before we
> > even get into this function.
> > 
> > So let's say we have
> > 
> >         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;
> >                 if (attrlen < NLA_HDRLEN)
> >                         return -ERANGE;
> >                 if (pt->validation_data) {
> >                         int err;
> > 
> >                         err = nla_validate_parse(nla_data(nla), nla_len(nla),
> >                                                  pt->len, pt->validation_data,
> >                                                  extack, extack_set, NULL);
> >                         if (err < 0)
> >                                 return err;
> >                 }
> >                 break;
> > 
> > right now after all the patches.
> > 
> > The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
> > I'm not really sure we can cleanly handle the other case?
> > 
> > Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
> > return points that can fail outside of validate_nla(), or we set up the
> > extack data there as well, so that once we have a nested
> > nla_validate_parse() we know that it's been set.
> > 
> > Actually, we need to do that anyway so that we can move the setting into
> > validate_nla(), and then it should work.
> > 
> > Mechanics aside - I'll take a look later tonight or tomorrow - do you
> > think the goal/external interface of this makes sense?
> 
> If it fails and returns (nested and all) on the first failure it should
> be fine. I was thinking something like this (whitespace damaged on paste):

This will avoid the situation that we were discussing in the older
thread, btw.

  Marcelo

Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion

[Johannes Berg]

On Wed, 2018-09-19 at 16:08 -0300, Marcelo Ricardo Leitner wrote:
> 
> > If it fails and returns (nested and all) on the first failure it should
> > be fine. I was thinking something like this (whitespace damaged on paste):
> 
> This will avoid the situation that we were discussing in the older
> thread, btw.

I think it only avoids the part where we have to worry about "have I
already set this" - which is David's point AFAICT.

I'll reply over to your other email (as I already started writing a
reply there)

johannes

[PATCH 6/7] 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          | 17 +++++++++++++++++
 2 files changed, 28 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 2b015e43b725..c0ea8a78eda6 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -67,6 +67,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
 	return 0;
 }
 
+static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack, bool *extack_set,
+			      struct nlattr **tb);
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy,
 			struct netlink_ext_ack *extack, bool *extack_set)
@@ -148,6 +153,18 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		 */
 		if (attrlen == 0)
 			break;
+		if (attrlen < NLA_HDRLEN)
+			return -ERANGE;
+		if (pt->validation_data) {
+			int err;
+
+			err = nla_validate_parse(nla_data(nla), nla_len(nla),
+						 pt->len, pt->validation_data,
+						 extack, extack_set, NULL);
+			if (err < 0)
+				return err;
+		}
+		break;
 	default:
 		if (pt->len)
 			minlen = pt->len;
-- 
2.14.4

[PATCH 7/7] 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          | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 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 c0ea8a78eda6..234763b41ab7 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -72,6 +72,35 @@ static int nla_validate_parse(const struct nlattr *head, int len, int maxtype,
 			      struct netlink_ext_ack *extack, bool *extack_set,
 			      struct nlattr **tb);
 
+static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
+			      const struct nla_policy *policy,
+			      struct netlink_ext_ack *extack, bool *extack_set)
+{
+	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_parse(nla_data(entry), nla_len(entry),
+					 maxtype, policy, extack, extack_set,
+					 NULL);
+		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, bool *extack_set)
@@ -165,6 +194,24 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 				return err;
 		}
 		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)
+			return -ERANGE;
+		if (pt->validation_data) {
+			int err;
+
+			err = nla_validate_array(nla_data(nla), nla_len(nla),
+						 pt->len, pt->validation_data,
+						 extack, extack_set);
+			if (err < 0)
+				return err;
+		}
+		break;
 	default:
 		if (pt->len)
 			minlen = pt->len;
-- 
2.14.4

Re: [PATCH 0/7] netlink recursive policy validation

[Johannes Berg]

Below is an example of a policy I just built using this.

This may seem rather complex, but that's because the problem is complex
- we want to be able to measure multiple different things (currently
only FTM though) with different peers, and some attributes are shared
(like channel, MAC address) whereas others are method-specific...

I'm sticking all of the measurement request into a single top-level
nl80211 attribute (NL80211_ATTR_PEER_MEASUREMENTS), then in there you
specify global parameters (elided) as well as an array of peers.

Each peer again contains some method-independent parameters (only "CHAN"
shown), as well as request data, which has some parts that are common
and some that are method dependent (yet another nesting level).

All of this gets validated - with the channel data exception in the
comment below - entirely without ever writing another line of code for
it. Yes, we'll still have to write some code to actually use it, but
then we need to worry much less about formatting there.

johannes


static const struct nla_policy
nl80211_pmsr_ftm_req_attr_policy[NL80211_PMSR_FTM_REQ_ATTR_MAX + 1] = {
/* ... */
};

static const struct nla_policy
nl80211_pmsr_req_data_policy[NL80211_PMSR_TYPE_MAX + 1] = {
	[NL80211_PMSR_TYPE_FTM] =
		NLA_POLICY_NESTED(NL80211_PMSR_FTM_REQ_ATTR_MAX,
				  nl80211_pmsr_ftm_req_attr_policy),
};

static const struct nla_policy
nl80211_pmsr_req_attr_policy[NL80211_PMSR_REQ_ATTR_MAX + 1] = {
	[NL80211_PMSR_REQ_ATTR_DATA] =
		NLA_POLICY_NESTED(NL80211_PMSR_TYPE_MAX,
				  nl80211_pmsr_req_data_policy),
/* ... */
};

static const struct nla_policy
nl80211_psmr_peer_attr_policy[NL80211_PMSR_PEER_ATTR_MAX + 1] = {
	/*
	 * we could specify this again to be the top-level policy,
	 * but that would open us up to recursion problems ...
	 */
	[NL80211_PMSR_PEER_ATTR_CHAN] = { .type = NLA_NESTED },
	[NL80211_PMSR_PEER_ATTR_REQ] =
		NLA_POLICY_NESTED(NL80211_PMSR_REQ_ATTR_MAX,
				  nl80211_pmsr_req_attr_policy),
/* ... */
};

static const struct nla_policy
nl80211_pmsr_attr_policy[NL80211_PMSR_ATTR_MAX + 1] = {
	[NL80211_PMSR_ATTR_PEERS] =
		NLA_POLICY_NESTED_ARRAY(NL80211_PMSR_PEER_ATTR_MAX,
					nl80211_psmr_peer_attr_policy),
/* ... */
};

static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
/* ... */
	[NL80211_ATTR_PEER_MEASUREMENTS] =
		NLA_POLICY_NESTED(NL80211_PMSR_FTM_REQ_ATTR_MAX,
				  nl80211_pmsr_attr_policy),
};