Re: [PATCH] tools/xl: Open xldevd.log with O_CLOEXEC

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, May 07, 2024 at 01:32:00PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, May 07, 2024 at 12:08:06PM +0100, Andrew Cooper wrote:
> > `xl devd` has been observed leaking /var/log/xldevd.log into children.
> > 
> > Link: https://github.com/QubesOS/qubes-issues/issues/8292
> > Reported-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> > CC: Anthony PERARD <anthony@xxxxxxxxxxxxxx>
> > CC: Juergen Gross <jgross@xxxxxxxx>
> > CC: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > 
> > Also entirely speculative based on the QubesOS ticket.
> > ---
> >  tools/xl/xl_utils.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c
> > index 17489d182954..060186db3a59 100644
> > --- a/tools/xl/xl_utils.c
> > +++ b/tools/xl/xl_utils.c
> > @@ -270,7 +270,7 @@ int do_daemonize(const char *name, const char *pidfile)
> >          exit(-1);
> >      }
> >  
> > -    CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644));
> > +    CHK_SYSCALL(logfile = open(fullname, O_WRONLY | O_CREAT | O_APPEND | 
> > O_CLOEXEC, 0644));
> 
> This one might be not enough, as the FD gets dup2()-ed to stdout/stderr
> just outside of the context here, and then inherited by various hotplug
> script. Just adding O_CLOEXEC here means the hotplug scripts will run
> with stdout/stderr closed. The scripts shipped with Xen do redirect
> stderr to a log quite early, but a) it doesn't do it for stdout, and b)
> custom hotplug scripts are a valid use case.
> Without that, I see at least few potential issues:
> - some log messages may be lost (minor, but annoying)
> - something might simply fail on writing to a closed FD, breaking the
>   hotplug script
> - FD 1 will be used as first free FD for any open() or similar call - if
>   a tool later tries writing something to stdout, it will gets written
>   to that FD - worse of all three

Wait, the above is wrong, dup does not copy the O_CLOEXEC flag over to
the new FD. So, maybe your patch is correct after all.

> What should be the behavior of hotplug scripts logging? Should they
> always take care of their own logging? If so, the hotplug calling part
> should redirect stdout/stderr to /dev/null IMO. But if `xl` should
> provide some default logging for them (like, the xldevd.log here?), then
> the O_CLOEXEC should be set only after duplicating logfile over stdout/err.
> 
> >      free(fullname);
> >      assert(logfile >= 3);
> >  
> > 
> > base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81
> > prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919
> 
> Which one is this? I don't see it in staging, nor in any of your
> branches on xenbits. Lore finds "tools/libxs: Open /dev/xen/xenbus fds
> as O_CLOEXEC" which I guess is correct, but I have no idea how it
> correlates it, as this hash doesn't appear anywhere in the message, nor
> its headers...
> 
> -- 
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab



-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature