netdev - Re: [PATCH iproute2-next] ipaddress: fix label matching

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: <0b8902b1-fad1-55ef-acfc-088edb4569c6@gmail.com>
Date:   Sat, 14 Jul 2018 21:54:07 +0300
From:   Serhey Popovych <serhe.popovych@...il.com>
To:     Vincent Bernat <vincent@...nat.im>, David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org
Subject: Re: [PATCH iproute2-next] ipaddress: fix label matching

Serhey Popovych wrote:
> Vincent Bernat wrote:
>>  ❦ 11 juillet 2018 21:01 -0400, David Ahern <dsahern@...il.com> :
>>
>>>> +++ b/ip/ipaddress.c
>>>> @@ -837,11 +837,6 @@ int print_linkinfo(const struct sockaddr_nl *who,
>>>>  	if (!name)
>>>>  		return -1;
>>>>  
>>>> -	if (filter.label &&
>>>> -	    (!filter.family || filter.family == AF_PACKET) &&
>>>> -	    fnmatch(filter.label, name, 0))
>>>> -		return -1;
>>>> -
>>>
>>> The offending commit changed the return code:
>>>
>>>         if (filter.label &&
>>>             (!filter.family || filter.family == AF_PACKET) &&
>>> -           fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
>>> -               return 0;
>>> +           fnmatch(filter.label, name, 0))
>>> +               return -1;
>>>
>>>
>>> Vincent: can you try leaving the code as is, but change the return to 0?
>>
>> Yes, it works by just returning 0. The code still doesn't make sense.
>>
> 
> I think return code is correct. Check presented by this code too because
> print_linkinfo() isn't static and called from ipmonitor.c where no
> ipaddr_filter() or similar call that filters by label present.

Ok, did more deep analysis of code. Vincent, David: we should return 0
as done before 9516823051ce.

This is special case to return from print_linkinfo() earlier and match
only filter.ifindex and filter.up if given and not rest fields in @filter.

Then call print_selected_addrinfo() without calling print_link_stats()
in ipaddr_list_flush_or_save().

Later print_selected_addrinfo() calls print_addrinfo() that finally
matches filter.label using ifa_label_match_rta().

> 
> Instead fnmatch() compares interface *name*, not label from IFA_LABEL
> attribute. Thus:
> 
>     fnmatch(pattern, string, flags) ->
>     fnmatch("lo:1", "lo", 0) == FNM_NOMATCH (1)

This still incorrect: we should not call fnmatch() with network device name.

Also ip-link(8) does not say anything that label could be used to
filter link output. Label is ip-address(8) specific. Therefore checking
filter.family == AF_PACKET looks incorrect. AF_PACKET is ip-link(8)
specific.

Checking against !filter.family (AF_UNSPEC) is incorrect too: user might
force address family at ip command line and we never get:

    ip -4 addr show label lo:1

So from the code:

    if (filter.label &&
       (!filter.family || filter.family == AF_PACKET) &&
        fnmatch(filter.label, RTA_DATA(tb[IFLA_IFNAME]), 0))
            return -1;

We should leave only filter.label check and return 0:

    if (filter.label)
            return 0;

This will ensure we exit from print_linkinfo() earlier, skip
print_link_stats() and push final filtering by label to
print_selected_addrinfo() and print_addrinfo().

And finally: this is regression and should be against iproute2, not -next.

> 
> Assuming above I would like to see ifa_label_match_rta() instead of open
> coded checks for filter.label with fmatch() in print_linkinfo().
> 
> Also it might be good idea to pass @name from get_ifname_rta() (like we
> do in print_linkinfo()) to ifa_label_match_rta() so that we respect
> IFLA_IFNAME if present.
> 

Download attachment "signature.asc" of type "application/pgp-signature" (491 bytes)