netdev - Re: [PATCH v2 net-next 02/15] neighbour: Move two validations from neigh_get() to neigh_valid_get

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: <20250715182236.317f232d@kernel.org>
Date: Tue, 15 Jul 2025 18:22:36 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Kuniyuki Iwashima <kuniyu@...gle.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, David Ahern
 <dsahern@...nel.org>, Simon Horman <horms@...nel.org>, Kuniyuki Iwashima
 <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 02/15] neighbour: Move two validations from
 neigh_get() to neigh_valid_get_req().

On Sat, 12 Jul 2025 20:34:11 +0000 Kuniyuki Iwashima wrote:
>  	err = nlmsg_parse_deprecated_strict(nlh, sizeof(struct ndmsg), tb,
>  					    NDA_MAX, nda_policy, extack);
>  	if (err < 0)
> @@ -2951,11 +2957,14 @@ static struct ndmsg *neigh_valid_get_req(const struct nlmsghdr *nlh,
>  	}
>  
>  	for (i = 0; i <= NDA_MAX; ++i) {
> -		if (!tb[i])
> -			continue;
> -
>  		switch (i) {
>  		case NDA_DST:
> +			if (!tb[i]) {
> +				NL_SET_ERR_MSG(extack, "Network address not specified");
> +				err = -EINVAL;
> +				goto err;
> +			}

Any idea why this attr validation is coded up so weirdly?

NDA_DST is 1, so we could make this whole thing:

const struct nla_policy nda_get_policy[] = {
	[NDA_DST]		= { .type = NLA_BINARY, .len = MAX_ADDR_LEN },
};

	err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb,
			  ARRAY_SIZE(nda_get_policy) - 1,
		  	  nda_get_policy, extack);

and then no looping over the attributes would be necessary.

I'd also be tempted to replace the extack string with
	NL_SET_ERR_ATTR_MISS(extack, NULL, NDA_DST);
but I'm biased towards YNL :)