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 11:04:24 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     David Ahern <dsahern@...il.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 17-07-28 10:19 AM, David Ahern wrote:
> 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.
>
Response below.

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

Kernel side checking for device ifindex must know what a device
ifindex means.
That doesnt disqualify that the generic code checks that it
is of the same size as a signed 32b, etc. That is generic
stuff that can be factored out.

In this case:
Checking for whether bits selected are in the allowed range
that the kernel understands, that the bit value are set in
the right bit position, that the bits set in the correct bit
value position are also selected in the transaction.
That is generic code (which the content validation does).

Checking what bit 5 means - that is specific per kernel user
code. Checking that when bit 5 is selected it is only useful
if bit 3 is unselected - that is specific to the kernel code
consuming those bits.

cheers,
jamal