netdev - Re: [PATCH net] genetlink: piggy back on resv

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: <20221021210815.44e8220f@kernel.org>
Date:   Fri, 21 Oct 2022 21:08:15 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, jacob.e.keller@...el.com, fw@...len.de,
        jiri@...dia.com
Subject: Re: [PATCH net] genetlink: piggy back on resv_op to default to a
 reject policy

On Fri, 21 Oct 2022 21:57:53 +0200 Johannes Berg wrote:
> It feels it might've been easier to implement as simply, apart from the
> doc changes:
> 
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -529,6 +529,10 @@ genl_family_rcv_msg_attrs_parse(const struct genl_family *family,
>  	struct nlattr **attrbuf;
>  	int err;
>  
> +	if (ops->cmd >= family->resv_start_op && !ops->maxattr &&
> +	    nlmsg_attrlen(nlh, hdrlen))
> +		return ERR_PTR(-EINVAL);
> +
>  	if (!ops->maxattr)
>  		return NULL;
>
> But maybe I'm missing something in the relation with the new split ops
> etc.

The reason was that payload length check is... "unintrospectable"?
The reject all policy shows up in GETPOLICY. Dunno how much it matters
in practice but that was the motivation. LMK which way you prefer.

> Also, technically, you could now have an op that is >= resv_start_op,
> but sets one of GENL_DONT_VALIDATE{_DUMP,}_STRICT and then gets the old
> behaviour except that attributes 0 and 1 are rejected?
> 
> Any particular reason you chose this implementation here? I can
> understand having chosen it with the yaml things since then you can be
> sure you're not setting GENL_DONT_VALIDATE{_DUMP,}_STRICT and you don't
> have another choice anyway, but here?
> 
> Hmm.
> 
> Then again, maybe anyway we should make sure that
> GENL_DONT_VALIDATE{_DUMP,}_STRICT aren't set for ops >= resv_start_op?
> 
> 
> Anyway, for the intended use it works, and I guess it'd be a stupid
> family that makes sure to set this but then still uses non-strict
> validation, though I've seen people (try to) copy/paste non-strict
> validation into new ops ...

Hm, yeah, adding DONT*_STRICT for new commands would be pretty odd as
you say. Someone may copy & paste an existing command, tho, without
understanding what this flag does. 

I can add a check separately I reckon. It's more of a "no new command
should set this flag" thing rather than inherently related to the
reject-all policy, right?