Re: [PATCH v2 2/2] netlink: add ethernet address policy types

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Mon, Sep 17, 2018 at 11:57:29AM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Commonly, ethernet addresses are just using a policy of
> 	{ .len = ETH_ALEN }
> which leaves userspace free to send more data than it should,
> which may hide bugs.
> 
> Introduce NLA_EXACT_LEN which checks for exact size, rejecting
> the attribute if it's not exactly that length. Also add
> NLA_EXACT_LEN_WARN which requires the minimum length and will
> warn on longer attributes, for backward compatibility.
> 
> Use these to define NLA_POLICY_ETH_ADDR (new strict policy) and
> NLA_POLICY_ETH_ADDR_COMPAT (compatible policy with warning);
> these are used like this:
> 
>     static const struct nla_policy <name>[...] = {
>         [NL_ATTR_NAME] = NLA_POLICY_ETH_ADDR,
>         ...
>     };
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
> v2: add only NLA_EXACT_LEN/NLA_EXACT_LEN_WARN and build on top
>     of that for ethernet address validation, so it can be extended
>     for other types (e.g. IPv6 addresses)
> ---
>  include/net/netlink.h | 13 +++++++++++++
>  lib/nlattr.c          |  8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index b318b0a9f6c3..318b1ded3833 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -181,6 +181,8 @@ enum {
>  	NLA_S64,
>  	NLA_BITFIELD32,
>  	NLA_REJECT,
> +	NLA_EXACT_LEN,
> +	NLA_EXACT_LEN_WARN,
>  	__NLA_TYPE_MAX,
>  };
>  
> @@ -211,6 +213,10 @@ enum {
>   *                         just like "All other"
>   *    NLA_BITFIELD32       Unused
>   *    NLA_REJECT           Unused
> + *    NLA_EXACT_LEN        Attribute must have exactly this length, otherwise
> + *                         it is rejected.
> + *    NLA_EXACT_LEN_WARN   Attribute should have exactly this length, a warning
> + *                         is logged if it is longer, shorter is rejected.
>   *    All other            Minimum length of attribute payload
>   *
>   * Meaning of `validation_data' field:
> @@ -236,6 +242,13 @@ struct nla_policy {
>  	void            *validation_data;
>  };
>  
> +#define NLA_POLICY_EXACT_LEN(_len)	{ .type = NLA_EXACT_LEN, .len = _len }
> +#define NLA_POLICY_EXACT_LEN_WARN(_len)	{ .type = NLA_EXACT_LEN_WARN, \
> +					  .len = _len }
> +
> +#define NLA_POLICY_ETH_ADDR		NLA_POLICY_EXACT_LEN(ETH_ALEN)
> +#define NLA_POLICY_ETH_ADDR_COMPAT	NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
> +
>  /**
>   * struct nl_info - netlink source information
>   * @nlh: Netlink message header of original request
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 36d74b079151..bb6fe5ed4ecf 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -82,12 +82,18 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>  
>  	BUG_ON(pt->type > NLA_TYPE_MAX);
>  
> -	if (nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) {
> +	if ((nla_attr_len[pt->type] && attrlen != nla_attr_len[pt->type]) ||
> +	    (pt->type == NLA_EXACT_LEN_WARN && attrlen != pt->len)) {
>  		pr_warn_ratelimited("netlink: '%s': attribute type %d has an invalid length.\n",
>  				    current->comm, type);
>  	}
>  
>  	switch (pt->type) {
> +	case NLA_EXACT_LEN:
> +		if (attrlen != pt->len)
> +			return -ERANGE;
> +		break;
> +
>  	case NLA_REJECT:
>  		if (pt->validation_data && error_msg)
>  			*error_msg = pt->validation_data;
> -- 
> 2.14.4
>