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

[Vlad Yasevich <vyasevic@redhat.com>]

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