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

Powered by blists - more mailing lists