From ff21827fee34a5c1db9fe7ca26b0cd94be9cb595 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 17:54:11 +0200 Subject: [PATCH 1/7] core/dbus-manager: use PidRef for log_caller() This is only for logging, but since we lookup for the unit here, let's try to be accurate. --- src/core/dbus-manager.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 6001dccd6e..207dbac52a 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1553,26 +1553,27 @@ static int verify_run_space_permissive(const char *message, sd_bus_error *error) static void log_caller(sd_bus_message *message, Manager *manager, const char *method) { _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - const char *comm = NULL; - Unit *caller; - pid_t pid; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; assert(message); assert(manager); assert(method); - if (sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID|SD_BUS_CREDS_AUGMENT|SD_BUS_CREDS_COMM, &creds) < 0) + if (sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID|SD_BUS_CREDS_PIDFD|SD_BUS_CREDS_AUGMENT|SD_BUS_CREDS_COMM, &creds) < 0) return; - /* We need at least the PID, otherwise there's nothing to log, the rest is optional */ - if (sd_bus_creds_get_pid(creds, &pid) < 0) + /* We need at least the PID, otherwise there's nothing to log, the rest is optional. */ + if (bus_creds_get_pidref(creds, &pidref) < 0) return; + const char *comm = NULL; + Unit *caller; + (void) sd_bus_creds_get_comm(creds, &comm); - caller = manager_get_unit_by_pid(manager, pid); + caller = manager_get_unit_by_pidref(manager, &pidref); log_info("%s requested from client PID " PID_FMT "%s%s%s%s%s%s...", - method, pid, + method, pidref.pid, comm ? " ('" : "", strempty(comm), comm ? "')" : "", caller ? " (unit " : "", caller ? caller->id : "", caller ? ")" : ""); } From b80ae4bcfdf6759108128bc6ab75ad06d07908db Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 18:07:41 +0200 Subject: [PATCH 2/7] core/dbus-manager: use path_simplify_alloc() for root and init paths --- src/core/dbus-manager.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 207dbac52a..6bd38afaaf 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1702,8 +1702,8 @@ static int method_reboot(sd_bus_message *message, void *userdata, sd_bus_error * } static int method_soft_reboot(sd_bus_message *message, void *userdata, sd_bus_error *error) { - _cleanup_free_ char *rt = NULL; Manager *m = ASSERT_PTR(userdata); + _cleanup_free_ char *rt = NULL; const char *root; int r; @@ -1729,9 +1729,9 @@ static int method_soft_reboot(sd_bus_message *message, void *userdata, sd_bus_er return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "New root directory path '%s' is not absolute.", root); - rt = strdup(root); - if (!rt) - return -ENOMEM; + r = path_simplify_alloc(root, &rt); + if (r < 0) + return r; } free_and_replace(m->switch_root, rt); @@ -1798,8 +1798,8 @@ static int method_kexec(sd_bus_message *message, void *userdata, sd_bus_error *e } static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_error *error) { - _cleanup_free_ char *ri = NULL, *rt = NULL; Manager *m = ASSERT_PTR(userdata); + _cleanup_free_ char *ri = NULL, *rt = NULL; const char *root, *init; int r; @@ -1877,14 +1877,14 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er "Could not resolve init executable %s: %m", init); } - rt = strdup(root); - if (!rt) - return -ENOMEM; + r = path_simplify_alloc(root, &rt); + if (r < 0) + return r; if (!isempty(init)) { - ri = strdup(init); - if (!ri) - return -ENOMEM; + r = path_simplify_alloc(init, &ri); + if (r < 0) + return r; } free_and_replace(m->switch_root, rt); From a85fe612d7c9882e978fed1ae056b9e79f34799d Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 18:03:21 +0200 Subject: [PATCH 3/7] core/dbus-manager: use sd_bus_error_set() rather than _setf() where appropriate Also, there can only be one system manager. --- src/core/dbus-manager.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 6bd38afaaf..5304edbc74 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1693,8 +1693,8 @@ static int method_reboot(sd_bus_message *message, void *userdata, sd_bus_error * return r; if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, - "Reboot is only supported for system managers."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Reboot is only supported by system manager."); m->objective = MANAGER_REBOOT; @@ -1751,8 +1751,8 @@ static int method_poweroff(sd_bus_message *message, void *userdata, sd_bus_error return r; if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, - "Powering off is only supported for system managers."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Powering off is only supported by system manager."); m->objective = MANAGER_POWEROFF; @@ -1770,8 +1770,8 @@ static int method_halt(sd_bus_message *message, void *userdata, sd_bus_error *er return r; if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, - "Halt is only supported for system managers."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Halt is only supported by system manager."); m->objective = MANAGER_HALT; @@ -1789,8 +1789,8 @@ static int method_kexec(sd_bus_message *message, void *userdata, sd_bus_error *e return r; if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, - "KExec is only supported for system managers."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "KExec is only supported by system manager."); m->objective = MANAGER_KEXEC; @@ -1814,8 +1814,8 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er return r; if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, - "Root switching is only supported by system manager."); + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Root switching is only supported by system manager."); r = sd_bus_message_read(message, "ss", &root, &init); if (r < 0) @@ -1827,8 +1827,8 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er root = "/sysroot"; else { if (!path_is_valid(root)) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "New root directory must be a valid path."); + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "New root directory must be a valid path."); if (!path_is_absolute(root)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, @@ -1840,14 +1840,14 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er "Failed to check if new root directory '%s' is the same as old root: %m", root); if (r > 0) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "New root directory cannot be the old root directory."); + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "New root directory cannot be the old root directory."); } /* Safety check */ if (!in_initrd()) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, - "Not in initrd, refusing switch-root operation."); + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, + "Not in initrd, refusing switch-root operation."); r = path_is_os_tree(root); if (r < 0) From 541aadff9778e0817da16f1b34d2437b1bb22fde Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 19:28:30 +0200 Subject: [PATCH 4/7] core/dbus-manager: check for runtime scope first for system-wide operations It's pointless to do selinux or /run/ space checks for user managers. --- src/core/dbus-manager.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 5304edbc74..ddc721f5d1 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1688,14 +1688,14 @@ static int method_reboot(sd_bus_message *message, void *userdata, sd_bus_error * assert(message); - r = mac_selinux_access_check(message, "reboot", error); - if (r < 0) - return r; - if (!MANAGER_IS_SYSTEM(m)) return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, "Reboot is only supported by system manager."); + r = mac_selinux_access_check(message, "reboot", error); + if (r < 0) + return r; + m->objective = MANAGER_REBOOT; return sd_bus_reply_method_return(message, NULL); @@ -1746,14 +1746,14 @@ static int method_poweroff(sd_bus_message *message, void *userdata, sd_bus_error assert(message); - r = mac_selinux_access_check(message, "halt", error); - if (r < 0) - return r; - if (!MANAGER_IS_SYSTEM(m)) return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, "Powering off is only supported by system manager."); + r = mac_selinux_access_check(message, "halt", error); + if (r < 0) + return r; + m->objective = MANAGER_POWEROFF; return sd_bus_reply_method_return(message, NULL); @@ -1765,14 +1765,14 @@ static int method_halt(sd_bus_message *message, void *userdata, sd_bus_error *er assert(message); - r = mac_selinux_access_check(message, "halt", error); - if (r < 0) - return r; - if (!MANAGER_IS_SYSTEM(m)) return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, "Halt is only supported by system manager."); + r = mac_selinux_access_check(message, "halt", error); + if (r < 0) + return r; + m->objective = MANAGER_HALT; return sd_bus_reply_method_return(message, NULL); @@ -1784,14 +1784,14 @@ static int method_kexec(sd_bus_message *message, void *userdata, sd_bus_error *e assert(message); - r = mac_selinux_access_check(message, "reboot", error); - if (r < 0) - return r; - if (!MANAGER_IS_SYSTEM(m)) return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, "KExec is only supported by system manager."); + r = mac_selinux_access_check(message, "reboot", error); + if (r < 0) + return r; + m->objective = MANAGER_KEXEC; return sd_bus_reply_method_return(message, NULL); @@ -1805,6 +1805,10 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er assert(message); + if (!MANAGER_IS_SYSTEM(m)) + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Root switching is only supported by system manager."); + r = verify_run_space_permissive("root switching may fail", error); if (r < 0) return r; @@ -1813,10 +1817,6 @@ static int method_switch_root(sd_bus_message *message, void *userdata, sd_bus_er if (r < 0) return r; - if (!MANAGER_IS_SYSTEM(m)) - return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, - "Root switching is only supported by system manager."); - r = sd_bus_message_read(message, "ss", &root, &init); if (r < 0) return r; From 236cd4854657745e1a59b224a191a232a476527e Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 18:08:06 +0200 Subject: [PATCH 5/7] core/dbus-manager: refuse SoftReboot() for user managers Otherwise, busctl --user call ... SoftReboot results in user manager broadcasting signal and initiating soft-reboot... --- src/core/dbus-manager.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index ddc721f5d1..3d2796f2c4 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -1709,6 +1709,10 @@ static int method_soft_reboot(sd_bus_message *message, void *userdata, sd_bus_er assert(message); + if (!MANAGER_IS_SYSTEM(m)) + return sd_bus_error_set(error, SD_BUS_ERROR_NOT_SUPPORTED, + "Soft reboot is only supported by system manager."); + r = verify_run_space_permissive("soft reboot may fail", error); if (r < 0) return r; From 6851abe40381cd2c5f059eccc22c6967d0b034e5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 18:30:44 +0200 Subject: [PATCH 6/7] core/manager: invoke special targets on signal only for system manager --- src/core/manager.c | 66 ++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 32 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 62a2da96c6..f5588380d6 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3087,41 +3087,43 @@ static int manager_dispatch_signal_fd(sd_event_source *source, int fd, uint32_t default: { - /* Starting SIGRTMIN+0 */ - static const struct { - const char *target; - JobMode mode; - } target_table[] = { - [0] = { SPECIAL_DEFAULT_TARGET, JOB_ISOLATE }, - [1] = { SPECIAL_RESCUE_TARGET, JOB_ISOLATE }, - [2] = { SPECIAL_EMERGENCY_TARGET, JOB_ISOLATE }, - [3] = { SPECIAL_HALT_TARGET, JOB_REPLACE_IRREVERSIBLY }, - [4] = { SPECIAL_POWEROFF_TARGET, JOB_REPLACE_IRREVERSIBLY }, - [5] = { SPECIAL_REBOOT_TARGET, JOB_REPLACE_IRREVERSIBLY }, - [6] = { SPECIAL_KEXEC_TARGET, JOB_REPLACE_IRREVERSIBLY }, - [7] = { SPECIAL_SOFT_REBOOT_TARGET, JOB_REPLACE_IRREVERSIBLY }, - }; + if (MANAGER_IS_SYSTEM(m)) { + /* Starting SIGRTMIN+0 */ + static const struct { + const char *target; + JobMode mode; + } target_table[] = { + [0] = { SPECIAL_DEFAULT_TARGET, JOB_ISOLATE }, + [1] = { SPECIAL_RESCUE_TARGET, JOB_ISOLATE }, + [2] = { SPECIAL_EMERGENCY_TARGET, JOB_ISOLATE }, + [3] = { SPECIAL_HALT_TARGET, JOB_REPLACE_IRREVERSIBLY }, + [4] = { SPECIAL_POWEROFF_TARGET, JOB_REPLACE_IRREVERSIBLY }, + [5] = { SPECIAL_REBOOT_TARGET, JOB_REPLACE_IRREVERSIBLY }, + [6] = { SPECIAL_KEXEC_TARGET, JOB_REPLACE_IRREVERSIBLY }, + [7] = { SPECIAL_SOFT_REBOOT_TARGET, JOB_REPLACE_IRREVERSIBLY }, + }; - /* Starting SIGRTMIN+13, so that target halt and system halt are 10 apart */ - static const ManagerObjective objective_table[] = { - [0] = MANAGER_HALT, - [1] = MANAGER_POWEROFF, - [2] = MANAGER_REBOOT, - [3] = MANAGER_KEXEC, - [4] = MANAGER_SOFT_REBOOT, - }; + /* Starting SIGRTMIN+13, so that target halt and system halt are 10 apart */ + static const ManagerObjective objective_table[] = { + [0] = MANAGER_HALT, + [1] = MANAGER_POWEROFF, + [2] = MANAGER_REBOOT, + [3] = MANAGER_KEXEC, + [4] = MANAGER_SOFT_REBOOT, + }; - if ((int) sfsi.ssi_signo >= SIGRTMIN+0 && - (int) sfsi.ssi_signo < SIGRTMIN+(int) ELEMENTSOF(target_table)) { - int idx = (int) sfsi.ssi_signo - SIGRTMIN; - manager_start_special(m, target_table[idx].target, target_table[idx].mode); - break; - } + if ((int) sfsi.ssi_signo >= SIGRTMIN+0 && + (int) sfsi.ssi_signo < SIGRTMIN+(int) ELEMENTSOF(target_table)) { + int idx = (int) sfsi.ssi_signo - SIGRTMIN; + manager_start_special(m, target_table[idx].target, target_table[idx].mode); + break; + } - if ((int) sfsi.ssi_signo >= SIGRTMIN+13 && - (int) sfsi.ssi_signo < SIGRTMIN+13+(int) ELEMENTSOF(objective_table)) { - m->objective = objective_table[sfsi.ssi_signo - SIGRTMIN - 13]; - break; + if ((int) sfsi.ssi_signo >= SIGRTMIN+13 && + (int) sfsi.ssi_signo < SIGRTMIN+13+(int) ELEMENTSOF(objective_table)) { + m->objective = objective_table[sfsi.ssi_signo - SIGRTMIN - 13]; + break; + } } switch (sfsi.ssi_signo - SIGRTMIN) { From 33a06bbc742fc27d24127a2d6a64d5f2f5cf23dd Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 2 Jul 2024 18:14:33 +0200 Subject: [PATCH 7/7] core/main: add an assertion to ensure user managers won't initiate destructive/system-wide operation --- src/core/main.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/core/main.c b/src/core/main.c index 6792a79c1f..8dc0245fa0 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -3285,14 +3285,15 @@ int main(int argc, char *argv[]) { &switch_root_dir, &switch_root_init, &error_message); - assert(r < 0 || IN_SET(r, MANAGER_EXIT, /* MANAGER_OK and MANAGER_RELOAD are not expected here. */ - MANAGER_REEXECUTE, - MANAGER_REBOOT, - MANAGER_SOFT_REBOOT, - MANAGER_POWEROFF, - MANAGER_HALT, - MANAGER_KEXEC, - MANAGER_SWITCH_ROOT)); + /* MANAGER_OK and MANAGER_RELOAD are not expected here. */ + assert(r < 0 || IN_SET(r, MANAGER_REEXECUTE, MANAGER_EXIT) || + (arg_runtime_scope == RUNTIME_SCOPE_SYSTEM && + IN_SET(r, MANAGER_REBOOT, + MANAGER_SOFT_REBOOT, + MANAGER_POWEROFF, + MANAGER_HALT, + MANAGER_KEXEC, + MANAGER_SWITCH_ROOT))); finish: pager_close();