Re: [PATCH net-next 14/20] net/neighbor: Update neigh_dump_info for strict data checking

[Christian Brauner <christian@brauner.io>]

On Thu, Oct 04, 2018 at 02:33:49PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Update neigh_dump_info for strict data checking. If the flag is set,
> the dump request is expected to have an ndmsg struct as the header
> potentially followed by one or more attributes. Any data passed in the
> header or as an attribute is taken as a request to influence the data
> returned. Only values supported by the dump handler are allowed to be
> non-0 or set in the request. At the moment only the NDA_IFINDEX and
> NDA_MASTER attributes are supported.
> 
> Existing code does not fail the dump if nlmsg_parse fails. That behavior
> is kept for non-strict checking.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/core/neighbour.c | 59 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index b06f794bf91e..3130d010b7ad 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2428,13 +2428,14 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
>  
>  static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> +	struct netlink_ext_ack *extack = cb->extack;
>  	const struct nlmsghdr *nlh = cb->nlh;
>  	struct neigh_dump_filter filter = {};
>  	struct nlattr *tb[NDA_MAX + 1];
>  	struct neigh_table *tbl;
>  	int t, family, s_t;
>  	int proxy = 0;
> -	int err;
> +	int err, i;
>  
>  	family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
>  
> @@ -2445,20 +2446,56 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>  	    ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY)
>  		proxy = 1;
>  
> -	err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL,
> -			  cb->extack);

So right, this seems like there was precedent in the kernel where an
nlmsg_parse() would fail and a warning as generated in the logs.
Anyway, as we have decided to not cause such warnings to be printed it
would make sense to make the whole nlmsg_parse() conditional and move it
under the if (strict) branch.

> -	if (!err) {
> -		if (tb[NDA_IFINDEX]) {
> -			if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
> -				return -EINVAL;
> -			filter.dev_idx = nla_get_u32(tb[NDA_IFINDEX]);
> +	if (cb->strict_check) {
> +		struct ndmsg *ndm;
> +
> +		if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ndm))) {
> +			NL_SET_ERR_MSG(extack, "Invalid header");
> +			return -EINVAL;
> +		}
> +
> +		ndm = nlmsg_data(nlh);
> +		if (ndm->ndm_pad1  || ndm->ndm_pad2  || ndm->ndm_ifindex ||
> +		    ndm->ndm_state || ndm->ndm_flags || ndm->ndm_type) {
> +			NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> +			return -EINVAL;
>  		}
> -		if (tb[NDA_MASTER]) {
> -			if (nla_len(tb[NDA_MASTER]) != sizeof(u32))
> +	}
> +
> +	err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL, extack);
> +	if (err < 0) {
> +		if (cb->strict_check)
> +			return -EINVAL;
> +		goto walk_entries;
> +	}
> +
> +	for (i = 0; i <= NDA_MAX; ++i) {
> +		if (!tb[i])
> +			continue;
> +		switch (i) {
> +		case NDA_IFINDEX:
> +			if (nla_len(tb[i]) != sizeof(u32)) {
> +				NL_SET_ERR_MSG(extack, "Invalid IFINDEX attribute");
>  				return -EINVAL;
> -			filter.master_idx = nla_get_u32(tb[NDA_MASTER]);
> +			}
> +			filter.dev_idx = nla_get_u32(tb[i]);
> +			break;
> +		case NDA_MASTER:
> +			if (nla_len(tb[i]) != sizeof(u32)) {
> +				NL_SET_ERR_MSG(extack, "Invalid MASTER attribute");
> +				return -EINVAL;
> +			}
> +			filter.master_idx = nla_get_u32(tb[i]);
> +			break;
> +		default:
> +			if (cb->strict_check) {
> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> +				return -EINVAL;
> +			}
>  		}
>  	}
> +
> +walk_entries:
>  	s_t = cb->args[0];
>  
>  	for (t = 0; t < NEIGH_NR_TABLES; t++) {
> -- 
> 2.11.0
>