Re: [PATCH] passt, util: Close any open file that the parent might have leaked

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 7 Aug 2024 11:44:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Aug 06, 2024 at 08:38:37PM +0200, Stefano Brivio wrote:
> > 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@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@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.

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.

> 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