- dev - passt

[PATCH v2] conf: Stop parsing options at first non-option argument
by Stefano Brivio 08 Aug '24

08 Aug '24

Given that pasta supports specifying a command to be executed on the command line, even without the usual -- separator as long as there's no ambiguity, we shouldn't eat up options that are not meant for us. Paul reports, for instance, that with: pasta --config-net ip -6 route -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. That's because getopt_long(), by default, shuffles the argument list to shift non-option arguments at the end. Avoid that by adding '-' at the beginning of 'optstring', and mark the position of the first non-option argument (getopt_long() will now return the character code 1 once we hit it), so that we can use that as command to run, or as PID for the target namespace. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Instead of overriding 'name' in the getopt_long() loop, to force exiting the loop, adjust the exit condition conf.c | 23 +++++++++++++++-------- util.c | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/conf.c b/conf.c index be5259e..1a30b47 100644 --- a/conf.c +++ b/conf.c @@ -1240,22 +1240,22 @@ void conf(struct ctx *c, int argc, char **argv) enum fwd_ports_mode fwd_default = FWD_NONE; bool v4_only = false, v6_only = false; struct fqdn *dnss = c->dns_search; + int name, ret, first_nonopt = -1; unsigned int ifi4 = 0, ifi6 = 0; const char *logfile = NULL; const char *optstring; size_t logsize = 0; char *runas = NULL; long fd_tap_opt; - int name, ret; uid_t uid; gid_t gid; if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "-dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; } else { - optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "-dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; @@ -1269,6 +1269,9 @@ void conf(struct ctx *c, int argc, char **argv) case -1: case 0: break; + case 1: + first_nonopt = optind - 1; + break; case 2: if (c->mode != MODE_PASTA) die("--userns is for pasta mode only"); @@ -1629,7 +1632,10 @@ void conf(struct ctx *c, int argc, char **argv) usage(argv[0], stderr, EXIT_FAILURE); break; } - } while (name != -1); + } while (name != -1 && name != 1); + + if (first_nonopt == -1) + first_nonopt = optind; if (c->mode == MODE_PASTA && !c->pasta_conf_ns) { if (copy_routes_opt) @@ -1689,9 +1695,9 @@ void conf(struct ctx *c, int argc, char **argv) } while (name != -1); if (c->mode == MODE_PASTA) - conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); - else if (optind != argc) - die("Extra non-option argument: %s", argv[optind]); + conf_pasta_ns(&netns_only, userns, netns, first_nonopt, argc, argv); + else if (first_nonopt != argc) + die("Extra non-option argument: %s", argv[first_nonopt]); conf_open_files(c); /* Before any possible setuid() / setgid() */ @@ -1705,7 +1711,8 @@ void conf(struct ctx *c, int argc, char **argv) pasta_open_ns(c, netns); } else { pasta_start_ns(c, uid, gid, - argc - optind, argv + optind); + argc - first_nonopt, + argv + first_nonopt); } } diff --git a/util.c b/util.c index 9c6be6a..13524e6 100644 --- a/util.c +++ b/util.c @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "-:F", optfd, NULL); if (name == 'F') { errno = 0; -- 2.43.0

2 3

[PATCH v6] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 08 Aug '24

08 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v6: (seriously?) fix STDERR_FILENO comparison in conf() v5: Reject any --fd matching standard streams v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..2f5d649 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt <= STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..fe9f436 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

2 1

[PATCH v5] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v5: Reject any --fd matching standard streams v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..b4ea7a5 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt < STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..fe9f436 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

1 0

[PATCH v3] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 3 ++- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 38 ++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 52 insertions(+), 6 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..5422813 100644 --- a/conf.c +++ b/conf.c @@ -1260,6 +1260,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1426,7 +1427,7 @@ void conf(struct ctx *c, int argc, char **argv) errno = 0; c->fd_tap = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (c->fd_tap < 0 || errno || c->fd_tap > INT_MAX) die("Invalid --fd: %s", optarg); c->one_off = true; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..9c6be6a 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,40 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (fd < 0 || errno || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1 || fd == 3) { + unsigned int first = (fd == 3) ? 4 : 3; + + if (close_range(first, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } else { + if (close_range(3, fd - 1, CLOSE_RANGE_UNSHARE) || + close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

2 2

[PATCH v4] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 7 +++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 38 ++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 55 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..be5259e 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,12 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (fd_tap_opt < 0 || errno || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..9c6be6a 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,40 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (fd < 0 || errno || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1 || fd == 3) { + unsigned int first = (fd == 3) ? 4 : 3; + + if (close_range(first, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } else { + if (close_range(3, fd - 1, CLOSE_RANGE_UNSHARE) || + close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

1 0

[PATCH] conf: Stop parsing options at first non-option argument
by Stefano Brivio 07 Aug '24

07 Aug '24

Given that pasta supports specifying a command to be executed on the command line, even without the usual -- separator as long as there's no ambiguity, we shouldn't eat up options that are not meant for us. Paul reports, for instance, that with: pasta --config-net ip -6 route -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. That's because getopt_long(), by default, shuffles the argument list to shift non-option arguments at the end. Avoid that by adding '-' at the beginning of 'optstring', and mark the position of the first non-option argument (getopt_long() will now return the character code 1 once we hit it), so that we can use that as command to run, or as PID for the target namespace. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 22 +++++++++++++++------- util.c | 2 +- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/conf.c b/conf.c index 5422813..a3e15f0 100644 --- a/conf.c +++ b/conf.c @@ -1240,21 +1240,21 @@ void conf(struct ctx *c, int argc, char **argv) enum fwd_ports_mode fwd_default = FWD_NONE; bool v4_only = false, v6_only = false; struct fqdn *dnss = c->dns_search; + int name, ret, first_nonopt = -1; unsigned int ifi4 = 0, ifi6 = 0; const char *logfile = NULL; const char *optstring; size_t logsize = 0; char *runas = NULL; - int name, ret; uid_t uid; gid_t gid; if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "-dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; } else { - optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "-dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; @@ -1268,6 +1268,10 @@ void conf(struct ctx *c, int argc, char **argv) case -1: case 0: break; + case 1: + first_nonopt = optind - 1; + name = -1; + break; case 2: if (c->mode != MODE_PASTA) die("--userns is for pasta mode only"); @@ -1629,6 +1633,9 @@ void conf(struct ctx *c, int argc, char **argv) } } while (name != -1); + if (first_nonopt == -1) + first_nonopt = optind; + if (c->mode == MODE_PASTA && !c->pasta_conf_ns) { if (copy_routes_opt) die("--no-copy-routes needs --config-net"); @@ -1687,9 +1694,9 @@ void conf(struct ctx *c, int argc, char **argv) } while (name != -1); if (c->mode == MODE_PASTA) - conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); - else if (optind != argc) - die("Extra non-option argument: %s", argv[optind]); + conf_pasta_ns(&netns_only, userns, netns, first_nonopt, argc, argv); + else if (first_nonopt != argc) + die("Extra non-option argument: %s", argv[first_nonopt]); conf_open_files(c); /* Before any possible setuid() / setgid() */ @@ -1703,7 +1710,8 @@ void conf(struct ctx *c, int argc, char **argv) pasta_open_ns(c, netns); } else { pasta_start_ns(c, uid, gid, - argc - optind, argv + optind); + argc - first_nonopt, + argv + first_nonopt); } } diff --git a/util.c b/util.c index 9c6be6a..13524e6 100644 --- a/util.c +++ b/util.c @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "-:F", optfd, NULL); if (name == 'F') { errno = 0; -- 2.43.0

1 0

[PATCH v2] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Move call to close_open_files() to isolate_initial() conf.c | 1 + isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 36 ++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..89f5b3d 100644 --- a/conf.c +++ b/conf.c @@ -1260,6 +1260,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..bd65b5a 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,38 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (fd < 0 || errno) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + if (close_range(3, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } else { + if (close_range(3, fd - 1, CLOSE_RANGE_UNSHARE) || + close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

3 3

[PATCH] conf, pasta: Make -g and -a skip route/addresses copy for matching IP version only
by Stefano Brivio 07 Aug '24

07 Aug '24

Paul reports that setting IPv4 address and gateway manually, using --address and --gateway, causes pasta to fail inserting IPv6 routes in a setup where multiple, inter-dependent IPv6 routes are present on the host. That's because, currently, any -g option implies --no-copy-routes altogether, and any -a implies --no-copy-addrs. Limit this implication to the matching IP version, instead, by having two copies of no_copy_routes and no_copy_addrs in the context structure, separately for IPv4 and IPv6. While at it, change them to 'bool': we had them as 'int' because getopt_long() used to set them directly, but it hasn't been the case for a while already. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 32 ++++++++++++++++++++------------ passt.1 | 4 ++-- passt.h | 14 ++++++++++---- pasta.c | 8 ++++---- 4 files changed, 36 insertions(+), 22 deletions(-) diff --git a/conf.c b/conf.c index 46fcd91..14d8ece 100644 --- a/conf.c +++ b/conf.c @@ -1379,14 +1379,16 @@ void conf(struct ctx *c, int argc, char **argv) die("--no-copy-routes is for pasta mode only"); warn("--no-copy-routes will be dropped soon"); - c->no_copy_routes = copy_routes_opt = true; + c->ip4.no_copy_routes = c->ip6.no_copy_routes = true; + copy_routes_opt = true; break; case 19: if (c->mode != MODE_PASTA) die("--no-copy-addrs is for pasta mode only"); warn("--no-copy-addrs will be dropped soon"); - c->no_copy_addrs = copy_addrs_opt = true; + c->ip4.no_copy_addrs = c->ip6.no_copy_addrs = true; + copy_addrs_opt = true; break; case 20: if (c->mode != MODE_PASTA) @@ -1465,23 +1467,26 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'a': - if (c->mode == MODE_PASTA) - c->no_copy_addrs = 1; - if (inet_pton(AF_INET6, optarg, &c->ip6.addr) && !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr) && !IN6_IS_ADDR_LOOPBACK(&c->ip6.addr) && !IN6_IS_ADDR_V4MAPPED(&c->ip6.addr) && !IN6_IS_ADDR_V4COMPAT(&c->ip6.addr) && - !IN6_IS_ADDR_MULTICAST(&c->ip6.addr)) + !IN6_IS_ADDR_MULTICAST(&c->ip6.addr)) { + if (c->mode == MODE_PASTA) + c->ip6.no_copy_addrs = true; break; + } if (inet_pton(AF_INET, optarg, &c->ip4.addr) && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr) && !IN4_IS_ADDR_BROADCAST(&c->ip4.addr) && !IN4_IS_ADDR_LOOPBACK(&c->ip4.addr) && - !IN4_IS_ADDR_MULTICAST(&c->ip4.addr)) + !IN4_IS_ADDR_MULTICAST(&c->ip4.addr)) { + if (c->mode == MODE_PASTA) + c->ip4.no_copy_addrs = true; break; + } die("Invalid address: %s", optarg); break; @@ -1495,19 +1500,22 @@ void conf(struct ctx *c, int argc, char **argv) parse_mac(c->mac, optarg); break; case 'g': - if (c->mode == MODE_PASTA) - c->no_copy_routes = 1; - if (inet_pton(AF_INET6, optarg, &c->ip6.gw) && !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw) && - !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw)) + !IN6_IS_ADDR_LOOPBACK(&c->ip6.gw)) { + if (c->mode == MODE_PASTA) + c->ip6.no_copy_routes = true; break; + } if (inet_pton(AF_INET, optarg, &c->ip4.gw) && !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw) && !IN4_IS_ADDR_BROADCAST(&c->ip4.gw) && - !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw)) + !IN4_IS_ADDR_LOOPBACK(&c->ip4.gw)) { + if (c->mode == MODE_PASTA) + c->ip4.no_copy_routes = true; break; + } die("Invalid gateway address: %s", optarg); break; diff --git a/passt.1 b/passt.1 index 81789cc..3062b71 100644 --- a/passt.1 +++ b/passt.1 @@ -589,7 +589,7 @@ or sourced from the host, and bring up the tap interface. .BR \-\-no-copy-routes " " (DEPRECATED) With \-\-config-net, do not copy all the routes associated to the interface we derive addresses and routes from: set up only the default gateway. Implied by --g, \-\-gateway. +-g, \-\-gateway, for the corresponding IP version only. Default is to copy all the routing entries from the interface in the outer namespace to the target namespace, translating the output interface attribute to @@ -604,7 +604,7 @@ below. .BR \-\-no-copy-addrs " " (DEPRECATED) With \-\-config-net, do not copy all the addresses associated to the interface we derive addresses and routes from: set up a single one. Implied by \-a, -\-\-address. +\-\-address, for the corresponding IP version only. Default is to copy all the addresses, except for link-local ones, from the interface from the outer namespace to the target namespace. diff --git a/passt.h b/passt.h index d0f31a2..12ae1b9 100644 --- a/passt.h +++ b/passt.h @@ -100,6 +100,8 @@ enum passt_modes { * @dns_host: Use this DNS on the host for forwarding * @addr_out: Optional source address for outbound traffic * @ifname_out: Optional interface name to bind outbound sockets to + * @no_copy_routes: Don't copy all routes when configuring target namespace + * @no_copy_addrs: Don't copy all addresses when configuring namespace */ struct ip4_ctx { struct in_addr addr; @@ -112,6 +114,9 @@ struct ip4_ctx { struct in_addr addr_out; char ifname_out[IFNAMSIZ]; + + bool no_copy_routes; + bool no_copy_addrs; }; /** @@ -126,6 +131,8 @@ struct ip4_ctx { * @dns_host: Use this DNS on the host for forwarding * @addr_out: Optional source address for outbound traffic * @ifname_out: Optional interface name to bind outbound sockets to + * @no_copy_routes: Don't copy all routes when configuring target namespace + * @no_copy_addrs: Don't copy all addresses when configuring namespace */ struct ip6_ctx { struct in6_addr addr; @@ -139,6 +146,9 @@ struct ip6_ctx { struct in6_addr addr_out; char ifname_out[IFNAMSIZ]; + + bool no_copy_routes; + bool no_copy_addrs; }; #include <netinet/if_ether.h> @@ -173,8 +183,6 @@ struct ip6_ctx { * @pasta_ifn: Name of namespace interface for pasta * @pasta_ifi: Index of namespace interface for pasta * @pasta_conf_ns: Configure namespace after creating it - * @no_copy_routes: Don't copy all routes when configuring target namespace - * @no_copy_addrs: Don't copy all addresses when configuring namespace * @no_tcp: Disable TCP operation * @tcp: Context for TCP protocol handler * @no_tcp: Disable UDP operation @@ -233,8 +241,6 @@ struct ctx { char pasta_ifn[IF_NAMESIZE]; unsigned int pasta_ifi; int pasta_conf_ns; - int no_copy_routes; - int no_copy_addrs; int no_tcp; struct tcp_ctx tcp; diff --git a/pasta.c b/pasta.c index b4a3d99..615ff7b 100644 --- a/pasta.c +++ b/pasta.c @@ -306,7 +306,7 @@ void pasta_ns_conf(struct ctx *c) nl_link_up(nl_sock_ns, c->pasta_ifi, c->mtu); if (c->ifi4) { - if (c->no_copy_addrs) { + if (c->ip4.no_copy_addrs) { rc = nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET, &c->ip4.addr, @@ -322,7 +322,7 @@ void pasta_ns_conf(struct ctx *c) strerror(-rc)); } - if (c->no_copy_routes) { + if (c->ip4.no_copy_routes) { rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi, AF_INET, &c->ip4.gw); } else { @@ -337,7 +337,7 @@ void pasta_ns_conf(struct ctx *c) } if (c->ifi6) { - if (c->no_copy_addrs) { + if (c->ip6.no_copy_addrs) { rc = nl_addr_set(nl_sock_ns, c->pasta_ifi, AF_INET6, &c->ip6.addr, 64); } else { @@ -351,7 +351,7 @@ void pasta_ns_conf(struct ctx *c) strerror(-rc)); } - if (c->no_copy_routes) { + if (c->ip6.no_copy_routes) { rc = nl_route_set_def(nl_sock_ns, c->pasta_ifi, AF_INET6, &c->ip6.gw); } else { -- 2.43.0

3 2

[PATCH v2 0/4] Fix several small errors in log time handling
by David Gibson 07 Aug '24

07 Aug '24

While working on the deduplication of clock_gettime() in the logging code, I noticed a few other minor issues. Changes since v1: * Dropped merged change making logfile_write() static * Handle errors from clock_gettime() * Added fixes for some additional problems I noticed David Gibson (4): util: Some corrections for timespec_diff_us log: Correct formatting of timestamps log: Handle errors from clock_gettime() log: Avoid duplicate calls to logtime() log.c | 70 ++++++++++++++++++++++++++++++++++++++++------------------ util.c | 4 ++-- util.h | 2 +- 3 files changed, 52 insertions(+), 24 deletions(-) -- 2.45.2

2 7

[PATCH] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 1 + passt.c | 2 ++ util.c | 36 ++++++++++++++++++++++++++++++++++++ util.h | 1 + 4 files changed, 40 insertions(+) diff --git a/conf.c b/conf.c index 14d8ece..89f5b3d 100644 --- a/conf.c +++ b/conf.c @@ -1260,6 +1260,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); diff --git a/passt.c b/passt.c index ea5bece..be7e84a 100644 --- a/passt.c +++ b/passt.c @@ -211,6 +211,8 @@ int main(int argc, char **argv) arch_avx2_exec(argv); + close_open_files(argc, argv); + isolate_initial(); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 54a9f58..ca627c6 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,38 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (fd < 0 || errno) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + if (close_range(3, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } else { + if (close_range(3, fd - 1, CLOSE_RANGE_UNSHARE) || + close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE)) + die_perror("Failed to close files leaked by parent"); + } +} diff --git a/util.h b/util.h index e8bf957..ab11ee7 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

2 2