As we add more complexity to what forwarding rules are allowed, our existing ad-hoc C parsing is starting to become quite awkward. We already have some parts that resemble a very simple recursive[0] descent parser, with composable subfunctions that consume as much input as they need, rather than pre-splitting based on known delimiters. Start extending this approach, by creating parse.[ch] with parsing helpers with a uniform interface. Initially we start with very simple cases: the parse_keyword() function from fwd_rule.c and another helper to check that we've reached the end of input. Rename parse_keyword() to parse_literal(), because it's not just useful for "keywords" as such. We can use it in a bunch of additional places for parsing delimiters and other symbols. This doesn't gain us a lot now but will make things clearer as we use more such parser helpers. [0] Except that the grammars we have aren't actually recursive, so neither is the code. Signed-off-by: David Gibson --- Makefile | 17 ++++++++------ conf.c | 5 +++- fwd_rule.c | 33 +++++--------------------- parse.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ parse.h | 14 +++++++++++ 5 files changed, 102 insertions(+), 35 deletions(-) create mode 100644 parse.c create mode 100644 parse.h diff --git a/Makefile b/Makefile index 5ed0f702..bd729c75 100644 --- a/Makefile +++ b/Makefile @@ -36,12 +36,13 @@ BASE_CFLAGS += -pedantic -Wall -Wextra -Wno-format-zero-length -Wformat-security PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \ epoll_ctl.c flow.c fwd.c fwd_rule.c icmp.c igmp.c inany.c iov.c ip.c \ isolation.c lineread.c log.c mld.c ndp.c netlink.c migrate.c packet.c \ - passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \ + parse.c passt.c pasta.c pcap.c pif.c repair.c serialise.c tap.c tcp.c \ tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c PASST_REPAIR_SRCS = passt-repair.c -PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c serialise.c +PESTO_SRCS = pesto.c bitmap.c fwd_rule.c inany.c ip.c lineread.c parse.c \ + serialise.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS) MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1 @@ -49,13 +50,14 @@ MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1 PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \ epoll_ctl.h flow.h fwd.h fwd_rule.h flow_table.h icmp.h icmp_flow.h \ inany.h iov.h ip.h isolation.h lineread.h log.h migrate.h ndp.h \ - netlink.h packet.h passt.h pasta.h pcap.h pif.h repair.h serialise.h \ - siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \ - tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \ - virtio.h vu_common.h + netlink.h packet.h parse.h passt.h pasta.h pcap.h pif.h repair.h \ + serialise.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \ + tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \ + vhost_user.h virtio.h vu_common.h QRAP_HEADERS = arp.h ip.h passt.h util.h PASST_REPAIR_HEADERS = linux_dep.h -PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h pesto.h serialise.h +PESTO_HEADERS = bitmap.h common.h fwd_rule.h inany.h ip.h log.h parse.h \ + pesto.h serialise.h C := \#include \nint main(){int a=getrandom(0, 0, 0);} ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) @@ -223,6 +225,7 @@ pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:bitmap.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.h pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h +pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:parse.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h diff --git a/conf.c b/conf.c index 6ab8efec..ca6c859c 100644 --- a/conf.c +++ b/conf.c @@ -52,6 +52,7 @@ #include "conf.h" #include "pesto.h" #include "serialise.h" +#include "parse.h" #define NETNS_RUN_DIR "/run/netns" @@ -1028,7 +1029,9 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) static void conf_nat(const char *arg, struct in_addr *addr4, struct in6_addr *addr6, int *no_map_gw) { - if (strcmp(arg, "none") == 0) { + const char *p = arg; + + if (parse_literal(&p, "none") && parse_eoi(p)) { *addr4 = in4addr_any; *addr6 = in6addr_any; if (no_map_gw) diff --git a/fwd_rule.c b/fwd_rule.c index 04a0101d..d0e90402 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -24,6 +24,7 @@ #include "fwd_rule.h" #include "lineread.h" #include "log.h" +#include "parse.h" #include "serialise.h" /* Ephemeral port range: values from RFC 6335 */ @@ -427,28 +428,6 @@ static int parse_port_range(const char *s, const char **endptr, return 0; } -/** - * parse_keyword() - Parse a literal keyword - * @s: String to parse - * @endptr: Update to the character after the keyword - * @kw: Keyword to accept - * - * Return: 0, if @s starts with @kw, -EINVAL if it does not - */ -static int parse_keyword(const char *s, const char **endptr, const char *kw) -{ - size_t len = strlen(kw); - - if (strlen(s) < len) - return -EINVAL; - - if (memcmp(s, kw, len)) - return -EINVAL; - - *endptr = s + len; - return 0; -} - /** * fwd_rule_range_except() - Set up forwarding for a range of ports minus a * bitmap of exclusions @@ -568,7 +547,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, continue; } - if (parse_keyword(p, &p, "auto") == 0) { + if (parse_literal(&p, "auto")) { if (p != ep) /* Garbage after the keyword */ goto bad; @@ -582,9 +561,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, } /* Should be an exclude range */ - if (*p != '~') + if (!parse_literal(&p, "~")) goto bad; - p++; if (parse_port_range(p, &p, &xrange)) goto bad; @@ -616,8 +594,9 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, if (parse_port_range(p, &p, &orig_range)) goto bad; - if (*p == ':') { /* There's a range to map to as well */ - if (parse_port_range(p + 1, &p, &mapped_range)) + if (parse_literal(&p, ":")) { + /* There's a range to map to as well */ + if (parse_port_range(p, &p, &mapped_range)) goto bad; if ((mapped_range.last - mapped_range.first) != (orig_range.last - orig_range.first)) diff --git a/parse.c b/parse.c new file mode 100644 index 00000000..4e3b527d --- /dev/null +++ b/parse.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * parse.c - Composable parsing helpers + * + * Copyright Red Hat + * Author: David Gibson + */ + +#include +#include +#include +#include +#include +#include + +#include "common.h" + +/** + * DOC: Theory of Operation + * + * These are a number of primitives which can be combined into parsers for + * moderately complex input. For simpler composability, they have common + * conventions. + * + * - Functions return a bool indicating whether they successfully parsed. + * - Any specific output from the parse is as output parameters + * - First argument is always @cursor, a const char **. + * - On entry, *@cursor has the point to start parsing. + * - On successful exit, *@cursor is updated to the next character after the + * parsed portion of the input + * - On failure, *@cursor and any output arguments are not modified + * + * For brevity the common parameters and return information are omitted from the + * individual function documentation comments. + */ + +/** + * parse_literal() - Parse a specified literal string + * @cursor: Point to parse from, updated on success + * @lit: Keyword to accept + */ +bool parse_literal(const char **cursor, const char *lit) +{ + size_t len = strlen(lit); + + if (strlen(*cursor) < len || memcmp(*cursor, lit, len)) + return false; + + *cursor += len; + return true; +} + +/** + * parse_eoi() - Parse end of input + * @cursor: Point to parse from + * + * Return: true if @p is at End of Input (\0), false otherwise + */ +bool parse_eoi(const char *cursor) +{ + return !(*cursor); +} diff --git a/parse.h b/parse.h new file mode 100644 index 00000000..eb51a469 --- /dev/null +++ b/parse.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright Red Hat + * Author: David Gibson + */ + +#ifndef PARSE_H +#define PARSE_H + +#include + +bool parse_literal(const char **cursor, const char *lit); +bool parse_eoi(const char *cursor); + +#endif /* _PARSE_H */ -- 2.55.0

The -F option is parsed in conf() along with everything else. However, because it informs what fds we can close at startup, we also have a special case parse of it in close_open_files(). At present we duplicate the parsing and validation code, which is a bit risky. Avoid that with a conf_tap_fd() helper. Signed-off-by: David Gibson --- conf.c | 29 +++++++++++++++++++---------- conf.h | 1 + util.c | 12 +++--------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/conf.c b/conf.c index ca6c859c..32163f56 100644 --- a/conf.c +++ b/conf.c @@ -1153,6 +1153,24 @@ static void conf_sock_listen(const struct ctx *c) die_perror("Couldn't add configuration socket to epoll"); } +/** + * conf_tap_fd() - Read tap fd as supplied by -F command line option + * @arg: Argument to -F command line option + */ +int conf_tap_fd(const char *arg) +{ + long val; + + errno = 0; + val = strtol(arg, NULL, 0); + + if (errno || (val != STDIN_FILENO && val <= STDERR_FILENO) || + val > INT_MAX) + die("Invalid --fd: %s", arg); + + return val; +} + /** * conf() - Process command-line arguments and set configuration * @c: Execution context @@ -1253,7 +1271,6 @@ void conf(struct ctx *c, int argc, char **argv) const char *logfile = NULL; char *runas = NULL; size_t logsize = 0; - long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1506,15 +1523,7 @@ void conf(struct ctx *c, int argc, char **argv) c->fd_control_listen = c->fd_control = -1; break; case 'F': - errno = 0; - fd_tap_opt = strtol(optarg, NULL, 0); - - if (errno || - (fd_tap_opt != STDIN_FILENO && fd_tap_opt <= STDERR_FILENO) || - fd_tap_opt > INT_MAX) - die("Invalid --fd: %s", optarg); - - c->fd_tap = fd_tap_opt; + c->fd_tap = conf_tap_fd(optarg); c->one_off = true; *c->sock_path = 0; break; diff --git a/conf.h b/conf.h index 16f97189..1fa1280e 100644 --- a/conf.h +++ b/conf.h @@ -7,6 +7,7 @@ #define CONF_H enum passt_modes conf_mode(int argc, char *argv[]); +int conf_tap_fd(const char *arg); void conf(struct ctx *c, int argc, char **argv); void conf_listen_handler(struct ctx *c, uint32_t events); void conf_handler(struct ctx *c, uint32_t events); diff --git a/util.c b/util.c index 0aa4d42d..4bc5d6f8 100644 --- a/util.c +++ b/util.c @@ -38,6 +38,7 @@ #include "epoll_ctl.h" #include "pasta.h" #include "serialise.h" +#include "conf.h" #ifdef HAS_GETRANDOM #include #endif @@ -939,15 +940,8 @@ void close_open_files(int argc, char **argv) do { name = getopt_long(argc, argv, "-:F:", optfd, NULL); - if (name == 'F') { - errno = 0; - fd = strtol(optarg, NULL, 0); - - if (errno || - (fd != STDIN_FILENO && fd <= STDERR_FILENO) || - fd > INT_MAX) - die("Invalid --fd: %s", optarg); - } + if (name == 'F') + fd = conf_tap_fd(optarg); } while (name != -1); if (fd == -1) { -- 2.55.0

conf_addr() sets c->ip[46]no_copy_addrs conditional upon being in pasta mode. That sort of makes sense, since address copying is only a thing for pasta mode. However, setting the variables anyway is harmless, and arguably more logically consistent. If we had a way of copying addresses for passt mode (or some future mode), it would still be incorrect to do so in these circumstances. So, make the assignments unconditional. Signed-off-by: David Gibson --- conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/conf.c b/conf.c index 3614776c..d1889e0c 100644 --- a/conf.c +++ b/conf.c @@ -1203,13 +1203,11 @@ static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n) c->ip4.addr = *inany_v4(&addr); c->ip4.prefix_len = prefix_len - 96; c->ip4.addr_fixed = true; - if (c->mode == MODE_PASTA) - c->ip4.no_copy_addrs = true; + c->ip4.no_copy_addrs = true; } else { c->ip6.addr = addr.a6; c->ip6.addr_fixed = true; - if (c->mode == MODE_PASTA) - c->ip6.no_copy_addrs = true; + c->ip6.no_copy_addrs = true; } return is_prefix; -- 2.55.0

Currently we handle -t all and the like as a special case, it can't be combined with other port specifier options. Remove that restriction, allowing combined options like: -t all,~9999 # Forward everything non-ephemeral except 9999 -t all,auto # Equivalent to -t auto -t all,33000 # Forward non-ephemeral plus port 33,000 This isn't particularly useful immediately, but will become important for destination address specification - it provides a place to attach the target address for "all" or exclude only mappings. It will also work better with some parsing reworks we want to make. Signed-off-by: David Gibson --- conf.c | 11 +++++------ fwd_rule.c | 39 ++++++++++++++++++++------------------- passt.1 | 33 ++++++++++++++++----------------- 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/conf.c b/conf.c index c4a36dee..a610c0c6 100644 --- a/conf.c +++ b/conf.c @@ -660,11 +660,9 @@ static void usage(const char *name, FILE *f, int status) " SPEC can be:\n" " 'none': don't forward any ports\n" " [ADDR[%%IFACE]/]PORTS: forward specific ports\n" - " PORTS is either 'all' (forward all unbound, non-ephemeral\n" - " ports), or a comma-separated list of ports, optionally\n" - " ranged with '-' and optional target ports after ':'.\n" - " Ranges can be reduced by excluding ports or ranges\n" - " prefixed by '~'.\n" + " PORTS is comma-separated list of ports, either\n" + " 'all', a port number or range. Ranges can be reduced\n" + " by excluding ports or ranges prefixed by '~'.\n" "%s" " Examples:\n" " -t all Forward all ports\n" @@ -677,7 +675,8 @@ static void usage(const char *name, FILE *f, int status) " corresponding port numbers plus 10\n" " -t 192.0.2.1/5 Bind port 5 of 192.0.2.1 to %s\n" " -t 5-25,~10-20 Forward ports 5 to 9, and 21 to 25\n" - " -t ~25 Forward all ports except for 25\n" + " -t ~25,all\n" + " -t 25 Forward all ports except for 25\n" "%s" " default: %s\n" " -u, --udp-ports SPEC UDP port forwarding to %s\n" diff --git a/fwd_rule.c b/fwd_rule.c index 6d7ec2c5..b14df340 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -471,20 +471,13 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, uint8_t flags = 0; unsigned i; - if (!strcmp(spec, "all")) { - /* Treat "all" as equivalent to "": all non-ephemeral ports */ - spec = ""; - } - /* Parse excluded ranges and "auto" in the first pass */ for_each_chunk(p, ep, spec, ",") { struct port_range xrange; - if (isdigit(*p)) { - /* Include range, parse later */ - exclude_only = false; + /* Include range, parse later */ + if (parse_literal(&p, "all") || isdigit(*p)) continue; - } if (parse_literal(&p, "auto")) { if (p != ep) /* Garbage after the keyword */ @@ -512,20 +505,18 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, bitmap_set(exclude, i); } - if (exclude_only) { - /* Exclude ephemeral ports */ - fwd_port_map_ephemeral(exclude); - - fwd_rule_range_except(fwd, del, proto, addr, ifname, - 1, NUM_PORTS - 1, exclude, - 1, flags | FWD_WEAK); - return; - } - /* Now process base ranges, skipping exclusions */ for_each_chunk(p, ep, spec, ",") { struct port_range orig_range, mapped_range; + /* Handle "all" like exclude only */ + if (parse_literal(&p, "all")) { + if (p != ep) /* Garbage after the keyword */ + goto bad; + + continue; + } + if (!isdigit(*p)) /* Already parsed */ continue; @@ -533,6 +524,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, if (!parse_port_range(&p, &orig_range)) goto bad; + exclude_only = false; + if (parse_literal(&p, ":")) { /* There's a range to map to as well */ if (!parse_port_range(&p, &mapped_range)) @@ -553,6 +546,14 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, mapped_range.first, flags); } + /* Finally handle "all" and exclude only specs */ + if (exclude_only) { + fwd_port_map_ephemeral(exclude); + + fwd_rule_range_except(fwd, del, proto, addr, ifname, + 1, NUM_PORTS - 1, exclude, + 1, flags | FWD_WEAK); + } return; bad: die("Invalid port specifier '%s'", spec); diff --git a/passt.1 b/passt.1 index 908fd4a4..c3722ef9 100644 --- a/passt.1 +++ b/passt.1 @@ -432,29 +432,22 @@ Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39. .TP .BR \-t ", " \-\-tcp-ports " " \fIspec -Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of: +Configure TCP port forwarding to guest or namespace. \fIspec\fR can be either: .RS .TP .BR none Don't forward any ports +or .TP [\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ... -Specific ports to forward. Optionally, a specific listening address -and interface name (since Linux 5.7) can be specified. \fIports\fR -may be either: -.RS -.TP -\fBall\fR -Forward all unbound, non-ephemeral ports, as permitted by current -capabilities. For low (< 1024) ports, see \fBNOTES\fR. No failures -are reported for unavailable ports, unless no ports could be forwarded -at all. + +Ports to forward. Optionally, a specific listening address and +interface name (since Linux 5.7) can be specified. .RE -.RS -or a comma-separated list of entries which may be any of: +\fIports\fR is a comma-separated list of entries which may be any of: .TP \fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]] Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR @@ -468,6 +461,13 @@ as \fIfirst\fR. Exclude range. Don't forward port numbers between \fIfirst\fR and \fIlast\fR. This takes precedences over include ranges. +.TP +.BR all +Forward all unbound, non-ephemeral ports, not covered by exclude +ranges above, as permitted by current capabilities. For low (< 1024) +ports, see \fBNOTES\fR. No failures are reported for unavailable +ports, unless no ports could be forwarded at all. + .TP .BR auto \fBpasta\fR only. Only forward ports in the specified set if the @@ -477,10 +477,9 @@ periodically derived (every second) from listening sockets reported by .RE Specifying excluded ranges only implies that all other non-ephemeral -ports are forwarded. Specifying no ranges at all implies forwarding -all non-ephemeral ports permitted by current capabilities. In this -case, no failures are reported for unavailable ports, unless no ports -could be forwarded at all. +ports are forwarded. Specifying no ranges is equivalent +to '\fBall\fR'. In this case, no failures are reported for +unavailable ports, unless no ports could be forwarded at all. Examples: .RS -- 2.55.0

parse_port_range() in fwd_rule.c takes an approach similar to that used by the new parsing helpers in parse.c (and is partially inspiration for it), but isn't quite the same. Move it into parse.c, and bring it into line with that file's conventions. Signed-off-by: David Gibson --- common.h | 3 +++ fwd_rule.c | 53 +++-------------------------------------------------- fwd_rule.h | 2 -- parse.c | 28 ++++++++++++++++++++++++++++ parse.h | 13 +++++++++++++ 5 files changed, 47 insertions(+), 52 deletions(-) diff --git a/common.h b/common.h index f2518f66..16b9dc8c 100644 --- a/common.h +++ b/common.h @@ -113,4 +113,7 @@ static inline const char *strerror_(int errnum) #define ntohll(x) (be64toh((x))) #define htonll(x) (htobe64((x))) +/* Number of ports for both TCP and UDP */ +#define NUM_PORTS (1U << 16) + #endif /* COMMON_H */ diff --git a/fwd_rule.c b/fwd_rule.c index d4b422d7..0b85b17e 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -367,53 +367,6 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) return 0; } -/** - * port_range() - Represents a non-empty range of ports - * @first: First port number in the range - * @last: Last port number in the range (inclusive) - * - * Invariant: @last >= @first - */ -struct port_range { - in_port_t first, last; -}; - -/** - * parse_port_range() - Parse a range of port numbers '<first>[-<last>]' - * @s: String to parse - * @endptr: Update to the character after the parsed range (similar to - * strtol() etc.) - * @range: Update with the parsed values on success - * - * Return: -EINVAL on parsing error, -ERANGE on out of range port - * numbers, 0 on success - */ -static int parse_port_range(const char *s, const char **endptr, - struct port_range *range) -{ - unsigned long first, last; - char *ep; - - last = first = strtoul(s, &ep, 10); - if (ep == s) /* Parsed nothing */ - return -EINVAL; - if (*ep == '-') { /* we have a last value too */ - const char *lasts = ep + 1; - last = strtoul(lasts, &ep, 10); - if (ep == lasts) /* Parsed nothing */ - return -EINVAL; - } - - if ((last < first) || (last >= NUM_PORTS)) - return -ERANGE; - - range->first = first; - range->last = last; - *endptr = ep; - - return 0; -} - /** * fwd_rule_range_except() - Set up forwarding for a range of ports minus a * bitmap of exclusions @@ -550,7 +503,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, if (!parse_literal(&p, "~")) goto bad; - if (parse_port_range(p, &p, &xrange)) + if (!parse_port_range(&p, &xrange)) goto bad; if (p != ep) /* Garbage after the range */ goto bad; @@ -577,12 +530,12 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, /* Already parsed */ continue; - if (parse_port_range(p, &p, &orig_range)) + if (!parse_port_range(&p, &orig_range)) goto bad; if (parse_literal(&p, ":")) { /* There's a range to map to as well */ - if (parse_port_range(p, &p, &mapped_range)) + if (!parse_port_range(&p, &mapped_range)) goto bad; if ((mapped_range.last - mapped_range.first) != (orig_range.last - orig_range.first)) diff --git a/fwd_rule.h b/fwd_rule.h index ae9a3cb0..435be5bd 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -18,8 +18,6 @@ #include "inany.h" #include "bitmap.h" -/* Number of ports for both TCP and UDP */ -#define NUM_PORTS (1U << 16) #define PORT_BITMAP_SIZE DIV_ROUND_UP(NUM_PORTS, 8) /* Forwarding capability bits */ diff --git a/parse.c b/parse.c index 9bd0e81f..ad210406 100644 --- a/parse.c +++ b/parse.c @@ -17,6 +17,7 @@ #include #include +#include "common.h" #include "parse.h" /** @@ -83,3 +84,30 @@ bool parse_unsigned(const char **cursor, int base, unsigned long *valp) *cursor = p; return true; } + +/** + * parse_port_range() - Parse a range of port numbers '<first>[-<last>]' + * @range: Update with the parsed values on success + */ +bool parse_port_range(const char **cursor, struct port_range *range) +{ + unsigned long first, last; + const char *p = *cursor; + + if (!parse_unsigned(&p, 10, &first)) + return false; + + last = first; + + if (parse_literal(&p, "-")) + if (!parse_unsigned(&p, 10, &last)) + return false; + + if ((last < first) || (last >= NUM_PORTS)) + return false; + + range->first = first; + range->last = last; + *cursor = p; + return true; +} diff --git a/parse.h b/parse.h index 4f38c28b..155b3995 100644 --- a/parse.h +++ b/parse.h @@ -7,9 +7,22 @@ #define PARSE_H #include +#include + +/** + * port_range() - Represents a non-empty range of ports + * @first: First port number in the range + * @last: Last port number in the range (inclusive) + * + * Invariant: @last >= @first + */ +struct port_range { + in_port_t first, last; +}; bool parse_literal(const char **cursor, const char *lit); bool parse_eoi(const char *cursor); bool parse_unsigned(const char **cursor, int base, unsigned long *valp); +bool parse_port_range(const char **cursor, struct port_range *range); #endif /* _PARSE_H */ -- 2.55.0

Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse() and fwd_rule_parse_ports(). Currently those use a mix of loosely recursive descent parsing helpers, consuming input from left to right, and ad-hoc C parsing - searching for delimiters with strchr() or the like and breaking down on that basis. As well as being a slightly awkward mix, the current approach will not work for adding target addresses to the rules: we can't easily split the listening from target parts first, because the ':' delimiter could appear multiple times for multiple port ranges, or in IPv6 addresses. However, without splitting the target part first, we can't first split off the listening address and interface because the '/' delimiter could appear in the target portion, but not the listening portion, e.g.: -t 12345:192.0.1.1/12345 To address this, rewrite the entirety of the parsing so we consume the input left to right in a more-or-less recursive descent manner. This means we no longer rely on certain delimiters having a single meaning over the whole input, just the next part of it. Because of the semantics of port specs, we need to make several passes over the list of comma separated chunks. Previously, some of the logic was duplicated between the two passes, making it fragile. Now, we introduce a parse_port_chunk() helper which we re-use in each pass to make it clearer we parse exactly the same way on each pass. Signed-off-by: David Gibson --- fwd_rule.c | 270 +++++++++++++++++++++++++++++++---------------------- parse.c | 29 ++++++ parse.h | 2 + 3 files changed, 189 insertions(+), 112 deletions(-) diff --git a/fwd_rule.c b/fwd_rule.c index b14df340..ef35e1b4 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -439,17 +439,102 @@ fail: fwd_rule_fmt(&rule, rulestr, sizeof(rulestr))); } -/* - * for_each_chunk - Step through delimited chunks of a string - * @p_: Pointer to start of each chunk (updated) - * @ep_: Pointer to end of each chunk (updated) - * @s_: String to step through - * @sep_: String of all allowed delimiters +/** + * enum fwd_port_chunk_kind - Kind of port specifier piece + * @CHUNK_ALL "all" + * @CHUNK_AUTO "auto" + * @CHUNK_EXCLUDE "~1111[-2222]" + * @CHUNK_INCLUDE "1111[-2222][:3333[-4444]]" */ -#define for_each_chunk(p_, ep_, s_, sep_) \ - for ((p_) = (s_); \ - (ep_) = (p_) + strcspn((p_), (sep_)), *(p_); \ - (p_) = *(ep_) ? (ep_) + 1 : (ep_)) +enum fwd_port_chunk_kind { + CHUNK_ALL, + CHUNK_AUTO, + CHUNK_EXCLUDE, + CHUNK_INCLUDE, +}; + +/** + * parse_port_chunk() - Parse one chunk of a port specifier + * @cursor: Parsing point (see parse.c) + * @kindp: Updated with kind of chunk we parsed + * @lrange: Updated with listening port range (for INCLUDE & EXCLUDE) + * @trange: Updated with target port range (for INCLUDE) + */ +static bool parse_port_chunk(const char **cursor, + enum fwd_port_chunk_kind *kindp, + struct port_range *lrange, + struct port_range *trange) +{ + struct port_range lr = { 0 }, tr = { 0 }; + enum fwd_port_chunk_kind kind; + const char *p = *cursor; + + if (parse_literal(&p, "all")) { + kind = CHUNK_ALL; + } else if (parse_literal(&p, "auto")) { + kind = CHUNK_AUTO; + } else if (parse_literal(&p, "~")) { + kind = CHUNK_EXCLUDE; + if (!parse_port_range(&p, &lr)) + return false; + } else if (parse_port_range(&p, &lr)) { + kind = CHUNK_INCLUDE; + + if (parse_literal(&p, ":")) { + if (!parse_port_range(&p, &tr)) + return false; + } else { + tr = lr; + } + } else { + return false; + } + + *kindp = kind; + *lrange = lr; + if (trange) + *trange = tr; + *cursor = p; + return true; +} + +/** + * parse_addrifname() - Parse ADDRESS[%IFNAME]/ + * @cursor: Parsing cursor (see parse.c) + * @addr: Updated with parsed inany address (NULL for *) + * @abuf: Buffer to store address + * @ifname: Updated with parsed interface name ("" if none) + */ +static bool parse_addrifname(const char **cursor, + const union inany_addr **addr, + union inany_addr *abuf, + char *ifname) +{ + union inany_addr atmp = inany_any6; + char iftmp[IFNAMSIZ] = {0}; + const char *p; + + if (p = *cursor, + parse_inany(&p, &atmp) && + parse_ifspec(&p, iftmp) && + parse_literal(&p, "/")) { + /* Specific listening address */ + *addr = abuf; + } else if (p = *cursor, + parse_literal(&p, "*"), + parse_ifspec(&p, iftmp) && + parse_literal(&p, "/")) { + /* Missing or "*" address */ + *addr = NULL; + } else { + return false; + } + + *abuf = atmp; + memcpy(ifname, iftmp, IFNAMSIZ); + *cursor = p; + return true; +} /** * fwd_rule_parse_ports() - Parse port range(s) specifier @@ -466,87 +551,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, const char *spec) { uint8_t exclude[PORT_BITMAP_SIZE] = { 0 }; + enum fwd_port_chunk_kind kind; + struct port_range lrange; bool exclude_only = true; - const char *p, *ep; uint8_t flags = 0; + const char *p; unsigned i; - /* Parse excluded ranges and "auto" in the first pass */ - for_each_chunk(p, ep, spec, ",") { - struct port_range xrange; - - /* Include range, parse later */ - if (parse_literal(&p, "all") || isdigit(*p)) - continue; - - if (parse_literal(&p, "auto")) { - if (p != ep) /* Garbage after the keyword */ - goto bad; + /* Consider excluded ranges and "auto" in the first pass */ + p = spec; + do { + if (!parse_port_chunk(&p, &kind, &lrange, NULL)) + goto bad; + switch (kind) { + case CHUNK_AUTO: if (!(fwd->caps & FWD_CAP_SCAN)) { die( "'auto' port forwarding is only allowed for pasta"); } - flags |= FWD_SCAN; - continue; - } - - /* Should be an exclude range */ - if (!parse_literal(&p, "~")) - goto bad; - - if (!parse_port_range(&p, &xrange)) - goto bad; - if (p != ep) /* Garbage after the range */ - goto bad; - - for (i = xrange.first; i <= xrange.last; i++) - bitmap_set(exclude, i); - } - - /* Now process base ranges, skipping exclusions */ - for_each_chunk(p, ep, spec, ",") { - struct port_range orig_range, mapped_range; - - /* Handle "all" like exclude only */ - if (parse_literal(&p, "all")) { - if (p != ep) /* Garbage after the keyword */ - goto bad; + break; - continue; + case CHUNK_EXCLUDE: + for (i = lrange.first; i <= lrange.last; i++) + bitmap_set(exclude, i); + break; + default: + ; /* Handled later */ } + } while (parse_literal(&p, ",")); - if (!isdigit(*p)) - /* Already parsed */ - continue; + /* Consider included ranges in next pass */ + p = spec; + do { + struct port_range trange; - if (!parse_port_range(&p, &orig_range)) + if (!parse_port_chunk(&p, &kind, &lrange, &trange)) goto bad; - exclude_only = false; + switch (kind) { + case CHUNK_AUTO: /* already handled */ + case CHUNK_EXCLUDE: /* already handled */ + case CHUNK_ALL: /* handled later */ + continue; - if (parse_literal(&p, ":")) { - /* There's a range to map to as well */ - if (!parse_port_range(&p, &mapped_range)) + case CHUNK_INCLUDE: + exclude_only = false; + if (trange.last - trange.first != + lrange.last - lrange.first) goto bad; - if ((mapped_range.last - mapped_range.first) != - (orig_range.last - orig_range.first)) - goto bad; - } else { - mapped_range = orig_range; - } - if (p != ep) /* Garbage after the ranges */ + fwd_rule_range_except(fwd, del, proto, addr, ifname, + lrange.first, lrange.last, + exclude, trange.first, flags); + break; + default: goto bad; + } + } while (parse_literal(&p, ",")); - fwd_rule_range_except(fwd, del, proto, addr, ifname, - orig_range.first, orig_range.last, - exclude, - mapped_range.first, flags); - } + if (!parse_eoi(p)) + goto bad; /* trailing garbage */ - /* Finally handle "all" and exclude only specs */ + /* Finally handle "all" and exclude only cases */ if (exclude_only) { fwd_port_map_ephemeral(exclude); @@ -569,10 +637,11 @@ bad: void fwd_rule_parse(char optname, bool del, const char *optarg, struct fwd_table *fwd) { - char buf[BUFSIZ], *spec, *ifname = NULL; - union inany_addr addr_buf = inany_any6; - const union inany_addr *addr = &addr_buf; + const union inany_addr *addr; + union inany_addr addr_buf; + char ifname[IFNAMSIZ]; uint8_t proto; + const char *p; if (optname == 't' || optname == 'T') proto = IPPROTO_TCP; @@ -581,7 +650,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, else assert(0); - if (!strcmp(optarg, "none")) { + if (p = optarg, + parse_literal(&p, "none") && parse_eoi(p)) { unsigned i; for (i = 0; i < fwd->count; i++) { @@ -593,45 +663,21 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, return; } - strncpy(buf, optarg, sizeof(buf) - 1); - - if ((spec = strchr(buf, '/'))) { - const char *p = buf; - - *spec = 0; - spec++; - - if (optname != 't' && optname != 'u') + if (p = optarg, + parse_addrifname(&p, &addr, &addr_buf, ifname)) { + if (optname == 'T' || optname == 'U') die("Listening address not allowed for -%c %s", optname, optarg); - - if ((ifname = strchr(buf, '%'))) { - *ifname = 0; - ifname++; - - /* spec is already advanced one past the '/', - * so the length of the given ifname is: - * (spec - ifname - 1) - */ - if (spec - ifname - 1 >= IFNAMSIZ) { - die("Interface name '%s' is too long (max %u)", - ifname, IFNAMSIZ - 1); - } - } - - if (ifname == buf + 1 || /* Interface without address */ - !strcmp(buf, "*")) /* Explicit wildcard address */ - addr = NULL; - else if (!parse_inany(&p, &addr_buf) && parse_eoi(p)) - die("Bad forwarding address '%s'", buf); + if (!strcmp(ifname, ".") || !strcmp(ifname, "..")) + die("Invalid interface name: %s", ifname); } else { - spec = buf; - + /* No address or ifname */ addr = NULL; + ifname[0] = '\0'; } if (optname == 'T' || optname == 'U') { - assert(!addr && !ifname); + assert(!addr && !*ifname); if (!(fwd->caps & FWD_CAP_IFNAME)) { warn( @@ -640,18 +686,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, if (fwd->caps & FWD_CAP_IPV4) { fwd_rule_parse_ports(fwd, del, proto, - &inany_loopback4, NULL, - spec); + &inany_loopback4, NULL, p); } if (fwd->caps & FWD_CAP_IPV6) { fwd_rule_parse_ports(fwd, del, proto, - &inany_loopback6, NULL, - spec); + &inany_loopback6, NULL, p); } return; } - ifname = "lo"; + static_assert(sizeof("lo") <= sizeof(ifname), + "ifname buffer too small"); + strcpy(ifname, "lo"); } /* No need for dual stack if we only have one IP version */ @@ -660,13 +706,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg, else if (!addr && !(fwd->caps & FWD_CAP_IPV6)) addr = &inany_any4; - if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) { + if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) { die( "Device binding for '-%c %s' unsupported (requires kernel 5.7+)", optname, optarg); } - fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec); + fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p); } /** diff --git a/parse.c b/parse.c index 24178873..44e90295 100644 --- a/parse.c +++ b/parse.c @@ -18,6 +18,7 @@ #include #include #include +#include #include "common.h" #include "parse.h" @@ -213,3 +214,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr, return false; } + +/** + * parse_ifspec() - Parse a interface name specifier (starting with %) + * @ifname: On success updated with parsed name (must have IFNAMSIZ space) + * + * This will accept a missing specifier (empty string), setting ifname to "" + */ +bool parse_ifspec(const char **cursor, char *ifname) +{ + const char *p = *cursor; + size_t len; + + if (!parse_literal(&p, "%")) { + /* No interface specifier */ + ifname[0] = '\0'; + return true; + } + + /* ifnames can have anything that's not '/', or whitespace */ + len = strcspn(p, "/ \f\n\r\t\v"); + if (!len || len >= IFNAMSIZ) + return false; + + memcpy(ifname, p, len); + ifname[len] = '\0'; + *cursor = p + len; + return true; +} diff --git a/parse.h b/parse.h index ab1d5adb..257b9c58 100644 --- a/parse.h +++ b/parse.h @@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr, #define parse_inany(cursor, addr) parse_inany_((cursor), (addr), NULL) +bool parse_ifspec(const char **cursor, char *ifname); + #endif /* _PARSE_H */ -- 2.55.0

On Thu, Jul 02, 2026 at 09:14:17AM +0200, Stefano Brivio wrote:

On Thu, 2 Jul 2026 16:31:42 +1000 David Gibson wrote:

...

Currently we handle -t all and the like as a special case, it can't be combined with other port specifier options. Remove that restriction, allowing combined options like: -t all,~9999 # Forward everything non-ephemeral except 9999 -t all,auto # Equivalent to -t auto -t all,33000 # Forward non-ephemeral plus port 33,000

This isn't particularly useful immediately, but will become important for destination address specification - it provides a place to attach the target address for "all" or exclude only mappings. It will also work better with some parsing reworks we want to make.

Signed-off-by: David Gibson --- conf.c | 11 +++++------ fwd_rule.c | 39 ++++++++++++++++++++------------------- passt.1 | 33 ++++++++++++++++----------------- 3 files changed, 41 insertions(+), 42 deletions(-)

diff --git a/conf.c b/conf.c index c4a36dee..a610c0c6 100644 --- a/conf.c +++ b/conf.c @@ -660,11 +660,9 @@ static void usage(const char *name, FILE *f, int status) " SPEC can be:\n" " 'none': don't forward any ports\n" " [ADDR[%%IFACE]/]PORTS: forward specific ports\n" - " PORTS is either 'all' (forward all unbound, non-ephemeral\n" - " ports), or a comma-separated list of ports, optionally\n" - " ranged with '-' and optional target ports after ':'.\n" - " Ranges can be reduced by excluding ports or ranges\n" - " prefixed by '~'.\n" + " PORTS is comma-separated list of ports, either\n"

I didn't really consider this change as worth updating usage and man page (the previous version wouldn't be entirely accurate anymore but practically speaking rather clear, I thought).

I think we should update the manpage, since it is pretty detailed (and now wrong). usage() I'll grant is borderline at best.

If it is:

- PORTS is _a_ comma-separated ...

Fixed.

- I think we should maintain the description for 'all' (forward all unbound, non-ephemeral ports), because otherwise just "Forward all ports" below becomes particularly misleading

Good point. I've reworded again, I think it's better.

...

+ " 'all', a port number or range. Ranges can be reduced\n" + " by excluding ports or ranges prefixed by '~'.\n" "%s" " Examples:\n" " -t all Forward all ports\n" @@ -677,7 +675,8 @@ static void usage(const char *name, FILE *f, int status) " corresponding port numbers plus 10\n" " -t 192.0.2.1/5 Bind port 5 of 192.0.2.1 to %s\n" " -t 5-25,~10-20 Forward ports 5 to 9, and 21 to 25\n" - " -t ~25 Forward all ports except for 25\n" + " -t ~25,all\n" + " -t 25 Forward all ports except for 25\n"

I think the previous version makes more sense. This isn't an exhaustive description, it just shows how to quickly do things. This is missing a ~ by the way.

Oops, yes. I dropped these lines.

...

"%s" " default: %s\n" " -u, --udp-ports SPEC UDP port forwarding to %s\n" diff --git a/fwd_rule.c b/fwd_rule.c index 6d7ec2c5..b14df340 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -471,20 +471,13 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, uint8_t flags = 0; unsigned i;

- if (!strcmp(spec, "all")) { - /* Treat "all" as equivalent to "": all non-ephemeral ports */ - spec = ""; - } - /* Parse excluded ranges and "auto" in the first pass */ for_each_chunk(p, ep, spec, ",") { struct port_range xrange;

- if (isdigit(*p)) { - /* Include range, parse later */ - exclude_only = false; + /* Include range, parse later */ + if (parse_literal(&p, "all") || isdigit(*p)) continue; - }

if (parse_literal(&p, "auto")) { if (p != ep) /* Garbage after the keyword */ @@ -512,20 +505,18 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, bitmap_set(exclude, i); }

- if (exclude_only) { - /* Exclude ephemeral ports */ - fwd_port_map_ephemeral(exclude); - - fwd_rule_range_except(fwd, del, proto, addr, ifname, - 1, NUM_PORTS - 1, exclude, - 1, flags | FWD_WEAK); - return; - } - /* Now process base ranges, skipping exclusions */ for_each_chunk(p, ep, spec, ",") { struct port_range orig_range, mapped_range;

+ /* Handle "all" like exclude only */ + if (parse_literal(&p, "all")) { + if (p != ep) /* Garbage after the keyword */ + goto bad; + + continue; + } + if (!isdigit(*p)) /* Already parsed */ continue; @@ -533,6 +524,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, if (!parse_port_range(&p, &orig_range)) goto bad;

+ exclude_only = false; + if (parse_literal(&p, ":")) { /* There's a range to map to as well */ if (!parse_port_range(&p, &mapped_range)) @@ -553,6 +546,14 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, mapped_range.first, flags); }

+ /* Finally handle "all" and exclude only specs */ + if (exclude_only) { + fwd_port_map_ephemeral(exclude); + + fwd_rule_range_except(fwd, del, proto, addr, ifname, + 1, NUM_PORTS - 1, exclude, + 1, flags | FWD_WEAK); + } return; bad: die("Invalid port specifier '%s'", spec); diff --git a/passt.1 b/passt.1 index 908fd4a4..c3722ef9 100644 --- a/passt.1 +++ b/passt.1 @@ -432,29 +432,22 @@ Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39.

.TP .BR \-t ", " \-\-tcp-ports " " \fIspec -Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of: +Configure TCP port forwarding to guest or namespace. \fIspec\fR can be either: .RS

.TP .BR none Don't forward any ports

+or .TP [\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ... -Specific ports to forward. Optionally, a specific listening address -and interface name (since Linux 5.7) can be specified. \fIports\fR -may be either: -.RS -.TP -\fBall\fR -Forward all unbound, non-ephemeral ports, as permitted by current -capabilities. For low (< 1024) ports, see \fBNOTES\fR. No failures -are reported for unavailable ports, unless no ports could be forwarded -at all. + +Ports to forward. Optionally, a specific listening address and +interface name (since Linux 5.7) can be specified. .RE

-.RS -or a comma-separated list of entries which may be any of: +\fIports\fR is a comma-separated list of entries which may be any of: .TP \fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]] Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR @@ -468,6 +461,13 @@ as \fIfirst\fR. Exclude range. Don't forward port numbers between \fIfirst\fR and \fIlast\fR. This takes precedences over include ranges.

+.TP +.BR all +Forward all unbound, non-ephemeral ports, not covered by exclude +ranges above, as permitted by current capabilities. For low (< 1024) +ports, see \fBNOTES\fR. No failures are reported for unavailable +ports, unless no ports could be forwarded at all. + .TP .BR auto \fBpasta\fR only. Only forward ports in the specified set if the @@ -477,10 +477,9 @@ periodically derived (every second) from listening sockets reported by .RE

Specifying excluded ranges only implies that all other non-ephemeral -ports are forwarded. Specifying no ranges at all implies forwarding -all non-ephemeral ports permitted by current capabilities. In this -case, no failures are reported for unavailable ports, unless no ports -could be forwarded at all. +ports are forwarded. Specifying no ranges is equivalent +to '\fBall\fR'. In this case, no failures are reported for +unavailable ports, unless no ports could be forwarded at all.

Nit: this could use a few more columns (I think it's slightly more readable as source), say:

Huh, odd. For some reason emacs M-q was making it narrower. Fixed manually.

ports are forwarded. Specifying no ranges is equivalent to '\fBall\fR'. In this case, no failures are reported for unavailable ports, unless no ports could be forwarded at all.

-- Stefano

-- David Gibson (he or they) | 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