Dear Maintainer,

I just came across a "stack smashing detected" crash when changing a gre6 to a gre4 tunnel
To reproduce create an ipv6 gre tunnel:
And then attempt to change it to an ipv4 one:
This results in:

The inverse (changing v4 to v6) results in:

Which I'm not sure if I should expect or if that's another issue, but it does not crash.

I've reproduced the crash on a few other bullseye servers/vms to rule out a single broken install.

I have also tested this on testing (iproute2 version 6.1.0-2) and the crash also happens there

I hope this is the right place and helpful enough to action on
Thanks!

Dear Maintainer,
I tried to find out where exactly the stack smashing takes place.
And found the ioctl SIOCCHGTUNNEL did write more than the 52 bytes
allocated in variable old_p, by that overwriting the stack canary.

Kind regards,
Bernhard


(gdb)
0x000055555557589f      62      {
1: x/i $pc
=> 0x55555557589f <parse_args+31>:      mov    %fs:0x28,%rax
(gdb)
0x00005555555758a8      62      {
1: x/i $pc
=> 0x5555555758a8 <parse_args+40>:      mov    %rax,0x68(%rsp)
(gdb) print/x $rax
$1 = 0xbf9b77d893accd00
(gdb) print/x $rsp + 0x68
$2 = 0x7fffffffea28


(gdb)
0x00007ffff7e575f5      120     in ../sysdeps/unix/syscall-template.S
1: x/i $pc
=> 0x7ffff7e575f5 <ioctl+5>:    syscall
2: /x *(uint64_t*)0x7fffffffea28 = 0xbf9b77d893accd00
(gdb) bt
#0  0x00007ffff7e575f5 in ioctl () at ../sysdeps/unix/syscall-template.S:120
#1  0x0000555555578230 in tnl_get_ioctl (basedev=0x7fffffffee8f "gre1", p=<optimized out>) at tunnel.c:77
#2  0x0000555555576243 in parse_args (argc=9, argv=0x7fffffffec50, cmd=35315, p=0x7fffffffea70) at iptunnel.c:181
#3  0x00005555555762fb in do_add (cmd=35315, argc=<optimized out>, argv=<optimized out>) at iptunnel.c:260
#4  0x000055555556258b in do_cmd (argv0=0x7fffffffee81 "tunnel", argc=11, argv=0x7fffffffec40) at ip.c:133
#5  0x0000555555561fc2 in main (argc=12, argv=0x7fffffffec38) at ip.c:344
(gdb) stepi
0x00007ffff7e575f7      120     in ../sysdeps/unix/syscall-template.S
1: x/i $pc
=> 0x7ffff7e575f7 <ioctl+7>:    cmp    $0xfffffffffffff001,%rax
2: /x *(uint64_t*)0x7fffffffea28 = 0x200000000000000

(gdb) print &old_p
$7 = (struct ip_tunnel_parm *) 0x7fffffffe9f0
(gdb) print sizeof(old_p)
$8 = 52
(gdb) print/x 0x7fffffffe9f0 + 52
$9 = 0x7fffffffea24

(gdb) list iptunnel.c:181
178                             if (cmd == SIOCCHGTUNNEL && count == 0) {
179                                     struct ip_tunnel_parm old_p = {};
180
181                                     if (tnl_get_ioctl(*argv, &old_p))
182                                             return -1;

Hi David and Stephen,

To reproduce, build iproute2 with -fstack-protector-strong in CFLAGS, and run:

ip tunnel add gre1 mode ip6gre local 2001:db8::1 remote 2001:db8::2 ttl 255
ip tunnel change gre1 mode gre local 192.168.0.0 remote 192.168.0.1 ttl 255

You'll get:

*** stack smashing detected ***: terminated
Aborted

This happens because iproute2 just assumes the tunnel is ipv4, but the
kernel "knows" it's actually ip6gre so when calling the SIOCGETTUNNEL
ioctl it writes back a struct ip6_tnl_parm2 into the struct
ip_tunnel_parm which is smaller, so the stack gets overwritten. Is
there any way to tell from userspace whether a gre is v4 or v6 before
doing an ioctl? The ioctls don't take/return a size parameter as far
as I can see...

On Sun, 2 Apr 2023 23:58:52 +0100 Luca Boccassi <bluca@debian.org> wrote:

Is setting IPv4 addresses on an IPv6 tunnel even a valid operation?
Assuming the ioctl to get is fixed, is there a reason to allow it?

One more reason netlink is better than ioctl.

On Mon, 3 Apr 2023 at 15:56, Stephen Hemminger <stephen@networkplumber.org> wrote:

I don't know, I don't really know anything about GRE, but if it isn't,
it should be rejected earlier, rather than overwriting the stack,
which seems dangerous.

ted

Ip uses and IPv4 UDP socket when it thinks it is talking to GRE.
And a IPv6 UDP socket when it is talking to GRE6.

So the kernel could check and error out?

Does seem like a kernel bug and a well known design flaw in ioctl
interface (assuming buffer of a specific size). The best iproute2 can do
is have `old_p` be a larger size (e.g., ip6_tnl_parm2) to avoid the
overrun, but then the result is nonsense with no way for it no an ipv6
struct was passed back. The crash at least indicates something is off.

On Mon, 3 Apr 2023 20:47:01 -0600 David Ahern <dsahern@kernel.org> wrote:

Actually any change tunnel can have similar issues where the tunnel
is of one type and the request wants to change parameters.
The two structs (ip_param and ip6_tunnel_param) are different enough
that getting the incorrect type will be complete garbage.

There doesn't seem to be a good way to identify the tunnel type.
The only way I can see is to look at the link type (ifi_type)
but this is ARPHRD_XXX value and not the ip protocol.

The other way would be to query link info (with netlink)
and make sure that IFLA_INFO_KIND (in IFLA_LINKINFO) matches when
changing.

Or maybe get rid of the ip tunnel command and just use ip link
which is all netlink based.  The iptunnel stuff was introduced long ago
when the only way to make tunnels was with ioctl.  Now you can do
same operations with ip link.

to

On Mon, 3 Apr 2023 20:47:01 -0600 David Ahern <dsahern@kernel.org> wrote:

I started to look into redoing the whole 'ip tunnel XXX' as just a remapping
of arguments and calling the equivalent 'ip link ... type YYY' and it is doable
for the basic stuff.

Then starting looking at the Potential Router List (PRL) stuff.
Looks like this is only supported through ioctl().
Definitely a dusty dark corner of networking code with rarely used features.

Plus things like, the code to get PRL will allow bigger get if called
from root vs non-root user??