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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.06.2024 18:59, Andrew Cooper wrote:
> 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@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>
>>> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>
>>> 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.
>>
>> Acked-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> 
> Thanks.  I did originally have that ifdefary here, but then I noticed
> that this isn't the first instance like this in xl, and I'm going to be
> using that as a justification soon to explicitly drop support for Linux
> < 2.6.23.

Just to mention that this is a two fold thing: I surely don't try to run
up-to-date Xen on top of this old a Linux kernel, but what is used for
building is still what the distro (with a very old kernel) would have put
there.

Jan