Bug: `tsi_connect` commits to `S_INET` too early with `SOCK_DGRAM`, causing routed but unreachable UDP packets to be emitted in guest network

[chaserhkj]

The current implementation of tsi_connect roughly goes like (ref)

static int tsi_connect(struct socket *sock, struct sockaddr *addr, int addr_len, int flags)
{
    // ...
    if (isocket) {
        err = isocket->ops->connect(isocket, addr, addr_len,
                                    flags & ~O_NONBLOCK);
        if (err == 0 || err == -EALREADY) {
            tsk->status = S_INET;
            goto release;
        } else if (err == -EINPROGRESS) {
            tsk->status = S_INET;
            goto release;
        }
    }
}

However this creates a problem with SOCK_DGRAM, upon connect with a DGRAM socket, isocket only checks if the target destination IP is routed but does not check if it is actually reachable (contrary to SOCK_STREAM where TCP handshake is performed thus confirming reachability).

This creates a problem for an edge case where if the guest configured its routing table with an IP that is reachable for the host, the guest kernel will always try to send UDP packets to that IP on the guest side.

Particularly with libkrun setting a dummy interface with IP 10.0.0.1/8 in past versions, and with k8s and podman using default pod address from this same range, this can make krun pods in these environment not able to route to other pods, and potentially problems with DNS as well.

Although this was fixed in this commit by replacing the dummy IP range with a less-used one. IMO This is still just a workaround not a proper fix. Under exotic network setups and edge cases, this might still trigger with overlapping network blocks.

I think the proper fix is to make DGRAM connects stay in S_HYBRID, copy the address to tsk->sendto_addr and defer the personality decision to sendmsg time, like this:

static int tsi_connect(struct socket *sock, struct sockaddr *addr, int addr_len, int flags)
{
    // ...
    if (isocket) {
        if (sock->type == SOCK_DGRAM) {
            // Store the address for later use in sendmsg
            if (!tsk->sendto_addr) {
                tsk->sendto_addr = kmalloc(addr_len, GFP_KERNEL);
                tsk->sendto_addr_len = addr_len;
            }
            memcpy(tsk->sendto_addr, addr, addr_len);
            goto release;  // stay in current state (S_HYBRID)
        }
        err = isocket->ops->connect(isocket, addr, addr_len,
                                    flags & ~O_NONBLOCK);
        // normal commit to S_INET for SOCK_STREAM ...
    }
}

I went for an issue because I am not sure how to contribute to the code in this repo since they are just a patchset and directly modifying a patch feels wrong... If there is a kernel source tree branch or mailing list you can point me to I would be more than happy to submit a patch for this there.

[chaserhkj]

Ok I just realized that my proposed fix has a problem - when an app is making connect + sendmsg call from the guest, the connect will never be called from the host side, and making the subsequent sendmsg calls not going through. I think the better way to do this is to do tsi_has_udp_listener from tsi_connect for SOCK_DGRAM and make a decision about using vsock personality based upon that. This mirrors the behavior from sendmsg and makes sure the host make a mirrored connect call as well such that all behaviors are mirrored.

[slp]

Also, isocket->ops->connect() is able to confirm there's a path to reach the destination, returning an error otherwise.

It might not be ideal, but TEST-NET-3 IP address are reserved by IANA and not routable. Trying to use them would led to issues way bigger than its incompatibility with TSI, so I think we're pretty safe here.

[chaserhkj]

Also, isocket->ops->connect() is able to confirm there's a path to reach the destination, returning an error otherwise.

This is true for TCP connections, but for UDP, connect will succeed as long as the destination ip matches any prefix in the guest routing table, even if the address is actually unreachable. UDP sockets will defer returning ENETUNREACH to sendmsg calls.

It might not be ideal, but TEST-NET-3 IP address are reserved by IANA and not routable. Trying to use them would led to issues way bigger than its incompatibility with TSI, so I think we're pretty safe here.

I agree that TEST-NET-3 IP are very safe for their use here. But the problem is that the guest network stack is easily configurable by any guest privileged processes (The default configuration just runs any container primary process with full guest privileges). So other IP address blocks could be potentially configured later and alter the behavior of TSI.

While I could not come up with an immediate useful example or potential exploit through setting IP addresses this way, I think the key argument here is that the UDP behavior is different than the TCP behavior, which may be a very unexpected outcome for anyone who sets these IP addresses for whatever reason.