[PATCH] rtnetlink: Mask the rta_type when range checking

[PATCH] rtnetlink: Mask the rta_type when range checking

[Vlad Yasevich]

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@redhat.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;
-- 
1.7.7.6

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Stephen Hemminger]

On Wed, 13 Mar 2013 10:18:58 -0400
Vlad Yasevich <vyasevic@redhat.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@redhat.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.

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Vlad Yasevich]

On 03/13/2013 11:36 AM, Stephen Hemminger wrote:
> On Wed, 13 Mar 2013 10:18:58 -0400
> Vlad Yasevich <vyasevic@redhat.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@redhat.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.
>

It makes nested IFLA_PROTINFO work that the bridge code expects. 
Without this change, sending a nested IFLA_PROTINFO causes a EIVNAL
return.

-vlad

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

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Vlad Yasevich]

On 03/13/2013 11:36 AM, Stephen Hemminger wrote:
> On Wed, 13 Mar 2013 10:18:58 -0400
> Vlad Yasevich <vyasevic@redhat.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@redhat.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@vyatta.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

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Thomas Graf]

On 03/14/13 at 01:40pm, Vlad Yasevich wrote:
> 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.

Stephen is not wrong, strictly speaking this is an ABI breaker.

However, we have been applying NLA_TYPE_MASK in nla_type() and
thus in nla_parse(), nlmsg_find_attr(), etc. since the
introduction of the current kernel netlink API.

static inline int nla_type(const struct nlattr *nla)
{
        return nla->nla_type & NLA_TYPE_MASK;
}

I have been attempting to get rid of that ugly attribute parsing
code in rtnetlink_rcv_msg() but it's hard to get rid of. I shall
give this another try.

> 2) The following commit:
> 
> commit 25c71c75ac87508528db053b818944f3650dd7a6
> Author: stephen hemminger <shemminger@vyatta.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.

You are right, user space applications that do not apply
NLA_TYPE_MASK will no longer see the IFLA_PROTINFO attribute
with the above commit.

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

I am not against your patch but I would love to see the affected
code paths to no longer rely on the attribute array provided by
the rtnetlink doit function but instead call nla_parse() themselves
for the sake of proper validation.

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Vlad Yasevich]

On 03/14/2013 05:28 PM, Thomas Graf wrote:
> On 03/14/13 at 01:40pm, Vlad Yasevich wrote:
>> 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.
>
> Stephen is not wrong, strictly speaking this is an ABI breaker.
>
> However, we have been applying NLA_TYPE_MASK in nla_type() and
> thus in nla_parse(), nlmsg_find_attr(), etc. since the
> introduction of the current kernel netlink API.
>
> static inline int nla_type(const struct nlattr *nla)
> {
>          return nla->nla_type & NLA_TYPE_MASK;
> }
>
> I have been attempting to get rid of that ugly attribute parsing
> code in rtnetlink_rcv_msg() but it's hard to get rid of. I shall
> give this another try.
>
>> 2) The following commit:
>>
>> commit 25c71c75ac87508528db053b818944f3650dd7a6
>> Author: stephen hemminger <shemminger@vyatta.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.
>
> You are right, user space applications that do not apply
> NLA_TYPE_MASK will no longer see the IFLA_PROTINFO attribute
> with the above commit.
>
>> 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).
>
> I am not against your patch but I would love to see the affected
> code paths to no longer rely on the attribute array provided by
> the rtnetlink doit function but instead call nla_parse() themselves
> for the sake of proper validation.
>

Doing a quick check on all the callers for rtnl_register and their 
handlers the following do not use nla_parse:
  1) dn_fib_rtm_newroute/delroute  - Don't seem to care about attribute
                                     types.
  2) dn_cache_getroute() - suspect use.  relies on rta_buf populated by
                           rtnetlink_rcv_msg

  3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop
                                   with nla_type(), so safe.

That's all that a quick look finds.  Out of all of them, looks like on 
dn_cache_getroute() would be broken.

-vlad

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[Thomas Graf]

On 03/14/13 at 08:30pm, Vlad Yasevich wrote:
> Doing a quick check on all the callers for rtnl_register and their
> handlers the following do not use nla_parse:
>  1) dn_fib_rtm_newroute/delroute  - Don't seem to care about attribute
>                                     types.
>  2) dn_cache_getroute() - suspect use.  relies on rta_buf populated by
>                           rtnetlink_rcv_msg
> 
>  3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop
>                                   with nla_type(), so safe.
> 
> That's all that a quick look finds.  Out of all of them, looks like
> on dn_cache_getroute() would be broken.

So checking values in rta_max[] which lists the maximum attribute
allowed for each message family range, all are limited to low
values so the NLA_F_NESTED bit is guaranteed to have been unused
up to now.

The risk that remains is that we would start accepting an attribute
which we previously didn't but we have that risk with every new
attribute that is added.

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCH] rtnetlink: Mask the rta_type when range checking

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Fri, 15 Mar 2013 08:51:24 +0000

> On 03/14/13 at 08:30pm, Vlad Yasevich wrote:
>> Doing a quick check on all the callers for rtnl_register and their
>> handlers the following do not use nla_parse:
>>  1) dn_fib_rtm_newroute/delroute  - Don't seem to care about attribute
>>                                     types.
>>  2) dn_cache_getroute() - suspect use.  relies on rta_buf populated by
>>                           rtnetlink_rcv_msg
>> 
>>  3) inet_rtm_newroute/delroute - rtm_to_fib_config() uses a custom loop
>>                                   with nla_type(), so safe.
>> 
>> That's all that a quick look finds.  Out of all of them, looks like
>> on dn_cache_getroute() would be broken.
> 
> So checking values in rta_max[] which lists the maximum attribute
> allowed for each message family range, all are limited to low
> values so the NLA_F_NESTED bit is guaranteed to have been unused
> up to now.
> 
> The risk that remains is that we would start accepting an attribute
> which we previously didn't but we have that risk with every new
> attribute that is added.
> 
> Acked-by: Thomas Graf <tgraf@suug.ch>

Applied and queued up for -stable.  And also agreed that abstracting the
rtnetlink attribute parsing out properly is the way to go.