1. Patchew
  2. Xen
  3. View series

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

Andrew Cooper posted 1 patch 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240621161656.63576-1-andrew.cooper3@citrix.com
tools/xl/xl_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Andrew Cooper 5 months ago 
`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);
 
-- 
2.39.2
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Oleksii 5 months ago 
On Fri, 2024-06-21 at 17:16 +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>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii

> ---
> 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);
>  
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Demi Marie Obenour 5 months ago 
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
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Anthony PERARD 5 months ago 
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.

Acked-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Andrew Cooper 5 months ago 
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
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Jan Beulich 5 months ago 
On 21.06.2024 18:55, 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'm pretty certain I'll run into that issue on one of my pretty old systems,
but if the general view is that we don't care about such environments anymore,
then so be it (and I'll take care of such issues locally).

Jan
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Andrew Cooper 5 months ago 
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.
>
> Acked-by: Anthony PERARD <anthony.perard@vates.tech>

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.

~Andrew
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Jan Beulich 5 months ago 
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@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.
>>
>> Acked-by: Anthony PERARD <anthony.perard@vates.tech>
> 
> 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
Re: [PATCH for-4.19 v2] tools/xl: Open xldevd.log with O_CLOEXEC
Posted by Marek Marczykowski-Górecki 5 months ago 
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>

Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.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);
>  
> -- 
> 2.39.2
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.