diff mbox series

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

Message ID 20180919120900.28708-6-johannes@sipsolutions.net
State Superseded, archived
Headers show
Series netlink recursive policy validation | expand

Commit Message

Johannes Berg Sept. 19, 2018, 12:08 p.m. UTC 
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(-)

Comments

David Ahern Sept. 19, 2018, 4:28 p.m. UTC | #1 
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.
Johannes Berg Sept. 19, 2018, 4:36 p.m. UTC | #2 
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
David Ahern Sept. 19, 2018, 4:44 p.m. UTC | #3 
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;
 }

 /**
Marcelo Ricardo Leitner Sept. 19, 2018, 7:08 p.m. UTC | #4 
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
Johannes Berg Sept. 19, 2018, 7:09 p.m. UTC | #5 
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
diff mbox series

Patch

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;