netdev - Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump

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]

Message-ID: <20181007102908.4gdbr7d74untlo7k@brauner.io>
Date:   Sun, 7 Oct 2018 12:29:09 +0200
From:   Christian Brauner <christian@...uner.io>
To:     David Ahern <dsahern@...il.com>
Cc:     David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, jbenc@...hat.com, stephen@...workplumber.org
Subject: Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for
 strict data checking

On Fri, Oct 05, 2018 at 01:22:24PM -0600, David Ahern wrote:
> On 10/5/18 11:59 AM, Christian Brauner wrote:
> >> +	err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack);
> >> +	if (err < 0) {
> >> +		if (cb->strict_check)
> >> +			return -EINVAL;
> >> +		goto walk_entries;
> >> +	}
> >>  
> >> -		if (master_idx || kind_ops)
> >> -			flags |= NLM_F_DUMP_FILTERED;
> >> +	for (i = 0; i <= IFLA_MAX; ++i) {
> >> +		if (!tb[i])
> >> +			continue;
> >> +		switch (i) {
> >> +		case IFLA_TARGET_NETNSID:
> >> +			netnsid = nla_get_s32(tb[i]);
> >> +			tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> >> +			if (IS_ERR(tgt_net)) {
> >> +				NL_SET_ERR_MSG(extack, "Invalid target namespace id");
> >> +				return PTR_ERR(tgt_net);
> >> +			}
> >> +			break;
> >> +		case IFLA_EXT_MASK:
> >> +			ext_filter_mask = nla_get_u32(tb[i]);
> >> +			break;
> >> +		case IFLA_MASTER:
> >> +			master_idx = nla_get_u32(tb[i]);
> >> +			break;
> >> +		case IFLA_LINKINFO:
> >> +			kind_ops = linkinfo_to_kind_ops(tb[i]);
> >> +			break;
> >> +		default:
> >> +			if (cb->strict_check) {
> >> +				NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> >> +				return -EINVAL;
> >> +			}
> >> +		}
> > 
> > This might make sense to be split into two helpers for parsing:
> > <blablabla>_strict() and <blablabla>_lenient(). :)
> 
> I thought about that, but there is so much overlap - they are mostly
> common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg
> is the one that has been (ab)used for other message types, so strict
> versus lenient does not really have a differentiator for this message
> type - other than checking the elements of the struct.

It's mostly about the function being extremely long and convoluted.
Having parts moved out into (a) descriptive helper(s) with whatever name
might make this way more readable than it is now especially with the new
handling we need for strict checking.