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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/06/2024 5:55 pm, Anthony PERARD wrote:
> 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));
> Everytime we use O_CLOEXEC, we add in the C file
>     #ifndef O_CLOEXEC
>     #define O_CLOEXEC 0
>     #endif
> we don't need to do that anymore?
> Or I guess we'll see if someone complain when they try to build on an
> ancien version of Linux.

I double checked, and it turns out it was an FD_CLOEXEC, not an
O_CLOEXEC which xl was using, so we don't have a pre-existing example.

Therefore I've re-added this at the top of xl_utils.c

~Andrew