October 2022 - dev

[PATCH] conf: Don't pass leading ~ to parse_port_range() on exclusions
by Stefano Brivio 25 Oct '22

25 Oct '22

Commit 84fec4e998b6 ("Clean up parsing of port ranges") drops the strspn() call before the parsing of excluded port ranges, because now we're checking against any stray characters at every step. However, that also has the effect of passing ~ as first character to the new parse_port_range(), which makes no sense: we already checked that ~ is the first character before the call, so skip it. Alona reported this output: Invalid port specifier ~15000,~15001,~15006,~15008,~15020,~15021,~15090 while the whole specifier is indeed valid. Reported-by: Alona Paz <alkaplan(a)redhat.com> Fixes: 84fec4e998b6 ("Clean up parsing of port ranges") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I'm not introducing new tests with this yet, this is another instance where I would need a test case restarting pasta a bunch of times with several different options instead of a fixed setup. I'll reopen the discussion around: https://archives.passt.top/passt-dev/Yz%2FlXVVDgMEWweTj@yekko/#t Also, I'm posting this for review, but again, let me push this out quickly, as configuration of excluded ports is completely broken at the moment. conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/conf.c b/conf.c index ed93a60..598c711 100644 --- a/conf.c +++ b/conf.c @@ -262,6 +262,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, exclude_only = false; continue; } + p++; if (parse_port_range(p, &p, &xrange)) goto bad; -- 2.35.1

2 1

[PATCH] qrap: Support JSON syntax for -device
by Andrea Bolognani 24 Oct '22

24 Oct '22

Starting with version 8.1.0, libvirt uses JSON syntax when generating the arguments to -device, so they will now look like {"driver":"virtio-scsi-pci","bus":"pci.3","addr":"0x0"} instead of virtio-scsi-pci,bus=pci.3,addr=0x0 qrap needs to parse these arguments and extract the bus number in order to figure out what address to use for the virtio-net device it adds, and the libvirt change described above has broken this parsing logic. Tweak the code so that both styles are accepted and handled correctly. Note that, when JSON is in use, qrap needs to generate its own command line options in that format as well or things will not work as expected. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- qrap.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/qrap.c b/qrap.c index a9a0fc1..3c6f5b8 100644 --- a/qrap.c +++ b/qrap.c @@ -69,6 +69,8 @@ static const struct drop_arg { * @name: Device ("-device") name to insert * @template: Prefix for device specification (first part of address) * @template_post: Suffix for device specification (last part of address) + * @template_json: Device prefix for when JSON is used + * @template_json_post: Device suffix for when JSON is used * @first: First usable PCI address * @last: Last usable PCI address */ @@ -77,15 +79,29 @@ static const struct pci_dev { char *name; char *template; char *template_post; + char *template_json; + char *template_json_post; int first; int last; } pci_devs[] = { - { "pc-q35", "virtio-net-pci", - "bus=pci.", ",addr=0x0", 3, /* 2: hotplug bus */ 16 }, - { "pc-", "virtio-net-pci", - "bus=pci.0,addr=0x", "", 2, /* 1: ISA bridge */ 16 }, - { "s390-ccw", "virtio-net-ccw", - "devno=fe.0.", "", 1, 16 }, + { + "pc-q35", "virtio-net-pci", + "bus=pci.", ",addr=0x0", + "\"bus\":\"pci.", ",\"addr\":\"0x0\"", + 3, /* 2: hotplug bus */ 16 + }, + { + "pc-", "virtio-net-pci", + "bus=pci.0,addr=0x", "", + "\"bus\":\"pci.0\",\"addr=0x", "", + 2, /* 1: ISA bridge */ 16 + }, + { + "s390-ccw", "virtio-net-ccw", + "devno=fe.0.", "", + "\"devno\":\"fe.0.", "", + 1, 16 + }, { 0 }, }; @@ -115,7 +131,7 @@ void usage(const char *name) */ int main(int argc, char **argv) { - int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, rc; + int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, has_json = 0, retry_on_reset, rc; struct timeval tv = { .tv_sec = 0, .tv_usec = (long)(500 * 1000) }; char *qemu_argv[ARG_MAX], dev_str[ARG_MAX]; struct sockaddr_un addr = { @@ -227,14 +243,22 @@ int main(int argc, char **argv) } if (!strcmp(argv[i], "-device") && i + 1 < argc) { + char *template = NULL; char *p; has_dev = 1; if ((p = strstr(argv[i + 1], dev->template))) { + template = dev->template; + } else if ((p = strstr(argv[i + 1], dev->template_json))) { + template = dev->template_json; + has_json = 1; + } + + if (template) { long n; - n = strtol(p + strlen(dev->template), NULL, 16); + n = strtol(p + strlen(template), NULL, 16); if (!errno) addr_map |= (1 << n); } @@ -254,8 +278,15 @@ int main(int argc, char **argv) if (has_dev) { qemu_argv[qemu_argc++] = "-device"; - snprintf(dev_str, ARG_MAX, "%s,%s%x%s,netdev=hostnet0,x-txburst=4096", - dev->name, dev->template, i, dev->template_post); + if (!has_json) { + snprintf(dev_str, ARG_MAX, + "%s,%s%x%s,netdev=hostnet0,x-txburst=4096", + dev->name, dev->template, i, dev->template_post); + } else { + snprintf(dev_str, ARG_MAX, + "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}", + dev->name, dev->template_json, i, dev->template_json_post); + } qemu_argv[qemu_argc++] = dev_str; } -- 2.37.3

2 3

[PATCH] util: Set NS_FN_STACK_SIZE to one eighth of ulimit-reported maximum stack size
by Stefano Brivio 24 Oct '22

24 Oct '22

...instead of one fourth. On the main() -> conf() -> nl_sock_init() call path, LTO from gcc 12 on (at least) x86_64 decides to inline... everything: nl_sock_init() is effectively part of main(), after commit 3e2eb4337bc0 ("conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()"). This means we exceed the maximum stack size, and we get SIGSEGV, under any condition, at start time, as reported by Andrea on a recent build for CentOS Stream 9. The calculation of NS_FN_STACK_SIZE, which is the stack size we reserve for clones, was previously obtained by dividing the maximum stack size by two, to avoid an explicit check on architecture (on PA-RISC, also known as hppa, the stack grows up, so we point the clone to the middle of this area), and then further divided by two to allow for any additional usage in the caller. Well, if there are essentially no function calls anymore, this is not enough. Divide it by eight, which is anyway much more than possibly needed by any clone()d callee. I think this is robust, so it's a fix in some sense. Strictly speaking, though, we have no formal guarantees that this isn't either too little or too much. What we should do, eventually: check cloned() callees, there are just thirteen of them at the moment. Note down any stack usage (they are mostly small helpers), bonus points for an automated way at build time, quadruple that or so, to allow for extreme clumsiness, and use as NS_FN_STACK_SIZE. Perhaps introduce a specific condition for hppa. Reported-by: Andrea Bolognani <abologna(a)redhat.com> Fixes: 3e2eb4337bc0 ("conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util.h b/util.h index 27829b1..c498a80 100644 --- a/util.h +++ b/util.h @@ -72,7 +72,7 @@ #define IPV4_IS_LOOPBACK(addr) \ ((addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET) -#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 4) +#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 8) #define NS_CALL(fn, arg) \ do { \ char ns_fn_stack[NS_FN_STACK_SIZE]; \ -- 2.35.1

2 4

passt crashes on CentOS Stream 9
by Andrea Bolognani 22 Oct '22

22 Oct '22

This should probably be filed on bugzilla but I can't be bothered signing up for yet another service, sorry! O:-) Short version: in a CentOS Stream 9 container, install the latest build (0^20221015.gb3f3591-1) from the official COPR, then run $ passt --runas 65534 -e -t 1234 Segmentation fault (core dumped) Doing the same thing in a CentOS Stream 8 container doesn't result in a crash, and the previous build (0^20220929.g06aa26f-1) is fine even on CentOS Stream 9. The backtrace produced by gdb doesn't look very illuminating, but maybe it will make more sense to a developer: Starting program: /usr/bin/passt --runas 65534 -e -t 1234 warning: Error disabling address space randomization: Operation not permitted [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". process 2856 is executing new program: /usr/bin/passt.avx2 warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Program received signal SIGSEGV, Segmentation fault. 0x000055663d5307ff in nl_sock_init (c=0x7fffd7fe4ed0, ns=false) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/netlink.c:78 78 { (gdb) t a a bt Thread 1 (Thread 0x7fe8763da740 (LWP 2856) "passt.avx2"): #0 0x000055663d5307ff in nl_sock_init (c=0x7fffd7fe4ed0, ns=false) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/netlink.c:78 #1 0x000055663d531296 in conf (c=<optimized out>, argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/conf.c:1547 #2 0x000055663d5262e6 in main (argc=6, argv=0x7fffd82b2a98) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/passt.c:243 A very interesting thing that I've noticed is that the crash doesn't occur when building from upstream sources (tag 2022_10_15.b3f3591, so it should match what's in the RPM). So I've tried looking into the compiler options used during the RPM build, and the gcc command line for passt.avx2 looks like gcc -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 \ -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -pie -fPIE -DPAGE_SIZE=4096 \ -DNETNS_RUN_DIR=\"/run/netns\" -DPASST_AUDIT_ARCH=AUDIT_ARCH_X86_64 \ -DRLIMIT_STACK_VAL=8192 -DARCH=\"x86_64\" \ -DVERSION=\"0^20221015.gb3f3591-1.el9.x86_64\" -DTCP_HASH_NOINLINE \ -DSIPHASH_20B_NOINLINE -DCSUM_UNALIGNED_NO_IPA -DHAS_SND_WND \ -DHAS_BYTES_ACKED -DHAS_MIN_RTT -DHAS_GETRANDOM \ -fstack-protector-strong -Ofast -mavx2 -ftree-vectorize \ -funroll-loops -flto=auto -ffat-lto-objects -fexceptions -g \ -grecord-gcc-switches -pipe -Wall -Werror=format-security \ -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS \ -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 \ -fstack-protector-strong \ -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64-v2 \ -mtune=generic -fasynchronous-unwind-tables \ -fstack-clash-protection -fcf-protection arch.c arp.c checksum.c \ conf.c dhcp.c dhcpv6.c icmp.c igmp.c isolation.c lineread.c log.c \ mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \ tap.c tcp.c tcp_splice.c udp.c util.c -o passt.avx2 -Wl,-z,relro \ -Wl,--as-needed -Wl,-z,now \ -specs=/usr/lib/rpm/redhat/redhat-hardened-ld \ -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 I tried making educated guesses at which ones among those could cause trouble, and pretty quickly landed on the LTO stuff. Indeed, dropping -flto=auto -ffat-lto-objects from the command results in a working binary, and adding %global _lto_cflags %nil to the top of the spec file produces a working RPM. Of course disabling LTO is a workaround, not a solution, especially considering that the previous version didn't have any problem with it, but hopefully there's enough information in here to allow the developers to track down and resolve the underlying issue :) -- Andrea Bolognani / Red Hat / Virtualization

2 4

[PATCH v2 00/14] Clean up checksum and header generation for inbound packets
by David Gibson 22 Oct '22

22 Oct '22

The main packet "fast paths" for UDP and TCP mostly just forward packets rather than generating them from scratch. However the control paths for ICMP and DHCP sometimes generate packets more or less from scratch. Because these are relatively rare, it's not performance critical. The paths for sending these packets have some duplication of the header generation. There's also some layering violation in tap_ip_send() which both generates IP headers and updates the L4 (UDP or UCMP) checksum. Finally that checksum generation is a little awkward: it temporarily generates the IP pseudo header (or something close enough to serve) in the place of the actual header, generates the checksum, then replaces it with the real IP header. This approach seems to be causing miscompiles with some LTO optimization, because the stores to the pseudo header are being moved or elided across the code calculating the checksum. This series addresses all of these. We consolidate and clarify the packet sending helpers, and use them in some places there was previously duplicated code. In the process we use new checksum generation helpers which take a different approach which should avoid the LTO problems (this aspect I haven't tested yet though). Changes since v1: * Numerous minor style changes * Rename header generation helpers to make their behaviour clearer * Added several missing function doc comments * Corrected some erroneous statements and terms in comments David Gibson (14): Add csum_icmp6() helper for calculating ICMPv6 checksums Add csum_icmp4() helper for calculating ICMP checksums Add csum_udp6() helper for calculating UDP over IPv6 checksums Add csum_udp4() helper for calculating UDP over IPv4 checksums Add csum_ip4_header() helper to calculate IPv4 header checksums Add helpers for normal inbound packet destination addresses Remove support for TCP packets from tap_ip_send() tap: Remove unhelpeful vnet_pre optimization from tap_send() Split tap_ip_send() into IPv4 and IPv6 specific functions tap: Split tap_ip6_send() into UDP and ICMP variants ndp: Remove unneeded eh_source parameter ndp: Use tap_icmp6_send() helper tap: Split tap_ip4_send() into UDP and ICMP variants dhcp: Use tap_udp4_send() helper in dhcp() arp.c | 2 +- checksum.c | 120 ++++++++++++++++----- checksum.h | 15 ++- dhcp.c | 19 +--- dhcpv6.c | 21 +--- icmp.c | 12 +-- ndp.c | 28 +---- ndp.h | 3 +- tap.c | 312 ++++++++++++++++++++++++++++++++++------------------- tap.h | 19 +++- 10 files changed, 345 insertions(+), 206 deletions(-) -- 2.37.3

2 16

[PATCH] Add git-publish configuration file
by Andrea Bolognani 22 Oct '22

22 Oct '22

Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- .gitpublish | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .gitpublish diff --git a/.gitpublish b/.gitpublish new file mode 100644 index 0000000..cdee64a --- /dev/null +++ b/.gitpublish @@ -0,0 +1,3 @@ +[gitpublishprofile "default"] +base = master +to = passt-dev(a)passt.top -- 2.37.3

3 2

[PATCH 00/14] Clean up checksum and header generation for inbound packets
by David Gibson 19 Oct '22

19 Oct '22

The main packet "fast paths" for UDP and TCP mostly just forward packets rather than generating them from scratch. However the control paths for ICMP and DHCP sometimes generate packets more or less from scratch. Because these are relatively rare, it's not performance critical. The paths for sending these packets have some duplication of the header generation. There's also some layering violation in tap_ip_send() which both generates IP headers and updates the L4 (UDP or UCMP) checksum. Finally that checksum generation is a little awkward: it temporarily generates the IP pseudo header (or something close enough to serve) in the place of the actual header, generates the checksum, then replaces it with the real IP header. This approach seems to be causing miscompiles with some LTO optimization, because the stores to the pseudo header are being moved or elided across the code calculating the checksum. This series addresses all of these. We consolidate and clarify the packet sending helpers, and use them in some places there was previously duplicated code. In the process we use new checksum generation helpers which take a different approach which should avoid the LTO problems (this aspect I haven't tested yet though). David Gibson (14): Add csum_icmp6() helper for calculating ICMPv6 checksums Add csum_icmp4() helper for calculating ICMPv4 checksums Add csum_udp6() helper for calculating UDP over IPv6 checksums Add csum_udp4() helper for calculating UDP over IPv4 checksums Add csum_ip4_header() helper to calculate IPv4 header checksums Add helpers for normal inbound packet destination addresses Remove support for TCP packets from tap_ip_send() tap: Remove unhelpeful vnet_pre optimization from tap_send() Split tap_ip_send() into IPv4 and IPv6 specific functions tap: Split tap_ip6_send() into UDP and ICMP variants ndp: Remove unneeded eh_source parameter ndp: Use tap_icmp6_send() helper tap: Split tap_ip4_send() into UDP and ICMP variants dhcp: Use tap_udp4_send() helper in dhcp() arp.c | 2 +- checksum.c | 122 ++++++++++++++++++----- checksum.h | 19 +++- dhcp.c | 19 +--- dhcpv6.c | 21 +--- icmp.c | 12 +-- ndp.c | 28 +----- ndp.h | 3 +- tap.c | 286 ++++++++++++++++++++++++++++++++--------------------- tap.h | 19 +++- 10 files changed, 323 insertions(+), 208 deletions(-) -- 2.37.3

2 35

[PATCH v2] conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()
by Stefano Brivio 17 Oct '22

17 Oct '22

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(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Simplify nl_sock_init_do(), add setcap for passt.avx2 in man page (David Gibson) conf.c | 71 +++++++++++++++++++++++++++---------------------------- netlink.c | 40 ++++++++++++++++--------------- netlink.h | 2 +- passt.1 | 47 ++++++++++++++++++++++++++++++------ tap.c | 1 + 5 files changed, 98 insertions(+), 63 deletions(-) diff --git a/conf.c b/conf.c index a22ef48..e1f42f1 100644 --- a/conf.c +++ b/conf.c @@ -1530,6 +1530,11 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1); + if (v4_only && v6_only) { + err("Options ipv4-only and ipv6-only are mutually exclusive"); + usage(argv[0]); + } + ret = conf_ugid(runas, &uid, &gid); if (ret) usage(argv[0]); @@ -1539,6 +1544,30 @@ void conf(struct ctx *c, int argc, char **argv) logfile, logsize); } + nl_sock_init(c, false); + if (!v6_only) + c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac); + if (!v4_only) + c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac); + if (!c->ifi4 && !c->ifi6) { + err("External interface not usable"); + exit(EXIT_FAILURE); + } + + /* Inbound port options can be parsed now (after IPv4/IPv6 settings) */ + optind = 1; + do { + struct port_fwd *fwd; + + name = getopt_long(argc, argv, optstring, options, NULL); + + if ((name == 't' && (fwd = &c->tcp.fwd_in)) || + (name == 'u' && (fwd = &c->udp.fwd_in.f))) { + if (!optarg || conf_ports(c, name, optarg, fwd)) + usage(argv[0]); + } + } while (name != -1); + if (c->mode == MODE_PASTA) { if (conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv) < 0) @@ -1561,50 +1590,20 @@ void conf(struct ctx *c, int argc, char **argv) } } - if (nl_sock_init(c)) { - err("Failed to get netlink socket"); - exit(EXIT_FAILURE); - } - - if (v4_only && v6_only) { - err("Options ipv4-only and ipv6-only are mutually exclusive"); - usage(argv[0]); - } - if (!v6_only) - c->ifi4 = conf_ip4(ifi, &c->ip4, c->mac); - if (!v4_only) - c->ifi6 = conf_ip6(ifi, &c->ip6, c->mac); - if (!c->ifi4 && !c->ifi6) { - err("External interface not usable"); - exit(EXIT_FAILURE); - } + if (c->mode == MODE_PASTA) + nl_sock_init(c, true); - /* Now we can process port configuration options */ + /* ...and outbound port options now that namespaces are set up. */ optind = 1; do { - struct port_fwd *fwd = NULL; + struct port_fwd *fwd; name = getopt_long(argc, argv, optstring, options, NULL); - switch (name) { - case 't': - case 'u': - case 'T': - case 'U': - if (name == 't') - fwd = &c->tcp.fwd_in; - else if (name == 'T') - fwd = &c->tcp.fwd_out; - else if (name == 'u') - fwd = &c->udp.fwd_in.f; - else if (name == 'U') - fwd = &c->udp.fwd_out.f; + if ((name == 'T' && (fwd = &c->tcp.fwd_out)) || + (name == 'U' && (fwd = &c->udp.fwd_out.f))) { if (!optarg || conf_ports(c, name, optarg, fwd)) usage(argv[0]); - - break; - default: - break; } } while (name != -1); diff --git a/netlink.c b/netlink.c index 6e5a96b..0850cbe 100644 --- a/netlink.c +++ b/netlink.c @@ -19,6 +19,7 @@ #include <sys/types.h> #include <limits.h> #include <stdlib.h> +#include <stdbool.h> #include <stdint.h> #include <unistd.h> #include <arpa/inet.h> @@ -39,57 +40,58 @@ static int nl_sock_ns = -1; static int nl_seq; /** - * nl_sock_init_do() - Set up netlink sockets in init and target namespace - * @arg: Execution context + * nl_sock_init_do() - Set up netlink sockets in init or target namespace + * @arg: Execution context, if running from namespace, NULL otherwise * * Return: 0 */ static int nl_sock_init_do(void *arg) { struct sockaddr_nl addr = { .nl_family = AF_NETLINK, }; - int *s = &nl_sock; + int *s = arg ? &nl_sock_ns : &nl_sock; #ifdef NETLINK_GET_STRICT_CHK int y = 1; #endif -ns: + if (arg) + ns_enter((struct ctx *)arg); + if (((*s) = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) < 0 || - bind(*s, (struct sockaddr *)&addr, sizeof(addr))) + bind(*s, (struct sockaddr *)&addr, sizeof(addr))) { *s = -1; - - if (*s == -1 || !arg || s == &nl_sock_ns) return 0; + } #ifdef NETLINK_GET_STRICT_CHK if (setsockopt(*s, SOL_NETLINK, NETLINK_GET_STRICT_CHK, &y, sizeof(y))) debug("netlink: cannot set NETLINK_GET_STRICT_CHK on %i", *s); #endif - - ns_enter((struct ctx *)arg); - s = &nl_sock_ns; - goto ns; + return 0; } /** - * nl_sock_init() - Call nl_sock_init_do() and check for failures + * nl_sock_init() - Call nl_sock_init_do(), won't return on failure * @c: Execution context - * - * Return: -EIO if sockets couldn't be set up, 0 otherwise + * @ns: Get socket in namespace, not in init */ -int nl_sock_init(const struct ctx *c) +void nl_sock_init(const struct ctx *c, bool ns) { - if (c->mode == MODE_PASTA) { + if (ns) { NS_CALL(nl_sock_init_do, c); if (nl_sock_ns == -1) - return -EIO; + goto fail; } else { nl_sock_init_do(NULL); } if (nl_sock == -1) - return -EIO; + goto fail; - return 0; + return; + +fail: + err("Failed to get netlink socket"); + exit(EXIT_FAILURE); } /** diff --git a/netlink.h b/netlink.h index 5ce5037..3c991cc 100644 --- a/netlink.h +++ b/netlink.h @@ -6,7 +6,7 @@ #ifndef NETLINK_H #define NETLINK_H -int nl_sock_init(const struct ctx *c); +void nl_sock_init(const struct ctx *c, bool ns); unsigned int nl_get_ext_if(sa_family_t af); void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw); void nl_addr(int ns, unsigned int ifi, sa_family_t af, diff --git a/passt.1 b/passt.1 index 7d113f2..4848087 100644 --- a/passt.1 +++ b/passt.1 @@ -771,14 +771,47 @@ possible to bind sockets to foreign addresses. .SS Binding to low numbered ports (well-known or system ports, up to 1023) -If the port forwarding configuration requires binding to port numbers lower than -1024, \fBpasst\fR and \fBpasta\fR will try to bind to them, but will fail if not -running as root, or without the \fICAP_NET_BIND_SERVICE\fR Linux capability, see -\fBservices\fR(5) and \fBcapabilities\fR(7). To grant the -\fICAP_NET_BIND_SERVICE\fR capability to passt, you can issue, as root: +If the port forwarding configuration requires binding to ports with numbers +lower than 1024, \fBpasst\fR and \fBpasta\fR will try to bind to them, but will +fail, unless, either: + +.IP \(bu 2 +the \fIsys.net.ipv4.ip_unprivileged_port_start\fR sysctl is set to the number +of the lowest port \fBpasst\fR and \fBpasta\fR need. For example, as root: + +.nf + sysctl -w net.ipv4.ip_unprivileged_port_start=443 +.fi + +\fBNote\fR: this is the recommended way of enabling \fBpasst\fR and \fBpasta\fR +to bind to ports with numbers below 1024. + +.IP \(bu +or the \fICAP_NET_BIND_SERVICE\fR Linux capability is granted, see +\fBservices\fR(5) and \fBcapabilities\fR(7). + +This is, in general, \fBnot the recommended way\fR, because \fBpasst\fR and +\fBpasta\fR might be used as vector to effectively use this capability from +another process. + +However, if your environment is sufficiently controlled by an LSM (Linux +Security Module) such as \fIAppArmor\fR, \fISELinux\fR, \fISmack\fR or +\fITOMOYO\fR, and no other processes can interact in such a way in virtue of +this, granting this capability to \fBpasst\fR and \fBpasta\fR only can +effectively prevent other processes from utilising it. + +Note that this will not work for automatic detection and forwarding of ports +with \fBpasta\fR, because \fBpasta\fR will relinquish this capability at +runtime. + +To grant this capability, you can issue, as root: + +.nf + for p in $(which passt passt.avx2); do + setcap 'cap_net_bind_service=+ep' "${p}" + done +.fi -.RS -setcap 'cap_net_bind_service=+ep' $(which passt) .RE .SS ICMP/ICMPv6 Echo sockets diff --git a/tap.c b/tap.c index 77e513c..8b6d9bc 100644 --- a/tap.c +++ b/tap.c @@ -30,6 +30,7 @@ #include <sys/stat.h> #include <fcntl.h> #include <sys/uio.h> +#include <stdbool.h> #include <stdlib.h> #include <unistd.h> #include <netinet/ip.h> -- 2.35.1

2 1

Alas for CAP_NET_BIND_SERVICE
by David Gibson 17 Oct '22

17 Oct '22

Hi Stefano, I've looked deeper into why giving passt/pasta CAP_NET_BIND_SERVICE isn't working, and I'm afraid I have bad news. We lose CAP_NET_BIND_SERVICE in the initial namespace as soon as we unshare() or setns() into the isolated namespace, and this appears to be intended behaviour. From user_namespaces(7), in the Capabilities section: The child process created by clone(2) with the CLONE_NEWUSER flag starts out with a complete set of capabilities in the new user namespace. Likewise, a process that creates a new user namespace using unshare(2) or joins an existing user namespace using setns(2) gains a full set of capabilities in that namespace. ***On the other hand, that process has no capabilities in the parent (in the case of clone(2)) or previous (in the case of unshare(2) and setns(2)) user namespace, even if the new namespace is created or joined by the root user (i.e., a process with user ID 0 in the root namespace).*** Emphasis (***) mine. Basically, despite the way it's phrased in many places, processes don't have an independent set of capabilities in each userns, they only have a set of capabilities in their current userns. Any capabilities in other namespaces are implied in a pretty much all or nothing way - if the process's UID (the real, init ns one) owns the userns (or one of its ancestors), it gets all caps, otherwise none. cap_capable() has the specific logic in the kernel. So, using CAP_NET_BIND_SERVICE isn't compatible with isolating ourselves in our own userns. At the very least "auto" inbound forwarding of low ports is pretty much off the cards. For forwarding of specific low ports, we could delay our entry into the new userns until we've set up the listening sockets, although it does mean rolling back some of the simplification we gained from the new-style userns handling. Or, we could abandon CAP_NET_BIND_SERVICE, and recommend the net.ipv4.ip_unprivileged_port_start sysctl as the only way to handle low ports in passt. I do see a fair bit of logic in that approach: passt has no meaningful way to limit what users do with the low ports it allows them (indirectly) to bind to, giving passt CAP_NET_BIND_SERVICE is pretty much equivalent to giving any process which can invoke passt CAP_NET_BIND_SERVICE. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

2 11

all pending patches pushed
by Stefano Brivio 15 Oct '22

15 Oct '22

Up to: 31fc8ed passt.1: Add David to AUTHORS -- Stefano

1 0