Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

From: Michal Kubecek
Date: Thu May 02 2019 - 09:14:39 EST


On Thu, May 02, 2019 at 02:54:56PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > Add new validation flag NL_VALIDATE_NESTED which adds three consistency
> > checks of NLA_F_NESTED_FLAG:
> >
> > - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
> > - the flag is not set on attributes with other policies except NLA_UNSPEC
> > - the flag is set on attribute passed to nla_parse_nested()
>
> Looks good to me!
>
> > @@ -415,7 +418,8 @@ enum netlink_validation {
> > #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> > NL_VALIDATE_MAXTYPE |\
> > NL_VALIDATE_UNSPEC |\
> > - NL_VALIDATE_STRICT_ATTRS)
> > + NL_VALIDATE_STRICT_ATTRS |\
> > + NL_VALIDATE_NESTED)
>
> This is fine _right now_, but in general we cannot keep adding here
> after the next release :-)

Right, that's why I would like to get this into the same cycle as your
series.

> > int netlink_rcv_skb(struct sk_buff *skb,
> > int (*cb)(struct sk_buff *, struct nlmsghdr *,
> > @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
> > const struct nla_policy *policy,
> > struct netlink_ext_ack *extack)
> > {
> > + if (!(nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
>
> Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
> attribute expected" could result in a lot of headscratching (without
> looking at the code) because it looks nested if you do nla_nest_start()
> etc.

How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?

>
> > + return -EINVAL;
> > + }
> > return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
> > NL_VALIDATE_STRICT, extack);
>
> I'd probably put a blank line there but ymmv.

OK

> > }
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index adc919b32bf9..92da65cb6637 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> > }
> > }
> >
> > + if (validate & NL_VALIDATE_NESTED) {
> > + if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
> > + !(nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla,
> > + "nested attribute expected");
> > + return -EINVAL;
> > + }
> > + if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
> > + pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
> > + NL_SET_ERR_MSG_ATTR(extack, nla,
> > + "nested attribute not expected");
> > + return -EINVAL;
>
> Same comment here wrt. the messages, I think they should more explicitly
> refer to the flag.
>
> johannes
>
> (PS: if you CC me on this address I generally can respond quicker)

I'll try to keep that in mind.

Michal