aboutgitcodebugslistschat
path: root/tap.c
Commit message (Collapse)AuthorAgeFilesLines
* tap: Revert recently added checks in tap_handler_passt()Stefano Brivio2022-11-161-16/+4
| | | | | | | | | | | | | | | This reverts commit 198f87835dc4 ("tap: Return -EIO from tap_handler_passt() on inconsistent packet stream") and commit 510dace86ccf ("tap: Keep stream consistent if qemu length descriptor spans two recv() calls"). I can hit occasional failures in perf/passt_tcp tests where we seem to be getting excess data at the end of a recv(), and for some reason I couldn't figure out yet, if we just ignore it, subsequent recv() calls from qemu return correct data. If we close the connection, qemu can't talk to us anymore, of course. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* arp, tap, util: Don't use perror() after seccomp filter is installedStefano Brivio2022-11-161-3/+3
| | | | | | | | | | | | | | | If stderr is closed, after we fork to background, glibc's implementation of perror() will try to re-open it by calling dup(), upon which the seccomp filter causes the process to terminate, because dup() is not included in the list of allowed syscalls. Replace perror() calls that might happen after isolation_postfork(). We could probably replace all of them, but early ones need a bit more attention as we have to check whether log.c functions work in early stages. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Return -EIO from tap_handler_passt() on inconsistent packet streamStefano Brivio2022-11-101-2/+2
| | | | | | | | While it's important to fail in that case, it makes little sense to fail quietly: it's better to tell qemu explicitly that something went wrong and that we won't recover, by closing the socket. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Keep stream consistent if qemu length descriptor spans two recv() callsStefano Brivio2022-11-101-3/+15
| | | | | | | | | | | | | | | | | | | | | | | | | I got all paranoid after triggering a divide-by-zero general protection fault in passt with a qemu version without the virtio_net TX hang fix, while flooding UDP. I start thinking this was actually coming from some random changes I was playing with, but before reaching this conclusion I reviewed once more the relatively short path in tap_handler_passt() before we start using packet_*() functions, and found this. Never observed in practice, but artificially reproduced with changes in qemu's socket interface: if we don't receive from qemu a complete length descriptor in one recv() call, or if we receive a partial one at the end of one call, we currently disregard the rest, which would make the stream inconsistent. Nothing really bad happens, except that from that point on we would disregard all the packets we get until, if ever, we get the stream back in sync by chance. Force reading a complete packet length descriptor with a blocking recv(), if needed -- not just a complete packet later. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, qrap, README: Update notes and documentation for AF_UNIX support in qemuStefano Brivio2022-11-041-6/+4
| | | | | | | | We can't get rid of qrap quite yet, but at least we should start telling users it's not going to be needed anymore starting from qemu 7.2. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Use endian-safer typing in struct tap4_l4_tDavid Gibson2022-11-041-15/+16
| | | | | | | | | | | | We recently converted to using struct in_addr rather than bare in_addr_t or uint32_t to represent IPv4 addresses in network order. This makes it harder forget to apply the correct endian conversions. We omitted the IPv4 addresses stored in struct tap4_l4_t, however. Convert those as well. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Use typing to reduce chances of IPv4 endianness errorsDavid Gibson2022-11-041-9/+9
| | | | | | | | | | | | | | | | | | | We recently corrected some errors handling the endianness of IPv4 addresses. These are very easy errors to make since although we mostly store them in network endianness, we sometimes need to manipulate them in host endianness. To reduce the chances of making such mistakes again, change to always using a (struct in_addr) instead of a bare in_addr_t or uint32_t to store network endian addresses. This makes it harder to accidentally do arithmetic or comparisons on such addresses as if they were host endian. We introduce a number of IN4_IS_ADDR_*() helpers to make it easier to directly work with struct in_addr values. This has the additional benefit of making the IPv4 and IPv6 paths more visually similar. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Support for detection of existing sockets on ramfsStefano Brivio2022-11-041-1/+2
| | | | | | | | | | | | | | | | | On ramfs, connecting to a non-existent UNIX domain socket yields EACCESS, instead of ENOENT. This is visible if we use passt directly on rootfs (a ramfs instance) from an initramfs image. It's probably wrong for ramfs to return EACCES, but given the simplicity of the filesystem, I doubt we should try to fix it there at the possible cost of added complexity. Also, this whole beauty should go away once qrap-less usage is established, so just accept EACCES as indication that a conflicting socket does not, in fact, exist. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Trace received (outbound) ICMP packets in debug mode, tooStefano Brivio2022-10-271-0/+2
| | | | | | | | | | This only worked for ICMPv6: ICMP packets have no TCP-style header, so they are handled as a special case before packet sequences are formed, and the call to tap_packet_debug() was missing. Fixes: bb708111833e ("treewide: Packet abstraction with mandatory boundary checks") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* dhcp: Use tap_udp4_send() helper in dhcp()David Gibson2022-10-191-1/+0
| | | | | | | | | | The IPv4 specific dhcp() manually constructs L2 and IP headers to send its DHCP reply packet, unlike its IPv6 equivalent in dhcpv6.c which uses the tap_udp6_send() helper. Now that we've broaded the parameters to tap_udp4_send() we can use it in dhcp() to avoid some duplicated logic. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Split tap_ip4_send() into UDP and ICMP variantsDavid Gibson2022-10-191-18/+59
| | | | | | | | | | | | | | | | | | tap_ip4_send() has special case logic to compute the checksums for UDP and ICMP packets, which is a mild layering violation. By using a suitable helper we can split it into tap_udp4_send() and tap_icmp4_send() functions without greatly increasing the code size, this removing that layering violation. We make some small changes to the interface while there. In both cases we make the destination IPv4 address a parameter, which will be useful later. For the UDP variant we make it take just the UDP payload, and it will generate the UDP header. For the ICMP variant we pass in the ICMP header as before. The inconsistency is because that's what seems to be the more natural way to invoke the function in the callers in each case. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* ndp: Remove unneeded eh_source parameterDavid Gibson2022-10-191-1/+1
| | | | | | | | | | | | | | | | | ndp() takes a parameter giving the ethernet source address of the packet it is to respond to, which it uses to determine the destination address to send the reply packet to. This is not necessary, because the address will always be the guest's MAC address. Even if the guest has just changed MAC address, then either tap_handler_passt() or tap_handler_pasta() - which are the only call paths leading to ndp() will have updated c->mac_guest with the new value. So, remove the parameter, and just use c->mac_guest, making it more consistent with other paths where we construct packets to send inwards. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Split tap_ip6_send() into UDP and ICMP variantsDavid Gibson2022-10-191-20/+62
| | | | | | | | | | | | | | | | | | tap_ip6_send() has special case logic to compute the checksums for UDP and ICMP packets, which is a mild layering violation. By using a suitable helper we can split it into tap_udp6_send() and tap_icmp6_send() functions without greatly increasing the code size, this removing that layering violation. We make some small changes to the interface while there. In both cases we make the destination IPv6 address a parameter, which will be useful later. For the UDP variant we make it take just the UDP payload, and it will generate the UDP header. For the ICMP variant we pass in the ICMP header as before. The inconsistency is because that's what seems to be the more natural way to invoke the function in the callers in each case. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Split tap_ip_send() into IPv4 and IPv6 specific functionsDavid Gibson2022-10-191-83/+94
| | | | | | | | | | | | | | | | | | | The IPv4 and IPv6 paths in tap_ip_send() have very little in common, and it turns out that every caller (statically) knows if it is using IPv4 or IPv6. So split into separate tap_ip4_send() and tap_ip6_send() functions. Use a new tap_l2_hdr() function for the very small common part. While we're there, make some minor cleanups: - We were double writing some fields in the IPv6 header, so that it temporary matched the pseudo-header for checksum calculation. With recent checksum reworks, this isn't neccessary any more. - We don't use any IPv4 header options, so use some sizeof() constructs instead of some open coded values for header length. - The comment used to say that the flow label was for TCP over IPv6, but in fact the only thing we used it for was DHCPv6 over UDP traffic Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Remove unhelpeful vnet_pre optimization from tap_send()David Gibson2022-10-191-20/+9
| | | | | | | | | | | | | | | | Callers of tap_send() can optionally use a small optimization by adding extra space for the 4 byte length header used on the qemu socket interface. tap_ip_send() is currently the only user of this, but this is used only for "slow path" ICMP and DHCP packets, so there's not a lot of value to the optimization. Worse, having the two paths here complicates the interface and makes future cleanups difficult, so just remove it. I have some plans to bring back the optimization in a more general way in future, but for now it's just in the way. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Remove support for TCP packets from tap_ip_send()David Gibson2022-10-191-9/+2
| | | | | | | | | | | tap_ip_send() is never used for TCP packets, we're unlikely to use it for that in future, and the handling of TCP packets makes other cleanups unnecessarily awkward. Remove it. This is the only user of csum_tcp4(), so we can remove that as well. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add helpers for normal inbound packet destination addressesDavid Gibson2022-10-191-5/+28
| | | | | | | | | | tap_ip_send() doesn't take a destination address, because it's specifically for inbound packets, and the IP addresses of the guest/namespace are already known to us. Rather than open-coding this destination address logic, make helper functions for it which will enable some later cleanups. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add csum_ip4_header() helper to calculate IPv4 header checksumsDavid Gibson2022-10-191-2/+1
| | | | | | | | We calculate IPv4 header checksums in at least two places, in dhcp() and in tap_ip_send. Add a helper to handle this calculation in both places. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add csum_udp4() helper for calculating UDP over IPv4 checksumsDavid Gibson2022-10-191-1/+1
| | | | | | | | | | | | | At least two places in passt fill in UDP over IPv4 checksums, although since UDP checksums are optional with IPv4 that just amounts to storing a 0 (in tap_ip_send()) or leaving a 0 from an earlier initialization (in dhcp()). For consistency, add a helper for this "calculation". Just for the heck of it, add the option (compile time disabled for now) to calculate real UDP checksums. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add csum_udp6() helper for calculating UDP over IPv6 checksumsDavid Gibson2022-10-191-3/+2
| | | | | | | | | | | Add a helper for calculating UDP checksums when used over IPv6 For future flexibility, the new helper takes parameters for the fields in the IPv6 pseudo-header, so an IPv6 header or pseudo-header doesn't need to be explicitly constructed. It also allows the UDP header and payload to be in separate buffers, although we don't use this yet. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add csum_icmp4() helper for calculating ICMP checksumsDavid Gibson2022-10-191-3/+1
| | | | | | | | | | Although tap_ip_send() is currently the only place calculating ICMP checksums, create a helper function for symmetry with ICMPv6. For future flexibility it allows the ICMPv6 header and payload to be in separate buffers. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Add csum_icmp6() helper for calculating ICMPv6 checksumsDavid Gibson2022-10-191-4/+2
| | | | | | | | | | | | At least two places in passt calculate ICMPv6 checksums, ndp() and tap_ip_send(). Add a helper to handle this calculation in both places. For future flexibility, the new helper takes parameters for the fields in the IPv6 pseudo-header, so an IPv6 header or pseudo-header doesn't need to be explicitly constructed. It also allows the ICMPv6 header and payload to be in separate buffers, although we don't use this yet. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()Stefano Brivio2022-10-151-0/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Even if CAP_NET_BIND_SERVICE is granted, we'll lose the capability in the target user namespace as we isolate the process, which means we're unable to bind to low ports at that point. Bind inbound ports, and only those, before isolate_user(). Keep the handling of outbound ports (for pasta mode only) after the setup of the namespace, because that's where we'll bind them. To this end, initialise the netlink socket for the init namespace before isolate_user() as well, as we actually need to know the addresses of the upstream interface before binding ports, in case they're not explicitly passed by the user. As we now call nl_sock_init() twice, checking its return code from conf() twice looks a bit heavy: make it exit(), instead, as we can't do much if we don't have netlink sockets. While at it: - move the v4_only && v6_only options check just after the first option processing loop, as this is more strictly related to option parsing proper - update the man page, explaining that CAP_NET_BIND_SERVICE is *not* the preferred way to bind ports, because passt and pasta can be abused to allow other processes to make effective usage of it. Add a note about the recommended sysctl instead - simplify nl_sock_init_do() now that it's called once for each case Reported-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Don't check sequence counts when adding packets to poolStefano Brivio2022-10-151-6/+6
| | | | | | | | | | | This is a minor optimisation possibility I spotted while trying to debug a hang in tap4_handler(): if we run out of space for packet sequences, it's fine to add packets to an existing per-sequence pool. We should check the count of packet sequences only once we realise that we actually need a new packet sequence. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, tap: Add option to quit once the client closes the connectionStefano Brivio2022-10-151-0/+5
| | | | | | | | This is practical to avoid explicit lifecycle management in users, e.g. libvirtd, and is trivial to implement. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Move logging functions to a new file, log.cStefano Brivio2022-10-141-0/+1
| | | | | | | | Logging to file is going to add some further complexity that we don't want to squeeze into util.c. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* cppcheck: Reduce scope of some variablesDavid Gibson2022-09-291-2/+3
| | | | | | | Minor style improvement suggested by cppcheck. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Check return value of accept4() before calling getsockopt()Stefano Brivio2022-09-221-2/+4
| | | | | | | | | | | | | | | | Reported by Coverity (CWE-119): Negative value used as argument to a function expecting a positive value (for example, size of buffer or allocation) and harmless, because getsockopt() would return -EBADF if the socket is -1, so we wouldn't print anything. Check if accept4() returns a valid socket before calling getsockopt() on it. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* Make substructures for IPv4 and IPv6 specific context informationDavid Gibson2022-07-301-11/+11
| | | | | | | | | | | | The context structure contains a batch of fields specific to IPv4 and to IPv6 connectivity. Split those out into a sub-structure. This allows the conf_ip4() and conf_ip6() functions, which take the entire context but touch very little of it, to be given more specific parameters, making it clearer what it affects without stepping through the code. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
* Separate IPv4 and IPv6 configurationDavid Gibson2022-07-301-2/+2
| | | | | | | | | | | | | | | | | | | After recent changes, conf_ip() now has essentially entirely disjoint paths for IPv4 and IPv6 configuration. So, it's cleaner to split them out into different functions conf_ip4() and conf_ip6(). Splitting these out also lets us make the interface a bit nicer, having them return success or failure directly, rather than manipulating c->v4 and c->v6 to indicate success/failure of the two versions. Since these functions may also initialize the interface index for each protocol, it turns out we can then drop c->v4 and c->v6 entirely, replacing tests on those with tests on whether c->ifi4 or c->ifi6 is non-zero (since a 0 interface index is never valid). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Whitespace fixes] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Move passt mac_guest init to be more symmetric with pastaDavid Gibson2022-07-301-0/+6
| | | | | | | | | | | | | | | | | In pasta mode, the guest's MAC address is set up in pasta_ns_cobf() called from tap_sock_tun_init(). If we have a guest MAC configured with --ns-mac-addr, this will set the given MAC on the kernel tuntap device, or if we haven't configured one it will update our record of the guest MAC to the kernel assigned one from the device. For passt, we don't initially know the guest's MAC until we receive packets from it, so we have to initially use a broadcast address. This is - oddly - set up in an entirely different place, in conf_ip() conditional on the mode. Move it to the logically matching place for passt - tap_sock_unix_init(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Add informational messages for UNIX domain socket connectionsStefano Brivio2022-06-181-0/+10
| | | | | | | | | ...namely, as connections are discarded or accepted. This was quite useful to debug an issue with libvirtd failing to start qemu (because passt refused the new connection) as a previous qemu instance was still active. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, tap: False "Buffer not null terminated" positives, CWE-170Stefano Brivio2022-04-071-3/+3
| | | | | | | Those strings are actually guaranteed to be NULL-terminated. Reported by Coverity. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Argument cannot be negative, CWE-687Stefano Brivio2022-04-071-0/+5
| | | | | | Actually harmless. Reported by Coverity. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Resource leak, CWE-404Stefano Brivio2022-04-071-1/+4
| | | | | | Reported by Coverity. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Unchecked return value from library, CWE-252Stefano Brivio2022-04-071-7/+12
| | | | | | | All instances were harmless, but it might be useful to have some debug messages here and there. Reported by Coverity. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Re-read from tap in tap_handler_pasta() on buffer fullStefano Brivio2022-03-301-2/+9
| | | | | | | | read() will return zero if we pass a zero length, which makes no sense: instead, track explicitly that we exhausted the buffer, flush packets to handlers and redo. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Allow ioctl() and openat() for tap_ns_tun() re-initialisationStefano Brivio2022-03-301-0/+2
| | | | | | | If the tun interface disappears, we'll call tap_ns_tun() after the seccomp profile is applied: add ioctl() and openat() to it. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap, tcp, udp, icmp: Cut down on some oversized buffersStefano Brivio2022-03-291-11/+13
| | | | | | | | | The existing sizes provide no measurable differences in throughput and packet rates at this point. They were probably needed as batched implementations were not complete, but they can be decreased quite a bit now. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Fix android-cloexec-* clang-tidy warnings, re-enable checksStefano Brivio2022-03-291-1/+2
| | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Mark constant references as constStefano Brivio2022-03-291-11/+15
| | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Packet abstraction with mandatory boundary checksStefano Brivio2022-03-291-163/+168
| | | | | | | | | | | | | | | | | | | | Implement a packet abstraction providing boundary and size checks based on packet descriptors: packets stored in a buffer can be queued into a pool (without storage of its own), and data can be retrieved referring to an index in the pool, specifying offset and length. Checks ensure data is not read outside the boundaries of buffer and descriptors, and that packets added to a pool are within the buffer range with valid offset and indices. This implies a wider rework: usage of the "queueing" part of the abstraction mostly affects tap_handler_{passt,pasta}() functions and their callees, while the "fetching" part affects all the guest or tap facing implementations: TCP, UDP, ICMP, ARP, NDP, DHCP and DHCPv6 handlers. Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Rework timers to use timerfd instead of periodic bitmap scanStefano Brivio2022-03-291-1/+1
| | | | | | | | | | | | | | | | | | With a lot of concurrent connections, the bitmap scan approach is not really sustainable. Switch to per-connection timerfd timers, set based on events and on two new flags, ACK_FROM_TAP_DUE and ACK_TO_TAP_DUE. Timers are added to the common epoll list, and implement the existing timeouts. While at it, drop the CONN_ prefix from flag names, otherwise they get quite long, and fix the logic to decide if a connection has a local, possibly unreachable endpoint: we shouldn't go through the rest of tcp_conn_from_tap() if we reset the connection due to a successful bind(2), and we'll get EACCES if the port number is low. Suggested by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* dhcpv6, tap, tcp: Use IN6_ARE_ADDR_EQUAL instead of open-coded memcmp()Stefano Brivio2022-03-281-2/+2
| | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf, util, tap: Implement --trace option for extra verbose loggingStefano Brivio2022-03-251-2/+2
| | | | | | | | --debug can be a bit too noisy, especially as single packets or socket messages are logged: implement a new option, --trace, implying --debug, that enables all debug messages. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Cast ETH_MAX_MTU to signed in comparisonsStefano Brivio2022-02-261-2/+2
| | | | | | At least gcc 8.3 and 10.2 emit a warning on armv6l and armv7l. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt: Drop PASST_LEGACY_NO_OPTIONS sectionsStefano Brivio2022-02-221-7/+0
| | | | | | ...nobody uses those builds anymore. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, pasta: Namespace-based sandboxing, defer seccomp policy applicationStefano Brivio2022-02-211-31/+27
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | To reach (at least) a conceptually equivalent security level as implemented by --enable-sandbox in slirp4netns, we need to create a new mount namespace and pivot_root() into a new (empty) mountpoint, so that passt and pasta can't access any filesystem resource after initialisation. While at it, also detach IPC, PID (only for passt, to prevent vulnerabilities based on the knowledge of a target PID), and UTS namespaces. With this approach, if we apply the seccomp filters right after the configuration step, the number of allowed syscalls grows further. To prevent this, defer the application of seccomp policies after the initialisation phase, before the main loop, that's where we expect bad things to happen, potentially. This way, we get back to 22 allowed syscalls for passt and 34 for pasta, on x86_64. While at it, move #syscalls notes to specific code paths wherever it conceptually makes sense. We have to open all the file handles we'll ever need before sandboxing: - the packet capture file can only be opened once, drop instance numbers from the default path and use the (pre-sandbox) PID instead - /proc/net/tcp{,v6} and /proc/net/udp{,v6}, for automatic detection of bound ports in pasta mode, are now opened only once, before sandboxing, and their handles are stored in the execution context - the UNIX domain socket for passt is also bound only once, before sandboxing: to reject clients after the first one, instead of closing the listening socket, keep it open, accept and immediately discard new connection if we already have a valid one Clarify the (unchanged) behaviour for --netns-only in the man page. To actually make passt and pasta processes run in a separate PID namespace, we need to unshare(CLONE_NEWPID) before forking to background (if configured to do so). Introduce a small daemon() implementation, __daemon(), that additionally saves the PID file before forking. While running in foreground, the process itself can't move to a new PID namespace (a process can't change the notion of its own PID): mention that in the man page. For some reason, fork() in a detached PID namespace causes SIGTERM and SIGQUIT to be ignored, even if the handler is still reported as SIG_DFL: add a signal handler that just exits. We can now drop most of the pasta_child_handler() implementation, that took care of terminating all processes running in the same namespace, if pasta started a shell: the shell itself is now the init process in that namespace, and all children will terminate once the init process exits. Issuing 'echo $$' in a detached PID namespace won't return the actual namespace PID as seen from the init namespace: adapt demo and test setup scripts to reflect that. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt: Address new clang-tidy warnings from LLVM 13.0.1Stefano Brivio2022-01-301-4/+4
| | | | | | | | | | | | | | | | | | | | | | | | | clang-tidy from LLVM 13.0.1 reports some new warnings from these checkers: - altera-unroll-loops, altera-id-dependent-backward-branch: ignore for the moment being, add a TODO item - bugprone-easily-swappable-parameters: ignore, nothing to do about those - readability-function-cognitive-complexity: ignore for the moment being, add a TODO item - altera-struct-pack-align: ignore, alignment is forced in protocol headers - concurrency-mt-unsafe: ignore for the moment being, add a TODO item Fix bugprone-implicit-widening-of-multiplication-result warnings, though, that's doable and they seem to make sense. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, tap: Daemonise once socket is ready without waiting for connectionStefano Brivio2022-01-281-17/+45
| | | | | | | | | | | | | The existing behaviour is not really practical: an automated agent in charge of starting both qemu and passt would need to fork itself to start passt, because passt won't fork to background until qemu connects, and the agent needs to unblock to start qemu. Instead of waiting for a connection to daemonise, do it right away as soon as a socket is available: that can be considered an initialised state already. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-11 16:24:01 +0000