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: <cb8113b9-ad0a-ad75-0784-19b8e0bc3fb2@intel.com>
Date:   Mon, 24 Oct 2022 03:00:39 -0700
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Johannes Berg <johannes@...solutions.net>,
        Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <edumazet@...gle.com>, <pabeni@...hat.com>, <fw@...len.de>,
        <jiri@...dia.com>
Subject: Re: [PATCH net] genetlink: piggy back on resv_op to default to a
 reject policy



On 10/23/2022 9:19 AM, Johannes Berg wrote:
> 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 :)
> 

I agree with reporting and making it possible to introspect. Thanks!

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

I agree here. Non-strict commands are very difficult if not impossible 
to extend or modify. Making it difficult to add new ones (whether by 
accident or not) is good in my book.

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