On Wed, 7 Aug 2024 11:44:01 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Tue, Aug 06, 2024 at 08:38:37PM +0200, Stefano Brivio wrote:I also thought of doing that, for the simple fact that isolate_initial() is called just after this, and reducing clutter didn't sound like a valid reason to move this to a place it doesn't really belong to (it's not about "isolation"). On the other hand, reading the isolate_*() functions again, we're assigning a rather broad scope to "isolation"... so it could actually fit.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);Any reason not to fold this logic into isolate_initial()? Seems like it is part of self-isolation handling.Also, I think this could wait until after the existing isolate_initial() logic. Dropping caps before examining the command line seems like a sensible precaution.Oh, right, we don't drop much there but at least we make sure we don't have CAP_DAC_OVERRIDE and friends. I'll move this to the end of isolate_initial() then. -- Stefano