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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Sep 2018 16:08:51 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Johannes Berg <johannes@...solutions.net>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH 5/7] netlink: prepare validate extack setting for
 recursion

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