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

[Michal Kubecek]

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