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]

Date:   Sun, 23 Oct 2022 18:19:51 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
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, 2022-10-21 at 21:08 -0700, Jakub Kicinski wrote:
> 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"?

Fair enough.

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

No I guess it's fine, it just felt a lot of overhead for what could've
been a one-line check. Having it introspectable is a nice benefit I
didn't think about :)

> > 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?

Yes. In fact there's also the strict_start_type in the policy[0] entry
too, which was kind of similar.

johannes