netdev - Re: [PATCH net-next 3/3] netlink: add validation of NLA_F

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:   Thu, 02 May 2019 14:54:56 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "David S. Miller" <davem@...emloft.net>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        David Ahern <dsahern@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED
 flag

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 :-)

>  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.

> +		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.

>  }
> 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)