netdev - Re: [PATCH iproute2 2/2] ip tunnel: warn when changing IPv6 tunnel without tunnel name

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:   Wed, 10 Jul 2019 10:33:01 +0200
From:   Andrea Claudi <aclaudi@...hat.com>
To:     Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
Cc:     linux-netdev <netdev@...r.kernel.org>,
        Stephen Hemminger <stephen@...workplumber.org>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH iproute2 2/2] ip tunnel: warn when changing IPv6 tunnel
 without tunnel name

On Wed, Jul 10, 2019 at 12:15 AM Mahesh Bandewar (महेश बंडेवार)
<maheshb@...gle.com> wrote:
>
> On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@...hat.com> wrote:
> >
> > Tunnel change fails if a tunnel name is not specified while using
> > 'ip -6 tunnel change'. However, no warning message is printed and
> > no error code is returned.
> >
> > $ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0
> > $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2
> > $ ip -6 tunnel show ip6tnl1
> > ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
> >
> > This commit checks if tunnel interface name is equal to an empty
> > string: in this case, it prints a warning message to the user.
> > It intentionally avoids to return an error to not break existing
> > script setup.
> >
> > This is the output after this commit:
> > $ ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote fd::2 tos inherit ttl 127 encaplimit none dev dummy0
> > $ ip -6 tunnel change dev dummy0 local 2001:1234::1 remote 2001:1234::2
> > Tunnel interface name not specified
> > $ ip -6 tunnel show ip6tnl1
> > ip6tnl1: gre/ipv6 remote fd::2 local fd::1 dev dummy0 encaplimit none hoplimit 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
> >
> > Reviewed-by: Matteo Croce <mcroce@...hat.com>
> > Signed-off-by: Andrea Claudi <aclaudi@...hat.com>
>
> I tried your patch and the commands that I posted in my (previous) patch.
>

Hi Mahesh,
Thank you for taking the time to review my patch.

> Here is the output after reverting my patch and applying your patch
>
> <show command>
> ------------------------
> vm0:/tmp# ./ip -6 tunnel add ip6tnl1 mode ip6gre local fd::1 remote
> fd::2 tos inherit ttl 127 encaplimit none
> vm0:/tmp# ./ip -6 tunnel show dev ip6tnl1
> vm0:/tmp# echo $?
> 0
>
> here the output is NULL and return code is 0. This is wrong and I
> would expect to see the tunnel info (as displayed in 'ip -6 tunnel
> show ip6tnl1')

It seems to me there is a bit of misunderstanding here. Looking at man
page for ip tunnel:

dev NAME
       bind the tunnel to the device NAME so that tunneled packets
will only be routed via this device and will not be able to escape to
another device when the route to endpoint changes.

>From what I read, dev parameter should not be used as an alias to the
tunnel device, but to indicate the device to which the tunnel should
be binded.
As such, ip -6 tunnel show dev <name> is a legitimate query that must
show the tunnel device(s) binded to <name> interface.
With the query ip -6 tunnel show <name> you instead obtain the tunnel itself.

By the way, the proper selector for the tunnel device name is the
default "name" parameter. From the man page:

name NAME (default)
       select the tunnel device name.

For example:
$ ip -6 tunnel show name ip6tnl0
ip6tnl0: ipv6/ipv6 remote :: local :: encaplimit 0 hoplimit inherit
tclass 0x00 flowlabel 0x00000 (flowinfo 0x00000000)

> <change command>
> lpaa10:/tmp# ip -6 tunnel change dev ip6tnl1 local 2001:1234::1 remote
> 2001:1234::2 encaplimit none ttl 127 tos inherit allow-localremote
> lpaa10:/tmp# echo $?
> 0
> lpaa10:/tmp# ip -6 tunnel show dev ip6tnl1
> lpaa10:/tmp# ip -6 tunnel show ip6tnl1
> ip6tnl1: gre/ipv6 remote fd::2 local fd::1 encaplimit none hoplimit
> 127 tclass inherit flowlabel 0x00000 (flowinfo 0x00000000)
>
> the change command appeared to be successful but change wasn't applied
> (expecting the allow-localremote to be present on the tunnel).

As you can read from the commit message, here I am not changing the
return code intentionally to not break existing script setup.
However, I can easily change this to return an error code in a v2.

> ---
> >  ip/ip6tunnel.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> > index 999408ed801b1..e3da11eb4518e 100644
> > --- a/ip/ip6tunnel.c
> > +++ b/ip/ip6tunnel.c
> > @@ -386,6 +386,9 @@ static int do_add(int cmd, int argc, char **argv)
> >         if (parse_args(argc, argv, cmd, &p) < 0)
> >                 return -1;
> >
> > +       if (!*p.name)
> > +               fprintf(stderr, "Tunnel interface name not specified\n");
> > +
> >         if (p.proto == IPPROTO_GRE)
> >                 basedev = "ip6gre0";
> >         else if (p.i_flags & VTI_ISVTI)
> > --
> > 2.20.1
> >

Regards,
Andrea