netdev - Re: [PATCH] rtnetlink: Mask the rta

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:	Thu, 14 Mar 2013 13:40:05 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Stephen Hemminger <stephen@...workplumber.org>
CC:	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH] rtnetlink: Mask the rta_type when range checking

On 03/13/2013 11:36 AM, Stephen Hemminger wrote:
> On Wed, 13 Mar 2013 10:18:58 -0400
> Vlad Yasevich <vyasevic@...hat.com> wrote:
>
>> Range/validity checks on rta_type in rtnetlink_rcv_msg() do
>> not account for flags that may be set.  This causes the function
>> to return -EINVAL when flags are set on the type (for example
>> NLA_F_NESTED).
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@...hat.com>
>> ---
>>   net/core/rtnetlink.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 1868625..dc5edf1 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2538,7 +2538,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>>   		struct rtattr *attr = (void *)nlh + NLMSG_ALIGN(min_len);
>>
>>   		while (RTA_OK(attr, attrlen)) {
>> -			unsigned int flavor = attr->rta_type;
>> +			unsigned int flavor = attr->rta_type & NLA_TYPE_MASK;
>>   			if (flavor) {
>>   				if (flavor > rta_max[sz_idx])
>>   					return -EINVAL;
>
> No. This is effectively an ABI change. It adds nothing.
>
> The NLA_F_NESTED attribute wasn't in the first generation version of netlink
> (before my time with Linux). It doesn't make sense to all of sudden start
> accepting it on requests. Also, then you would expect the query to set
> the NESTED flag as well, and that would be another ABI change.
>

So let me rebuff this a bit more intelligently.

1) NLA_F_NESTED is used by netfilter in a lot of places.  It seems that
only rtnetlink interface doesn't account for it.

2) The following commit:

commit 25c71c75ac87508528db053b818944f3650dd7a6
Author: stephen hemminger <shemminger@...tta.com>
Date:   Tue Nov 13 07:53:05 2012 +0000

     bridge: bridge port parameters over netlink

introduced NLA_F_NESTED usage in rtnetlink for both setlink and
getlink, so one could argue that was an ABI change.  However, I can
prove that without the change I am introducing, one can not use the 
above mentioned bridge API.  Feel free to try it with iproute2 patches
I sent earlier ([PATCH iproute2 0/2] Add support for bridge port link 
information).

Now, if we truly want to say that we may not introduce NLA_F_NESTED into
the rtnetlink patch, then the above commit to bridge needs to be
reworked.

Do you still object to this patch?  If so, we would have to rework the
bridge netlink api.  Otherwise, it enables the current API to actually work.

Thanks
-vlad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html