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

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

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

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

Attachment: signature.asc
Description: PGP signature