netdev - Re: [PATCH net-next v2] net: ip6mr: add RTM

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 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:   Mon, 4 Jul 2022 13:44:52 +0300
From:   Nikolay Aleksandrov <razor@...ckwall.org>
To:     David Lamparter <equinox@...c24.net>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next v2] net: ip6mr: add RTM_GETROUTE netlink op

On 04/07/2022 13:38, David Lamparter wrote:
> On Mon, Jul 04, 2022 at 01:22:36PM +0300, Nikolay Aleksandrov wrote:
>> On 04/07/2022 12:58, David Lamparter wrote:
>>> +const struct nla_policy rtm_ipv6_mr_policy[RTA_MAX + 1] = {
>>> +	[RTA_UNSPEC]		= { .strict_start_type = RTA_UNSPEC },
>>
>> I don't think you need to add RTA_UNSPEC, nlmsg_parse() would reject
>> it due to NL_VALIDATE_STRICT
> 
> Will remove it.
> 
>>> +	if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
>>> +		NL_SET_ERR_MSG(extack, "ipv6: Invalid header for multicast route get request");
>>> +		return -EINVAL;
>>> +	}
>>
>> I think you can drop this check if you...
>>
>>> +
>>> +	rtm = nlmsg_data(nlh);
>>> +	if ((rtm->rtm_src_len && rtm->rtm_src_len != 128) ||
>>> +	    (rtm->rtm_dst_len && rtm->rtm_dst_len != 128) ||
>>> +	    rtm->rtm_tos || rtm->rtm_table || rtm->rtm_protocol ||
>>> +	    rtm->rtm_scope || rtm->rtm_type || rtm->rtm_flags) {
>>> +		NL_SET_ERR_MSG(extack,
>>> +			       "ipv6: Invalid values in header for multicast route get request");
>>> +		return -EINVAL;
>>> +	}
>>
>> ...move these after nlmsg_parse() because it already does the hdrlen
>> check for you
> 
> Indeed it does.  Moving it down.
> 
> [...]
>>> +	/* rtm_ipv6_mr_policy does not list other attributes right now, but
>>> +	 * future changes may reuse rtm_ipv6_mr_policy with adding further
>>> +	 * attrs.  Enforce the subset.
>>> +	 */
>>> +	for (i = 0; i <= RTA_MAX; i++) {
>>> +		if (!tb[i])
>>> +			continue;
>>> +
>>> +		switch (i) {
>>> +		case RTA_SRC:
>>> +		case RTA_DST:
>>> +		case RTA_TABLE:
>>> +			break;
>>> +		default:
>>> +			NL_SET_ERR_MSG_ATTR(extack, tb[i],
>>> +					    "ipv6: Unsupported attribute in multicast route get request");
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>
>> I think you can skip this loop as well, nlmsg_parse() shouldn't allow attributes that
>> don't have policy defined when policy is provided (i.e. they should show up as NLA_UNSPEC
>> and you should get "Error: Unknown attribute type.").
> 
> I left it in with the comment above:
> 
>>> +	/* rtm_ipv6_mr_policy does not list other attributes right now, but
>>> +	 * future changes may reuse rtm_ipv6_mr_policy with adding further
>>> +	 * attrs.  Enforce the subset.
>>> +	 */
> 
> ... to try and avoid silently starting to accept more attributes if/when
> future patches add other netlink operations reusing the same policy but
> with adding new attributes.
> 

They really should be using policies specific to their actions with only the allowed
attributes. Re-using this policy is ok only if those match, otherwise it's a bug IMO.

> But I don't feel particularly about this - shall I remove it?  (just
> confirming with the rationale above)
> 

I don't have a preference either, IMO if anyone re-uses this policy without making
sure the same attributes and types are needed is adding buggy code. Actually the thing
that I like about keeping the loop is the more specific error message, let's see what
others think.

>>> +	struct net *net = sock_net(in_skb->sk);
>>> +	struct nlattr *tb[RTA_MAX + 1];
>>> +	struct sk_buff *skb = NULL;
>>> +	struct mfc6_cache *cache;
>>> +	struct mr_table *mrt;
>>> +	struct in6_addr src = {}, grp = {};
>>
>> reverse xmas tree order
> 
> Ah.  Wasn't aware of that coding style aspect.  Fixing.
> 
> Thanks for the review!
> 
> 
> -David/equi