Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

On Fri, Jun 21, 2024 at 05:16:56PM +0100, Andrew Cooper wrote:
> `xl devd` has been observed leaking /var/log/xldevd.log into children.
> 
> Note this is specifically safe; dup2() leaves O_CLOEXEC disabled on newfd, so
> after setting up stdout/stderr, it's only the logfile fd which will close on
> exec().
> 
> Link: https://github.com/QubesOS/qubes-issues/issues/8292
> Reported-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Anthony PERARD <anthony@xenproject.org>
> CC: Juergen Gross <jgross@suse.com>
> CC: Demi Marie Obenour <demi@invisiblethingslab.com>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> 
> Also entirely speculative based on the QubesOS ticket.
> 
> v2:
>  * Extend the commit message to explain why stdout/stderr aren't closed by
>    this change
> 
> For 4.19.  This bugfix was posted earlier, but fell between the cracks.
> ---
>  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));
>      free(fullname);
>      assert(logfile >= 3);

Definitely an improvement.  I would also add O_NOCTTY to work around a
particularly unfortunate Linux kernel design decision, but that can
either be fixed up on commit or be a separate patch.

Reviewed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]