| Message ID | 20240621161656.63576-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.19,v2] tools/xl: Open xldevd.log with O_CLOEXEC | expand |
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 >
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,
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
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>
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
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
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); >
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
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);
`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(-)