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]

Message-ID: <1537374995.10305.47.camel@sipsolutions.net>
Date:   Wed, 19 Sep 2018 18:36:35 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     David Ahern <dsahern@...il.com>, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH 5/7] netlink: prepare validate extack setting for
 recursion

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