netdev - Re: [PATCH net-next 9/9] genetlink: allow dumping command-specific policy

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: <dcaeaedaa179c558cab0d417277fea9ac29d8b79.camel@sipsolutions.net>
Date:   Thu, 01 Oct 2020 23:03:01 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, andrew@...n.ch, jiri@...nulli.us,
        mkubecek@...e.cz, dsahern@...nel.org, pablo@...filter.org
Subject: Re: [PATCH net-next 9/9] genetlink: allow dumping command-specific
 policy

On Thu, 2020-10-01 at 11:30 -0700, Jakub Kicinski wrote:
> 
> +++ b/net/netlink/genetlink.c
> @@ -1119,6 +1119,7 @@ static const struct nla_policy ctrl_policy_policy[] = {
>  	[CTRL_ATTR_FAMILY_ID]	= { .type = NLA_U16 },
>  	[CTRL_ATTR_FAMILY_NAME]	= { .type = NLA_NUL_STRING,
>  				    .len = GENL_NAMSIZ - 1 },
> +	[CTRL_ATTR_OP]		= { .type = NLA_U8 },

I slightly wonder if we shouldn't make this wider. There's no real
*benefit* to using a u8 since, due to padding, the attribute actually
has the same size anyway (up to U32), and we also still need to validate
it actually exists.

However, if we ever run out of command numbers in some family (nl80211
is almost half way :-) ) then I believe we still have some reserved
space in the genl header that we could use for extensions, but if then
we have to also go around and change a bunch of other interfaces like
this, it'll just be so much harder, and ... we'd probably just be
tempted to multiplex onto an "extension command" or something? Or maybe
then we should have a separate genl family at that point?

(Internal interfaces are much easier to change)

Dunno. Just a thought. We probably won't ever get there, it just sort of
rubs me the wrong way that we'd be restricting this in an "unfixable"
API for no real reason :)

> -	if (!rt->policy)
> +	if (tb[CTRL_ATTR_OP]) {
> +		err = genl_get_cmd(nla_get_u8(tb[CTRL_ATTR_OP]), rt, &op);

OK, so maybe if we make that wider we also have to make the argument to
genl_get_cmd() wider so we don't erroneously return op 1 if you ask for
257, but that's in an at least 32-bit register anyway, presumably?

johannes