initialize_tun() adding neighbors fix proposal

[Alon Zahavi]

Hi,

During fuzzing I saw that the process of adding neighbors to the `syz_tun` interface is broken, thus breaking the fuzzing of the network stack from an external network.

After a bit of debugging, I saw that it happens because the interface should be activated (`ip link set <dev> up`) before trying to add new neighbors.

My proposed solution for this is to just move up `netlink_device_change()` in the calling order, so it will be called before `netlink_add_neigh()`.

Before state (Trimmed for simplicity)

```

static void initialize_tun(void)
{
    ...

    tunfd = open("/dev/net/tun", O_RDWR | O_NONBLOCK);
    if (tunfd == -1) {
        fail("tun: can't open /dev/net/tun");
    }

    ...

    // Setting up TUN device as a network interface
    if (ioctl(tunfd, TUNSETIFF, (void*)&ifr) < 0) {
            fail("tun: ioctl(TUNSETIFF) failed");
    }

    ...

    // Adding IP addresses
    netlink_add_addr4(&nlmsg, sock, TUN_IFACE, LOCAL_IPV4);
    netlink_add_addr6(&nlmsg, sock, TUN_IFACE, LOCAL_IPV6);

    // Adding IPv4 Neighbour
    uint64 macaddr = REMOTE_MAC;
    struct in_addr in_addr;
    inet_pton(AF_INET, REMOTE_IPV4, &in_addr);
    netlink_add_neigh(&nlmsg, sock, TUN_IFACE, &in_addr, sizeof(in_addr), &macaddr, ETH_ALEN);
    
    // Adding IPv6 Neighbour
    struct in6_addr in6_addr;
    inet_pton(AF_INET6, REMOTE_IPV6, &in6_addr);
    netlink_add_neigh(&nlmsg, sock, TUN_IFACE, &in6_addr, sizeof(in6_addr), &macaddr, ETH_ALEN);
    
    macaddr = LOCAL_MAC;
    netlink_device_change(&nlmsg, sock, TUN_IFACE, true, 0, &macaddr, ETH_ALEN, NULL);
    close(sock);
}

```

After fix:

```

static void initialize_tun(void)
{
    ...

    tunfd = open("/dev/net/tun", O_RDWR | O_NONBLOCK);
    if (tunfd == -1) {
        fail("tun: can't open /dev/net/tun");
    }

    ...

    // Setting up TUN device as a network interface
    if (ioctl(tunfd, TUNSETIFF, (void*)&ifr) < 0) {
            fail("tun: ioctl(TUNSETIFF) failed");
    }

    ...

    // Adding IP addresses
    netlink_add_addr4(&nlmsg, sock, TUN_IFACE, LOCAL_IPV4);
    netlink_add_addr6(&nlmsg, sock, TUN_IFACE, LOCAL_IPV6);

    uint64 macaddr = LOCAL_MAC;
    netlink_device_change(&nlmsg, sock, TUN_IFACE, true, 0, &macaddr, ETH_ALEN, NULL);

    // Adding IPv4 Neighbour
    macaddr = REMOTE_MAC;
    struct in_addr in_addr;
    inet_pton(AF_INET, REMOTE_IPV4, &in_addr);
    netlink_add_neigh(&nlmsg, sock, TUN_IFACE, &in_addr, sizeof(in_addr), &macaddr, ETH_ALEN);
    
    // Adding IPv6 Neighbour
    struct in6_addr in6_addr;
    inet_pton(AF_INET6, REMOTE_IPV6, &in6_addr);
    netlink_add_neigh(&nlmsg, sock, TUN_IFACE, &in6_addr, sizeof(in6_addr), &macaddr, ETH_ALEN);

close(sock);
}

```

Also, We need to change the macros of LOCAL_MAC and REMOTE_MAC to be in network byte-order, as the netlink_add_neigh is adding a reversed MAC address.

In case this will be accepted as a good solution I will send a proper patch.

Thank you,

Alon.

[Dmitry Vyukov]

netlink_add_neigh should print this message on failure:

https://github.com/google/syzkaller/blob/71b00cfbfc5c749ff257952d9bd9e7cdbc7c654b/executor/common_linux.h#LL605C13-L605C28

but running syz-execprog I don't see this message:

https://gist.github.com/dvyukov/5764544f75c631af82d75b8adba61fd0

What's up here?

[Alon Zahavi]

My suspicion is that when we add a neighbor before we put the interface up, the adding process is not failing but it doesn't work either, meaning it becomes redundant.

When I moved `netlink_device_change` (which puts up the syz_tun interface) to be executed before `netlink_add_neigh`, the neighbor was added correctly.

[Dmitry Vyukov]

On Tue, 16 May 2023 at 10:45, Alon Zahavi <zahav...@gmail.com> wrote:
>
> My suspicion is that when we add a neighbor before we put the interface up, the adding process is not failing but it doesn't work either, meaning it becomes redundant.
> When I moved `netlink_device_change` (which puts up the syz_tun interface) to be executed before `netlink_add_neigh`, the neighbor was added correctly.

Also tried this test:

https://github.com/google/syzkaller/blob/71b00cfbfc5c749ff257952d9bd9e7cdbc7c654b/sys/linux/test/udp2

it also succeeded:

#0 [836ms] -> socket$inet_udp(0x2, 0x2, 0x0)
#0 [837ms] <- socket$inet_udp=0x3
#0 [838ms] -> bind$inet(0x3, 0x20000040, 0x10)
#0 [839ms] <- bind$inet=0x0
#0 [839ms] -> syz_emit_ethernet(0x34, 0x20000080, 0x0)
aa aa aa aa aa aa 00 00 00 00 00 00 08 00 45 00
00 26 00 00 00 00 00 11 ba c8 00 00 00 00 00 00
00 00 00 00 4e 20 00 12 b1 aa 00 00 00 00 00 00
00 00 00 00
#0 [846ms] <- syz_emit_ethernet=0x34
#0 [846ms] -> recvfrom(0x3, 0x200000c0, 0xa, 0x0, 0x0, 0x0)
#0 [847ms] <- recvfrom=0xa


Can you write a test that currently fails? It would undoubtedly prove
that there is a bug now and that the change fixes it. It will also
help to keep it working in future (presumably tun setup worked at some
point, but silently broke).
To be fair I no linux admin, I have no idea in what order what network
device configuration commands should be issued.

[Alon Zahavi]

After giving it some more thought, there is no test we can currently preform that will fail correctly. This is because the netlink message with the "add neighbor" is being sent correctly, but just not working.

To check if it failed we should try getting the neighbor table (whether using netlink or just shell command), and check if the new neighbor is there or not.

[Dmitry Vyukov]

Hi Alon,

I meant a test like this one:

https://github.com/google/syzkaller/blob/71b00cfbfc5c749ff257952d9bd9e7cdbc7c654b/sys/linux/test/udp2

There must be some user-space test that's not working.

> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/0d8506f5-14e9-4166-9e7d-4b95ac1e1de5n%40googlegroups.com.

[Alon Zahavi]

Hi,

I couldn't find a failing test for this issue.

The current code will not fail if the neighbor was not added. If we  want it to fail correctly we should add a check at the end of `netlink_add_neigh` that validates the neighbor was indeed added. Only after we would do that we could check for a fail.

For example, the following test will not fail, but it won't work either. The third call should send a packet to a new neighbor (`@remote`), however, instead it sends arp traffic because the neighbor was not added. The tes twill not result in a fail but won't work the way it should:

```

syz_emit_ethernet(0x36, &(0x7f0000000000)={@local, @remote, @void, {{0x800, {{0x5, 0x4, 0x0, 0x0, 0x28, 0x0, 0x0, 0x40, 0x6, 0x0, @remote, @local}, {{0x3000, 0x2001, 0x41424344, 0x0, 0x0, 0x0, 0x5, 0x2, 0xffd7}}}}}}, 0x0)

syz_extract_tcp_res(&(0x7f0000003000)={<r0=>0x41424344, <r1=>0x41424344}, 0x0, 0x1)

syz_emit_ethernet(0x36, &(0x7f0000000000)={@local, @remote, @void, {{0x800, {{0x5, 0x4, 0x0, 0x0, 0x28, 0x0, 0x0, 0x40, 0x6, 0x0, @remote, @local}, {{0x3000, 0x2001, r0, r1, 0x0, 0x0, 0x5, 0x10, 0x200}}}}}}, 0x0)  

```

[Dmitry Vyukov]

On Tue, 11 Jul 2023 at 11:13, Alon Zahavi <zahav...@gmail.com> wrote:
>
> Hi,
>

> I couldn't find a failing test for this issue.
> The current code will not fail if the neighbor was not added. If we want it to fail correctly we should add a check at the end of `netlink_add_neigh` that validates the neighbor was indeed added. Only after we would do that we could check for a fail.
>
> For example, the following test will not fail, but it won't work either. The third call should send a packet to a new neighbor (`@remote`), however, instead it sends arp traffic because the neighbor was not added. The tes twill not result in a fail but won't work the way it should:
> ```
> syz_emit_ethernet(0x36, &(0x7f0000000000)={@local, @remote, @void, {{0x800, {{0x5, 0x4, 0x0, 0x0, 0x28, 0x0, 0x0, 0x40, 0x6, 0x0, @remote, @local}, {{0x3000, 0x2001, 0x41424344, 0x0, 0x0, 0x0, 0x5, 0x2, 0xffd7}}}}}}, 0x0)
> syz_extract_tcp_res(&(0x7f0000003000)={<r0=>0x41424344, <r1=>0x41424344}, 0x0, 0x1)
> syz_emit_ethernet(0x36, &(0x7f0000000000)={@local, @remote, @void, {{0x800, {{0x5, 0x4, 0x0, 0x0, 0x28, 0x0, 0x0, 0x40, 0x6, 0x0, @remote, @local}, {{0x3000, 0x2001, r0, r1, 0x0, 0x0, 0x5, 0x10, 0x200}}}}}}, 0x0)
> ```

A test like this one does not just emit a packet, it also receives it
on a socket:
https://github.com/google/syzkaller/blob/71b00cfbfc5c749ff257952d9bd9e7cdbc7c654b/sys/linux/test/udp2
So if the packet is discarded, then we won't receive it.

> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/0a23d5a4-b235-43ac-9575-52b110a3b0f0n%40googlegroups.com.