diff mbox series

tools/xl: Open xldevd.log with O_CLOEXEC

Message ID 20240507110806.1692135-1-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series tools/xl: Open xldevd.log with O_CLOEXEC | expand

Commit Message

Andrew Cooper May 7, 2024, 11:08 a.m. UTC 
`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@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>

Also entirely speculative based on the QubesOS ticket.
---
 tools/xl/xl_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


base-commit: ebab808eb1bb8f24c7d0dd41b956e48cb1824b81
prerequisite-patch-id: 212e50457e9b6bdfd06a97da545a5aa7155bb919

Comments

Marek Marczykowski-Górecki May 7, 2024, 11:32 a.m. UTC | #1 
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@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>
> 
> 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...
Andrew Cooper May 7, 2024, 2:15 p.m. UTC | #2 
On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote:
> 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@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>
>>
>> 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.

Lovely :(  Yes - this won't work.  I guess what we want instead is:

diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c
index 060186db3a59..a0ce7dd7fa21 100644
--- a/tools/xl/xl_utils.c
+++ b/tools/xl/xl_utils.c
@@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile)
     dup2(logfile, 2);
 
     close(nullfd);
+    close(logfile);
 
     CHK_SYSCALL(daemon(0, 1));
 
which at least means there's not a random extra fd attached to the logfile.
>>      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...

It's the libxs patch, but rebased over yesterday's push to staging. 
auto-base doesn't work quite so well in cases like this.

~Andrew
Marek Marczykowski-Górecki May 7, 2024, 2:23 p.m. UTC | #3 
On Tue, May 07, 2024 at 03:15:48PM +0100, Andrew Cooper wrote:
> On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote:
> > 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@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>
> >>
> >> 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.
> 
> Lovely :(  Yes - this won't work.  I guess what we want instead is:
> 
> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c
> index 060186db3a59..a0ce7dd7fa21 100644
> --- a/tools/xl/xl_utils.c
> +++ b/tools/xl/xl_utils.c
> @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile)
>      dup2(logfile, 2);
>  
>      close(nullfd);
> +    close(logfile);
>  
>      CHK_SYSCALL(daemon(0, 1));
>  
> which at least means there's not a random extra fd attached to the logfile.

But logfile is a global variable, and it looks to be used in dolog()...
Marek Marczykowski-Górecki May 7, 2024, 2:26 p.m. UTC | #4 
On Tue, May 07, 2024 at 01:32:00PM +0200, Marek Marczykowski-Górecki wrote:
> 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@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>
> > 
> > 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

Wait, the above is wrong, dup does not copy the O_CLOEXEC flag over to
the new FD. So, maybe your patch is correct after all.

> 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
Andrew Cooper May 7, 2024, 2:26 p.m. UTC | #5 
On 07/05/2024 3:23 pm, Marek Marczykowski-Górecki wrote:
> On Tue, May 07, 2024 at 03:15:48PM +0100, Andrew Cooper wrote:
>> On 07/05/2024 12:32 pm, Marek Marczykowski-Górecki wrote:
>>> 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@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>
>>>>
>>>> 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.
>> Lovely :(  Yes - this won't work.  I guess what we want instead is:
>>
>> diff --git a/tools/xl/xl_utils.c b/tools/xl/xl_utils.c
>> index 060186db3a59..a0ce7dd7fa21 100644
>> --- a/tools/xl/xl_utils.c
>> +++ b/tools/xl/xl_utils.c
>> @@ -282,6 +282,7 @@ int do_daemonize(const char *name, const char *pidfile)
>>      dup2(logfile, 2);
>>  
>>      close(nullfd);
>> +    close(logfile);
>>  
>>      CHK_SYSCALL(daemon(0, 1));
>>  
>> which at least means there's not a random extra fd attached to the logfile.
> But logfile is a global variable, and it looks to be used in dolog()...

Urgh, fine.  Lets go back to your suggestion of setting CLOEXEC after
dup()ing.

~Andrew
diff mbox series

Patch

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);