netdev - Re: [PATCH net-next v11 1/4] net netlink: Add new type NLA_BITFIELD

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:   Fri, 28 Jul 2017 08:19:06 -0600
From:   David Ahern <dsahern@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, jiri@...nulli.us, xiyou.wangcong@...il.com,
        eric.dumazet@...il.com, mrv@...atatu.com,
        simon.horman@...ronome.com, alex.aring@...il.com
Subject: Re: [PATCH net-next v11 1/4] net netlink: Add new type
 NLA_BITFIELD_32

On 7/28/17 7:51 AM, Jamal Hadi Salim wrote:
> On 17-07-25 10:41 AM, David Ahern wrote:
>> On 7/23/17 7:35 PM, Jamal Hadi Salim wrote:
>>> In the most basic form, the user specifies the attribute policy as:
>>> [ATTR_GOO] = { .type = NLA_BITFIELD_32, .validation_data =
>>> &myvalidflags },
>>>
>>> where myvalidflags is the bit mask of the flags the kernel understands.
>>>
>>> If the user _does not_ provide myvalidflags then the attribute will
>>> also be rejected.
>>
>> No other netlink attribute has this requirement. 
> 
> This is the first one where we have to inspect content. We add things
> when we need them - as in this case.

Sure, the validation is required. My argument is that the validation
should be done where other attributes are validated -- inline with its
use. Nothing about this new bitfield says it must have a generic
validation code.

> 
>> Users of the attributes
>> are the only ones that know if a value is valid or not (e.g, attribute
>> passing a device index) and those are always checked in line.
> 
> It doesnt make sense that every user of the API has to repeat that
> validation code. Same principle as someone specifying that a type is
> u32 and have the nla validation check it. At some point we never had
> the u32 validation code. Then it was factored out because everyone
> repeats the same boilerplate code.

Every user of an attribute that uses a device index must verify the
device index is valid. The same code is repeated over and over.

Now you are suggesting to have 1 attribute whose content is validated by
generic infra and the rest are validated inline by the code using it. I
believe it is wrong and going to lead to problems.