netdev - Re: [PATCH net-next iproute2 1/3] netlink: import netlink message parsing from kernel

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: <91687d1a-0187-9e30-911b-f752af1c3be6@cumulusnetworks.com>
Date:   Tue, 2 May 2017 14:39:40 -0600
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, jakub.kicinski@...ronome.com
Subject: Re: [PATCH net-next iproute2 1/3] netlink: import netlink message
 parsing from kernel

On 5/2/17 1:49 PM, Stephen Hemminger wrote:
> I am not disagreeing that iproute2 should handle the extended
> error format. Just want the solution to be as small as possible;
> ie do no more than is absolutely necessary. And future proof
> for the inevitable growth in new area.

Understood. I was trying to not reinvent a wheel. nla_parse and
nla_validate have not really been touched in 10 years, and both are well
written with a good API to the rest of the code base.

>From there, I grabbed whole snippets as opposed to just taking what is
needed for the ext-ack feature. I left the name as nlattr.c for easy
diff if future code is wanted. The header file was renamed to nlattr.h
to avoid confusion with other netlink.h files. Not adding it to
libnetlink.h facilitates pulling more code via diff as needed.

In short, I think it is good to re-use code from the kernel (lib and
tools/lib) where possible and doing so in a way that makes updates as
easy as header files.

> 
>> +
>> +static const __u8 nla_attr_minlen[NLA_TYPE_MAX+1] = {
>> +	[NLA_U8]	= sizeof(__u8),
>> +	[NLA_U16]	= sizeof(__u16),
>> +	[NLA_U32]	= sizeof(__u32),
>> +	[NLA_U64]	= sizeof(__u64),
>> +	[NLA_MSECS]	= sizeof(__u64),
>> +	[NLA_NESTED]	= NLA_HDRLEN,
>> +	[NLA_S8]	= sizeof(__s8),
>> +	[NLA_S16]	= sizeof(__s16),
>> +	[NLA_S32]	= sizeof(__s32),
>> +	[NLA_S64]	= sizeof(__s64),
>> +};
>> +
> 
> This patch makes iproute2 now doing validation of netlink attributes
> coming back from the kernel. What is the point, userspace should be
> trusting the kernel.

The kernel has bugs too; userspace should verify what it sends. In this
case the policy validation is just data types -- a string was expected
and a string was returned, or an attribute should be a u32 and it is.
You can argue it is overkill for iproute2, but it is good coding practice.

And for many netlink based features iproute2 tends to be the model that
is copied into other code bases.