[PATCH v2 0/8] softmmu: move and refactor -runas, -chroot and -daemonize

This small series was motivated by my thoughts on the proposals in https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg01135.html It demonstrates the approach I mention there, and has the further benefit of untangling and isolating the implementation of UID changing, chrooting and daemonizing, from the parsing of the corresponding command line options. Changed in v2: - Fix mistake that left stderr open when daemonizing - Remove use of is_daemonized from chardev - Remove use of is_daemonized from logging - Eliminate is_daemonized function Daniel P. Berrangé (8): softmmu: remove deprecated --enable-fips option os-posix: refactor code handling the -runas argument os-posix: refactor code handling the -chroot argument util: remove use of is_daemonized flag from logging code softmmu: refactor use of is_daemonized() method chardev: add API to block use of the stdio implementation softmmu: move parsing of -runas, -chroot and -daemonize code softmmu: remove is_daemonized() method chardev/char-stdio.c | 12 +- docs/about/deprecated.rst | 12 -- docs/about/removed-features.rst | 11 ++ include/chardev/char-stdio.h | 29 ++++ include/qemu/log.h | 1 + include/qemu/osdep.h | 3 - include/sysemu/os-posix.h | 6 +- include/sysemu/os-win32.h | 6 - os-posix.c | 227 ++++++++++---------------------- os-win32.c | 9 -- qemu-options.hx | 10 -- softmmu/vl.c | 92 +++++++++++-- stubs/is-daemonized.c | 9 -- stubs/meson.build | 1 - ui/vnc.c | 7 - util/log.c | 12 +- util/osdep.c | 28 ---- 17 files changed, 213 insertions(+), 262 deletions(-) create mode 100644 include/chardev/char-stdio.h delete mode 100644 stubs/is-daemonized.c -- 2.34.1

Users requiring FIPS support must build QEMU with either the libgcrypt or gnutls libraries as the crytography backend. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/about/deprecated.rst | 12 ------------ docs/about/removed-features.rst | 11 +++++++++++ include/qemu/osdep.h | 3 --- os-posix.c | 8 -------- qemu-options.hx | 10 ---------- ui/vnc.c | 7 ------- util/osdep.c | 28 ---------------------------- 7 files changed, 11 insertions(+), 68 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 26d00812ba..a458dd453c 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -67,18 +67,6 @@ and will cause a warning. The replacement for the ``nodelay`` short-form boolean option is ``nodelay=on`` rather than ``delay=off``. -``--enable-fips`` (since 6.0) -''''''''''''''''''''''''''''' - -This option restricts usage of certain cryptographic algorithms when -the host is operating in FIPS mode. - -If FIPS compliance is required, QEMU should be built with the ``libgcrypt`` -library enabled as a cryptography provider. - -Neither the ``nettle`` library, or the built-in cryptography provider are -supported on FIPS enabled hosts. - ``-writeconfig`` (since 6.0) ''''''''''''''''''''''''''''' diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index cb0575fd49..6ca66f658d 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -336,6 +336,17 @@ for the RISC-V ``virt`` machine and ``sifive_u`` machine. The ``-no-quit`` was a synonym for ``-display ...,window-close=off`` which should be used instead. +``--enable-fips`` (removed in 7.0) +'''''''''''''''''''''''''''''''''' + +This option restricted usage of certain cryptographic algorithms when +the host is operating in FIPS mode. + +If FIPS compliance is required, QEMU should be built with the ``libgcrypt`` +or ``gnutls`` library enabled as a cryptography provider. + +Neither the ``nettle`` library, or the built-in cryptography provider are +supported on FIPS enabled hosts. QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 7bcce3bceb..66e70e24ff 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -534,9 +534,6 @@ static inline void qemu_timersub(const struct timeval *val1, void qemu_set_cloexec(int fd); -void fips_set_state(bool requested); -bool fips_get_state(void); - /* Return a dynamically allocated pathname denoting a file or directory that is * appropriate for storing local state. * diff --git a/os-posix.c b/os-posix.c index ae6c9f2a5e..7cd662098e 100644 --- a/os-posix.c +++ b/os-posix.c @@ -151,14 +151,6 @@ int os_parse_cmd_args(int index, const char *optarg) case QEMU_OPTION_daemonize: daemonize = 1; break; -#if defined(CONFIG_LINUX) - case QEMU_OPTION_enablefips: - warn_report("-enable-fips is deprecated, please build QEMU with " - "the `libgcrypt` library as the cryptography provider " - "to enable FIPS compliance"); - fips_set_state(true); - break; -#endif default: return -1; } diff --git a/qemu-options.hx b/qemu-options.hx index 094a6c1d7c..cb0c58904b 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4655,16 +4655,6 @@ HXCOMM Internal use DEF("qtest", HAS_ARG, QEMU_OPTION_qtest, "", QEMU_ARCH_ALL) DEF("qtest-log", HAS_ARG, QEMU_OPTION_qtest_log, "", QEMU_ARCH_ALL) -#ifdef __linux__ -DEF("enable-fips", 0, QEMU_OPTION_enablefips, - "-enable-fips enable FIPS 140-2 compliance\n", - QEMU_ARCH_ALL) -#endif -SRST -``-enable-fips`` - Enable FIPS 140-2 compliance mode. -ERST - DEF("msg", HAS_ARG, QEMU_OPTION_msg, "-msg [timestamp[=on|off]][,guest-name=[on|off]]\n" " control error message format\n" diff --git a/ui/vnc.c b/ui/vnc.c index 3ccd33dedc..82b28aec95 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -4051,13 +4051,6 @@ void vnc_display_open(const char *id, Error **errp) password = qemu_opt_get_bool(opts, "password", false); } if (password) { - if (fips_get_state()) { - error_setg(errp, - "VNC password auth disabled due to FIPS mode, " - "consider using the VeNCrypt or SASL authentication " - "methods as an alternative"); - goto fail; - } if (!qcrypto_cipher_supports( QCRYPTO_CIPHER_ALG_DES, QCRYPTO_CIPHER_MODE_ECB)) { error_setg(errp, diff --git a/util/osdep.c b/util/osdep.c index 723cdcb004..456df9e81a 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -43,8 +43,6 @@ extern int madvise(char *, size_t, int); #include "qemu/hw-version.h" #include "monitor/monitor.h" -static bool fips_enabled = false; - static const char *hw_version = QEMU_HW_VERSION; int socket_set_cork(int fd, int v) @@ -526,32 +524,6 @@ const char *qemu_hw_version(void) return hw_version; } -void fips_set_state(bool requested) -{ -#ifdef __linux__ - if (requested) { - FILE *fds = fopen("/proc/sys/crypto/fips_enabled", "r"); - if (fds != NULL) { - fips_enabled = (fgetc(fds) == '1'); - fclose(fds); - } - } -#else - fips_enabled = false; -#endif /* __linux__ */ - -#ifdef _FIPS_DEBUG - fprintf(stderr, "FIPS mode %s (requested %s)\n", - (fips_enabled ? "enabled" : "disabled"), - (requested ? "enabled" : "disabled")); -#endif -} - -bool fips_get_state(void) -{ - return fips_enabled; -} - #ifdef _WIN32 static void socket_cleanup(void) { -- 2.34.1

Change the change_process_uid() function so that it takes its input as parameters instead of relying on static global variables. Reviewed-by: Eric Blake <eblake@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- os-posix.c | 83 +++++++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 44 deletions(-) diff --git a/os-posix.c b/os-posix.c index 7cd662098e..5a127feee2 100644 --- a/os-posix.c +++ b/os-posix.c @@ -42,13 +42,9 @@ #include <sys/prctl.h> #endif -/* - * Must set all three of these at once. - * Legal combinations are unset by name by uid - */ -static struct passwd *user_pwd; /* NULL non-NULL NULL */ -static uid_t user_uid = (uid_t)-1; /* -1 -1 >=0 */ -static gid_t user_gid = (gid_t)-1; /* -1 -1 >=0 */ +static char *user_name; +static uid_t user_uid = (uid_t)-1; +static gid_t user_gid = (gid_t)-1; static const char *chroot_dir; static int daemonize; @@ -100,7 +96,8 @@ void os_set_proc_name(const char *s) } -static bool os_parse_runas_uid_gid(const char *optarg) +static bool os_parse_runas_uid_gid(const char *optarg, + uid_t *runas_uid, gid_t *runas_gid) { unsigned long lv; const char *ep; @@ -120,9 +117,8 @@ static bool os_parse_runas_uid_gid(const char *optarg) return false; } - user_pwd = NULL; - user_uid = got_uid; - user_gid = got_gid; + *runas_uid = got_uid; + *runas_gid = got_gid; return true; } @@ -132,13 +128,18 @@ static bool os_parse_runas_uid_gid(const char *optarg) */ int os_parse_cmd_args(int index, const char *optarg) { + struct passwd *user_pwd; + switch (index) { case QEMU_OPTION_runas: user_pwd = getpwnam(optarg); if (user_pwd) { - user_uid = -1; - user_gid = -1; - } else if (!os_parse_runas_uid_gid(optarg)) { + user_uid = user_pwd->pw_uid; + user_gid = user_pwd->pw_gid; + user_name = g_strdup(user_pwd->pw_name); + } else if (!os_parse_runas_uid_gid(optarg, + &user_uid, + &user_gid)) { error_report("User \"%s\" doesn't exist" " (and is not <uid>:<gid>)", optarg); @@ -158,41 +159,33 @@ int os_parse_cmd_args(int index, const char *optarg) return 0; } -static void change_process_uid(void) +static void change_process_uid(uid_t uid, gid_t gid, const char *name) { - assert((user_uid == (uid_t)-1) || user_pwd == NULL); - assert((user_uid == (uid_t)-1) == - (user_gid == (gid_t)-1)); - - if (user_pwd || user_uid != (uid_t)-1) { - gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid; - uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid; - if (setgid(intended_gid) < 0) { - error_report("Failed to setgid(%d)", intended_gid); - exit(1); - } - if (user_pwd) { - if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) { - error_report("Failed to initgroups(\"%s\", %d)", - user_pwd->pw_name, user_pwd->pw_gid); - exit(1); - } - } else { - if (setgroups(1, &user_gid) < 0) { - error_report("Failed to setgroups(1, [%d])", - user_gid); - exit(1); - } - } - if (setuid(intended_uid) < 0) { - error_report("Failed to setuid(%d)", intended_uid); + if (setgid(gid) < 0) { + error_report("Failed to setgid(%d)", gid); + exit(1); + } + if (name) { + if (initgroups(name, gid) < 0) { + error_report("Failed to initgroups(\"%s\", %d)", + name, gid); exit(1); } - if (setuid(0) != -1) { - error_report("Dropping privileges failed"); + } else { + if (setgroups(1, &gid) < 0) { + error_report("Failed to setgroups(1, [%d])", + gid); exit(1); } } + if (setuid(uid) < 0) { + error_report("Failed to setuid(%d)", uid); + exit(1); + } + if (setuid(0) != -1) { + error_report("Dropping privileges failed"); + exit(1); + } } static void change_root(void) @@ -275,7 +268,9 @@ void os_setup_post(void) } change_root(); - change_process_uid(); + if (user_uid != -1 && user_gid != -1) { + change_process_uid(user_uid, user_gid, user_name); + } if (daemonize) { uint8_t status = 0; -- 2.34.1

Change the change_root() function so that it takes its input as parameters instead of relying on static global variables. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- os-posix.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/os-posix.c b/os-posix.c index 5a127feee2..30da1a1491 100644 --- a/os-posix.c +++ b/os-posix.c @@ -188,19 +188,16 @@ static void change_process_uid(uid_t uid, gid_t gid, const char *name) } } -static void change_root(void) +static void change_root(const char *root) { - if (chroot_dir) { - if (chroot(chroot_dir) < 0) { - error_report("chroot failed"); - exit(1); - } - if (chdir("/")) { - error_report("not able to chdir to /: %s", strerror(errno)); - exit(1); - } + if (chroot(root) < 0) { + error_report("chroot failed"); + exit(1); + } + if (chdir("/")) { + error_report("not able to chdir to /: %s", strerror(errno)); + exit(1); } - } void os_daemonize(void) @@ -267,7 +264,9 @@ void os_setup_post(void) } } - change_root(); + if (chroot_dir) { + change_root(chroot_dir); + } if (user_uid != -1 && user_gid != -1) { change_process_uid(user_uid, user_gid, user_name); } -- 2.34.1

We want to decouple knowledge of daemonization from other code. What the logging code really wants to know is whether it can use stdio or not. Add an API to let the logging code be informed of this fact explicitly. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/qemu/log.h | 1 + softmmu/vl.c | 3 +++ util/log.c | 12 +++++++++--- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index 9b80660207..a35e11a788 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -147,6 +147,7 @@ typedef struct QEMULogItem { extern const QEMULogItem qemu_log_items[]; +void qemu_log_stdio_disable(void); void qemu_set_log(int log_flags); void qemu_log_needs_buffers(void); void qemu_set_log_filename(const char *filename, Error **errp); diff --git a/softmmu/vl.c b/softmmu/vl.c index 1fe028800f..f6f33e15e4 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -3664,6 +3664,9 @@ void qemu_init(int argc, char **argv, char **envp) error_report("Option not supported in this build"); exit(1); } + if (is_daemonized()) { + qemu_log_stdio_disable(); + } } } } diff --git a/util/log.c b/util/log.c index 2ee1500bee..a123622ee3 100644 --- a/util/log.c +++ b/util/log.c @@ -33,6 +33,12 @@ QemuLogFile *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; +static bool stdio_disabled; + +void qemu_log_stdio_disable(void) +{ + stdio_disabled = true; +} /* Return the number of characters emitted. */ int qemu_log(const char *fmt, ...) @@ -92,7 +98,7 @@ void qemu_set_log(int log_flags) * If we are daemonized, * we will only log if there is a logfilename. */ - if (qemu_loglevel && (!is_daemonized() || logfilename)) { + if (qemu_loglevel && (!stdio_disabled || logfilename)) { need_to_open_file = true; } QEMU_LOCK_GUARD(&qemu_logfile_mutex); @@ -110,7 +116,7 @@ void qemu_set_log(int log_flags) _exit(1); } /* In case we are a daemon redirect stderr to logfile */ - if (is_daemonized()) { + if (stdio_disabled) { dup2(fileno(logfile->fd), STDERR_FILENO); fclose(logfile->fd); /* This will skip closing logfile in qemu_log_close() */ @@ -118,7 +124,7 @@ void qemu_set_log(int log_flags) } } else { /* Default to stderr if no log file specified */ - assert(!is_daemonized()); + assert(!stdio_disabled); logfile->fd = stderr; } /* must avoid mmap() usage of glibc by setting a buffer "by hand" */ -- 2.34.1

On Fri, Mar 04, 2022 at 06:56:16PM +0000, Daniel P. Berrangé wrote:
We want to decouple knowledge of daemonization from other code. What the logging code really wants to know is whether it can use stdio or not. Add an API to let the logging code be informed of this fact explicitly.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/qemu/log.h | 1 + softmmu/vl.c | 3 +++ util/log.c | 12 +++++++++--- 3 files changed, 13 insertions(+), 3 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Use of the is_daemonized() method is isolated to allow it to be more easily eliminated in a future change. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- softmmu/vl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/softmmu/vl.c b/softmmu/vl.c index f6f33e15e4..30342b9df2 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -1371,11 +1371,11 @@ static void qemu_disable_default_devices(void) } } -static void qemu_create_default_devices(void) +static void qemu_create_default_devices(bool daemonize) { MachineClass *machine_class = MACHINE_GET_CLASS(current_machine); - if (is_daemonized()) { + if (daemonize) { /* According to documentation and historically, -nographic redirects * serial port, parallel port and monitor to stdio, which does not work * with -daemonize. We can redirect these to null instead, but since @@ -2455,7 +2455,8 @@ static void create_default_memdev(MachineState *ms, const char *path) &error_fatal); } -static void qemu_validate_options(const QDict *machine_opts) +static void qemu_validate_options(const QDict *machine_opts, + bool daemonize) { const char *kernel_filename = qdict_get_try_str(machine_opts, "kernel"); const char *initrd_filename = qdict_get_try_str(machine_opts, "initrd"); @@ -2484,7 +2485,7 @@ static void qemu_validate_options(const QDict *machine_opts) } #ifdef CONFIG_CURSES - if (is_daemonized() && dpy.type == DISPLAY_TYPE_CURSES) { + if (daemonize && dpy.type == DISPLAY_TYPE_CURSES) { error_report("curses display cannot be used with -daemonize"); exit(1); } @@ -3676,7 +3677,7 @@ void qemu_init(int argc, char **argv, char **envp) */ loc_set_none(); - qemu_validate_options(machine_opts_dict); + qemu_validate_options(machine_opts_dict, is_daemonized()); qemu_process_sugar_options(); /* @@ -3714,7 +3715,7 @@ void qemu_init(int argc, char **argv, char **envp) suspend_mux_open(); qemu_disable_default_devices(); - qemu_create_default_devices(); + qemu_create_default_devices(is_daemonized()); qemu_create_early_backends(); qemu_apply_legacy_machine_options(machine_opts_dict); -- 2.34.1

On Fri, Mar 04, 2022 at 06:56:17PM +0000, Daniel P. Berrangé wrote:
Use of the is_daemonized() method is isolated to allow it to be more easily eliminated in a future change.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- softmmu/vl.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

When daemonizing QEMU it is not possible to use the stdio chardev backend because the file descriptors are connected to /dev/null. Currently the chardev checks for this scenario directly, but to decouple it from the system emulator daemonizing code, we reverse the relationship. Now the system emulator calls a helper to explicitly disable use of the stdio backend. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char-stdio.c | 12 ++++++++++-- include/chardev/char-stdio.h | 29 +++++++++++++++++++++++++++++ softmmu/vl.c | 2 ++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 include/chardev/char-stdio.h diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c index 403da308c9..bab0f5ade1 100644 --- a/chardev/char-stdio.c +++ b/chardev/char-stdio.c @@ -28,6 +28,7 @@ #include "qemu/sockets.h" #include "qapi/error.h" #include "chardev/char.h" +#include "chardev/char-stdio.h" #ifdef _WIN32 #include "chardev/char-win.h" @@ -37,6 +38,13 @@ #include "chardev/char-fd.h" #endif +static bool stdio_disabled; + +void qemu_chr_stdio_disable(void) +{ + stdio_disabled = true; +} + #ifndef _WIN32 /* init terminal so that we can grab keys */ static struct termios oldtty; @@ -90,8 +98,8 @@ static void qemu_chr_open_stdio(Chardev *chr, ChardevStdio *opts = backend->u.stdio.data; struct sigaction act; - if (is_daemonized()) { - error_setg(errp, "cannot use stdio with -daemonize"); + if (stdio_disabled) { + error_setg(errp, "cannot use stdio with this configuration"); return; } diff --git a/include/chardev/char-stdio.h b/include/chardev/char-stdio.h new file mode 100644 index 0000000000..eae93a2900 --- /dev/null +++ b/include/chardev/char-stdio.h @@ -0,0 +1,29 @@ +/* + * QEMU System Emulator + * + * Copyright (c) 2003-2008 Fabrice Bellard + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +#ifndef CHAR_STDIO_H +#define CHAR_STDIO_H + +void qemu_chr_stdio_disable(void); + +#endif /* CHAR_STDIO_H */ diff --git a/softmmu/vl.c b/softmmu/vl.c index 30342b9df2..12b714795d 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -69,6 +69,7 @@ #include "exec/gdbstub.h" #include "qemu/timer.h" #include "chardev/char.h" +#include "chardev/char-stdio.h" #include "qemu/bitmap.h" #include "qemu/log.h" #include "sysemu/blockdev.h" @@ -3667,6 +3668,7 @@ void qemu_init(int argc, char **argv, char **envp) } if (is_daemonized()) { qemu_log_stdio_disable(); + qemu_chr_stdio_disable(); } } } -- 2.34.1

On Fri, Mar 04, 2022 at 06:56:18PM +0000, Daniel P. Berrangé wrote:
When daemonizing QEMU it is not possible to use the stdio chardev backend because the file descriptors are connected to /dev/null. Currently the chardev checks for this scenario directly, but to decouple it from the system emulator daemonizing code, we reverse the relationship. Now the system emulator calls a helper to explicitly disable use of the stdio backend.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- chardev/char-stdio.c | 12 ++++++++++-- include/chardev/char-stdio.h | 29 +++++++++++++++++++++++++++++ softmmu/vl.c | 2 ++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 include/chardev/char-stdio.h
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

With the future intent to try to move to a fully QAPI driven configuration system, we want to have any current command parsing well isolated from logic that applies the resulting configuration. We also don't want os-posix.c to contain code that is specific to the system emulators, as this file is linked to other binaries too. To satisfy these goals, we move parsing of the -runas, -chroot and -daemonize code out of the os-posix.c helper code, and pass the parsed data into APIs in os-posix.c. As a side benefit the 'os_daemonize()' function now lives up to its name and will always daemonize, instead of using global state to decide to be a no-op sometimes. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/sysemu/os-posix.h | 4 +- include/sysemu/os-win32.h | 1 - os-posix.c | 148 +++++++++++--------------------------- os-win32.c | 9 --- softmmu/vl.c | 86 ++++++++++++++++++---- 5 files changed, 117 insertions(+), 131 deletions(-) diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index 2edf33658a..390f3f8396 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -46,7 +46,9 @@ void os_set_line_buffering(void); void os_set_proc_name(const char *s); void os_setup_signal_handling(void); void os_daemonize(void); -void os_setup_post(void); +void os_setup_post(const char *chroot_dir, + uid_t uid, gid_t gid, + const char *username); int os_mlock(void); #define closesocket(s) close(s) diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h index 43f569b5c2..4879f8731d 100644 --- a/include/sysemu/os-win32.h +++ b/include/sysemu/os-win32.h @@ -61,7 +61,6 @@ struct tm *localtime_r(const time_t *timep, struct tm *result); static inline void os_setup_signal_handling(void) {} static inline void os_daemonize(void) {} -static inline void os_setup_post(void) {} void os_set_line_buffering(void); static inline void os_set_proc_name(const char *dummy) {} diff --git a/os-posix.c b/os-posix.c index 30da1a1491..f598a52a4f 100644 --- a/os-posix.c +++ b/os-posix.c @@ -42,11 +42,6 @@ #include <sys/prctl.h> #endif -static char *user_name; -static uid_t user_uid = (uid_t)-1; -static gid_t user_gid = (gid_t)-1; - -static const char *chroot_dir; static int daemonize; static int daemon_pipe; @@ -96,69 +91,6 @@ void os_set_proc_name(const char *s) } -static bool os_parse_runas_uid_gid(const char *optarg, - uid_t *runas_uid, gid_t *runas_gid) -{ - unsigned long lv; - const char *ep; - uid_t got_uid; - gid_t got_gid; - int rc; - - rc = qemu_strtoul(optarg, &ep, 0, &lv); - got_uid = lv; /* overflow here is ID in C99 */ - if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) { - return false; - } - - rc = qemu_strtoul(ep + 1, 0, 0, &lv); - got_gid = lv; /* overflow here is ID in C99 */ - if (rc || got_gid != lv || got_gid == (gid_t)-1) { - return false; - } - - *runas_uid = got_uid; - *runas_gid = got_gid; - return true; -} - -/* - * Parse OS specific command line options. - * return 0 if option handled, -1 otherwise - */ -int os_parse_cmd_args(int index, const char *optarg) -{ - struct passwd *user_pwd; - - switch (index) { - case QEMU_OPTION_runas: - user_pwd = getpwnam(optarg); - if (user_pwd) { - user_uid = user_pwd->pw_uid; - user_gid = user_pwd->pw_gid; - user_name = g_strdup(user_pwd->pw_name); - } else if (!os_parse_runas_uid_gid(optarg, - &user_uid, - &user_gid)) { - error_report("User \"%s\" doesn't exist" - " (and is not <uid>:<gid>)", - optarg); - exit(1); - } - break; - case QEMU_OPTION_chroot: - chroot_dir = optarg; - break; - case QEMU_OPTION_daemonize: - daemonize = 1; - break; - default: - return -1; - } - - return 0; -} - static void change_process_uid(uid_t uid, gid_t gid, const char *name) { if (setgid(gid) < 0) { @@ -202,54 +134,56 @@ static void change_root(const char *root) void os_daemonize(void) { - if (daemonize) { - pid_t pid; - int fds[2]; + pid_t pid; + int fds[2]; - if (pipe(fds) == -1) { - exit(1); - } + if (pipe(fds) == -1) { + exit(1); + } - pid = fork(); - if (pid > 0) { - uint8_t status; - ssize_t len; + pid = fork(); + if (pid > 0) { + uint8_t status; + ssize_t len; - close(fds[1]); + close(fds[1]); - do { - len = read(fds[0], &status, 1); - } while (len < 0 && errno == EINTR); + do { + len = read(fds[0], &status, 1); + } while (len < 0 && errno == EINTR); - /* only exit successfully if our child actually wrote - * a one-byte zero to our pipe, upon successful init */ - exit(len == 1 && status == 0 ? 0 : 1); + /* only exit successfully if our child actually wrote + * a one-byte zero to our pipe, upon successful init */ + exit(len == 1 && status == 0 ? 0 : 1); - } else if (pid < 0) { - exit(1); - } + } else if (pid < 0) { + exit(1); + } - close(fds[0]); - daemon_pipe = fds[1]; - qemu_set_cloexec(daemon_pipe); + close(fds[0]); + daemon_pipe = fds[1]; + qemu_set_cloexec(daemon_pipe); - setsid(); + setsid(); - pid = fork(); - if (pid > 0) { + pid = fork(); + if (pid > 0) { exit(0); - } else if (pid < 0) { - exit(1); - } - umask(027); - - signal(SIGTSTP, SIG_IGN); - signal(SIGTTOU, SIG_IGN); - signal(SIGTTIN, SIG_IGN); + } else if (pid < 0) { + exit(1); } + umask(027); + + signal(SIGTSTP, SIG_IGN); + signal(SIGTTOU, SIG_IGN); + signal(SIGTTIN, SIG_IGN); + + daemonize = true; } -void os_setup_post(void) +void os_setup_post(const char *root_dir, + uid_t runas_uid, gid_t runas_gid, + const char *runas_name) { int fd = 0; @@ -264,11 +198,11 @@ void os_setup_post(void) } } - if (chroot_dir) { - change_root(chroot_dir); + if (root_dir != NULL) { + change_root(root_dir); } - if (user_uid != -1 && user_gid != -1) { - change_process_uid(user_uid, user_gid, user_name); + if (runas_uid != -1 && runas_gid != -1) { + change_process_uid(runas_uid, runas_gid, runas_name); } if (daemonize) { diff --git a/os-win32.c b/os-win32.c index e31c921983..6f21b57841 100644 --- a/os-win32.c +++ b/os-win32.c @@ -61,12 +61,3 @@ void os_set_line_buffering(void) setbuf(stdout, NULL); setbuf(stderr, NULL); } - -/* - * Parse OS specific command line options. - * return 0 if option handled, -1 otherwise - */ -int os_parse_cmd_args(int index, const char *optarg) -{ - return -1; -} diff --git a/softmmu/vl.c b/softmmu/vl.c index 12b714795d..0bdd064451 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2604,11 +2604,13 @@ static void qemu_process_help_options(void) } } -static void qemu_maybe_daemonize(const char *pid_file) +static void qemu_maybe_daemonize(bool daemonize, const char *pid_file) { Error *err = NULL; - os_daemonize(); + if (daemonize) { + os_daemonize(); + } rcu_disable_atfork(); if (pid_file && !qemu_write_pidfile(pid_file, &err)) { @@ -2770,6 +2772,35 @@ void qmp_x_exit_preconfig(Error **errp) } } +#ifndef WIN32 +static bool os_parse_runas_uid_gid(const char *optarg, + uid_t *runas_uid, + gid_t *runas_gid) +{ + unsigned long lv; + const char *ep; + uid_t got_uid; + gid_t got_gid; + int rc; + + rc = qemu_strtoul(optarg, &ep, 0, &lv); + got_uid = lv; /* overflow here is ID in C99 */ + if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) { + return false; + } + + rc = qemu_strtoul(ep + 1, 0, 0, &lv); + got_gid = lv; /* overflow here is ID in C99 */ + if (rc || got_gid != lv || got_gid == (gid_t)-1) { + return false; + } + + *runas_gid = got_gid; + *runas_uid = got_uid; + return true; +} +#endif /* !WIN32 */ + void qemu_init(int argc, char **argv, char **envp) { QemuOpts *opts; @@ -2780,6 +2811,14 @@ void qemu_init(int argc, char **argv, char **envp) MachineClass *machine_class; bool userconfig = true; FILE *vmstate_dump_file = NULL; + bool daemonize = false; +#ifndef WIN32 + struct passwd *pwd; + uid_t runas_uid = -1; + gid_t runas_gid = -1; + g_autofree char *runas_name = NULL; + const char *chroot_dir = NULL; +#endif /* !WIN32 */ qemu_add_opts(&qemu_drive_opts); qemu_add_drive_opts(&qemu_legacy_drive_opts); @@ -3661,15 +3700,34 @@ void qemu_init(int argc, char **argv, char **envp) case QEMU_OPTION_nouserconfig: /* Nothing to be parsed here. Especially, do not error out below. */ break; - default: - if (os_parse_cmd_args(popt->index, optarg)) { - error_report("Option not supported in this build"); +#ifndef WIN32 + case QEMU_OPTION_runas: + pwd = getpwnam(optarg); + if (pwd) { + runas_uid = pwd->pw_uid; + runas_gid = pwd->pw_gid; + runas_name = g_strdup(pwd->pw_name); + } else if (!os_parse_runas_uid_gid(optarg, + &runas_uid, + &runas_gid)) { + error_report("User \"%s\" doesn't exist" + " (and is not <uid>:<gid>)", + optarg); exit(1); } - if (is_daemonized()) { - qemu_log_stdio_disable(); - qemu_chr_stdio_disable(); - } + break; + case QEMU_OPTION_chroot: + chroot_dir = optarg; + break; + case QEMU_OPTION_daemonize: + daemonize = 1; + qemu_log_stdio_disable(); + qemu_chr_stdio_disable(); + break; +#endif /* !WIN32 */ + default: + error_report("Option not supported in this build"); + exit(1); } } } @@ -3679,7 +3737,7 @@ void qemu_init(int argc, char **argv, char **envp) */ loc_set_none(); - qemu_validate_options(machine_opts_dict, is_daemonized()); + qemu_validate_options(machine_opts_dict, daemonize); qemu_process_sugar_options(); /* @@ -3689,7 +3747,7 @@ void qemu_init(int argc, char **argv, char **envp) qemu_process_early_options(); qemu_process_help_options(); - qemu_maybe_daemonize(pid_file); + qemu_maybe_daemonize(daemonize, pid_file); /* * The trace backend must be initialized after daemonizing. @@ -3717,7 +3775,7 @@ void qemu_init(int argc, char **argv, char **envp) suspend_mux_open(); qemu_disable_default_devices(); - qemu_create_default_devices(is_daemonized()); + qemu_create_default_devices(daemonize); qemu_create_early_backends(); qemu_apply_legacy_machine_options(machine_opts_dict); @@ -3784,6 +3842,8 @@ void qemu_init(int argc, char **argv, char **envp) } qemu_init_displays(); accel_setup_post(current_machine); - os_setup_post(); +#ifndef WIN32 + os_setup_post(chroot_dir, runas_uid, runas_gid, runas_name); +#endif /* !WIN32 */ resume_mux_open(); } -- 2.34.1

On Fri, Mar 04, 2022 at 06:56:19PM +0000, Daniel P. Berrangé wrote:
With the future intent to try to move to a fully QAPI driven configuration system, we want to have any current command parsing well isolated from logic that applies the resulting configuration.
We also don't want os-posix.c to contain code that is specific to the system emulators, as this file is linked to other binaries too.
To satisfy these goals, we move parsing of the -runas, -chroot and -daemonize code out of the os-posix.c helper code, and pass the parsed data into APIs in os-posix.c.
As a side benefit the 'os_daemonize()' function now lives up to its name and will always daemonize, instead of using global state to decide to be a no-op sometimes.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/sysemu/os-posix.h | 4 +- include/sysemu/os-win32.h | 1 - os-posix.c | 148 +++++++++++--------------------------- os-win32.c | 9 --- softmmu/vl.c | 86 ++++++++++++++++++---- 5 files changed, 117 insertions(+), 131 deletions(-)
@@ -2780,6 +2811,14 @@ void qemu_init(int argc, char **argv, char **envp) MachineClass *machine_class; bool userconfig = true; FILE *vmstate_dump_file = NULL; + bool daemonize = false;
Given this declaration,...
+#ifndef WIN32 + struct passwd *pwd; + uid_t runas_uid = -1; + gid_t runas_gid = -1; + g_autofree char *runas_name = NULL; + const char *chroot_dir = NULL; +#endif /* !WIN32 */
qemu_add_opts(&qemu_drive_opts); qemu_add_drive_opts(&qemu_legacy_drive_opts); @@ -3661,15 +3700,34 @@ void qemu_init(int argc, char **argv, char **envp) case QEMU_OPTION_nouserconfig: /* Nothing to be parsed here. Especially, do not error out below. */ break; - default: - if (os_parse_cmd_args(popt->index, optarg)) { - error_report("Option not supported in this build"); +#ifndef WIN32 + case QEMU_OPTION_runas: + pwd = getpwnam(optarg); + if (pwd) { + runas_uid = pwd->pw_uid; + runas_gid = pwd->pw_gid; + runas_name = g_strdup(pwd->pw_name); + } else if (!os_parse_runas_uid_gid(optarg, + &runas_uid, + &runas_gid)) { + error_report("User \"%s\" doesn't exist" + " (and is not <uid>:<gid>)", + optarg); exit(1); } - if (is_daemonized()) { - qemu_log_stdio_disable(); - qemu_chr_stdio_disable(); - } + break; + case QEMU_OPTION_chroot: + chroot_dir = optarg; + break; + case QEMU_OPTION_daemonize: + daemonize = 1;
...this should s/1/true/. With that tweak, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

There are no longer any users of this method, so it can be removed to prevent future accidental (mis)use. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/sysemu/os-posix.h | 2 -- include/sysemu/os-win32.h | 5 ----- os-posix.c | 5 ----- stubs/is-daemonized.c | 9 --------- stubs/meson.build | 1 - 5 files changed, 22 deletions(-) delete mode 100644 stubs/is-daemonized.c diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index 390f3f8396..2c375f5b49 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -57,8 +57,6 @@ int os_mlock(void); typedef struct timeval qemu_timeval; #define qemu_gettimeofday(tp) gettimeofday(tp, NULL) -bool is_daemonized(void); - /** * qemu_alloc_stack: * @sz: pointer to a size_t holding the requested usable stack size diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h index 4879f8731d..a81f4fa9c1 100644 --- a/include/sysemu/os-win32.h +++ b/include/sysemu/os-win32.h @@ -76,11 +76,6 @@ typedef struct { } qemu_timeval; int qemu_gettimeofday(qemu_timeval *tp); -static inline bool is_daemonized(void) -{ - return false; -} - static inline int os_mlock(void) { return -ENOSYS; diff --git a/os-posix.c b/os-posix.c index f598a52a4f..bd1140ab22 100644 --- a/os-posix.c +++ b/os-posix.c @@ -232,11 +232,6 @@ void os_set_line_buffering(void) setvbuf(stdout, NULL, _IOLBF, 0); } -bool is_daemonized(void) -{ - return daemonize; -} - int os_mlock(void) { #ifdef HAVE_MLOCKALL diff --git a/stubs/is-daemonized.c b/stubs/is-daemonized.c deleted file mode 100644 index 8f63325bb2..0000000000 --- a/stubs/is-daemonized.c +++ /dev/null @@ -1,9 +0,0 @@ -#include "qemu/osdep.h" - -/* Win32 has its own inline stub */ -#ifndef _WIN32 -bool is_daemonized(void) -{ - return false; -} -#endif diff --git a/stubs/meson.build b/stubs/meson.build index d359cbe1ad..3d092b808e 100644 --- a/stubs/meson.build +++ b/stubs/meson.build @@ -18,7 +18,6 @@ if linux_io_uring.found() endif stub_ss.add(files('iothread-lock.c')) stub_ss.add(files('isa-bus.c')) -stub_ss.add(files('is-daemonized.c')) if libaio.found() stub_ss.add(files('linux-aio.c')) endif -- 2.34.1

On Fri, Mar 04, 2022 at 06:56:20PM +0000, Daniel P. Berrangé wrote:
There are no longer any users of this method, so it can be removed to prevent future accidental (mis)use.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- include/sysemu/os-posix.h | 2 -- include/sysemu/os-win32.h | 5 ----- os-posix.c | 5 ----- stubs/is-daemonized.c | 9 --------- stubs/meson.build | 1 - 5 files changed, 22 deletions(-) delete mode 100644 stubs/is-daemonized.c
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Daniel P. Berrangé
-
Eric Blake