From 569ad251adde02dc0915758fe027e0346e50738a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Jan 2019 16:35:55 +0100 Subject: [PATCH 1/7] udevadm: drop pointless must_be_root() checks Checking if we are root on the client side is generally pointless, since the privileged operation will fail anyway and we can than log what precisly went wrong. A check like this makes sense only if: - we need to do some expensive unprivileged operation before attempting the privileged operation, and the check allows us avoid wasting resources. - the privileged operation would fail but in an unclear way. Neither of those cases applies here. This fixes calls like 'udevadm control -h' as unprivileged user. --- src/udev/udevadm-control.c | 4 ---- src/udev/udevadm-trigger.c | 6 ------ 2 files changed, 10 deletions(-) diff --git a/src/udev/udevadm-control.c b/src/udev/udevadm-control.c index f9b3e95794a..82e25149325 100644 --- a/src/udev/udevadm-control.c +++ b/src/udev/udevadm-control.c @@ -73,10 +73,6 @@ int control_main(int argc, char *argv[], void *userdata) { {} }; - r = must_be_root(); - if (r < 0) - return r; - if (running_in_chroot() > 0) { log_info("Running in chroot, ignoring request."); return 0; diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index b7dafb77557..144b464d24f 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -301,12 +301,6 @@ int trigger_main(int argc, char *argv[], void *userdata) { } } - if (!arg_dry_run || ping) { - r = must_be_root(); - if (r < 0) - return r; - } - if (ping) { _cleanup_(udev_ctrl_unrefp) struct udev_ctrl *uctrl = NULL; From ded3a403016494a3e01b6de2f4ed14dde78a62b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Jan 2019 16:46:37 +0100 Subject: [PATCH 2/7] bless-boot: drop must_be_root() checks If we lack permissions, we will fail anyway. But by not doing the artifial check, we get more information. For example, on my machine, I see $ build/systemd-bless-boot good Not booted with boot counting in effect. while "Need to be root" is actually untrue, because being root doesn't change the outcome in any way. Letting the operation fail on the actual error makes it easier to do test runs: we *know* the command will fail, but we want to see what the first step would be. Not doing the articial check makes it also easier to do create alternative security arrangements, for example where the directories are mounted with special ownership mode and an otherwise unprivileged user can perform certain operations. --- src/boot/bless-boot.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/boot/bless-boot.c b/src/boot/bless-boot.c index b5d110f4224..f2d033fc407 100644 --- a/src/boot/bless-boot.c +++ b/src/boot/bless-boot.c @@ -480,13 +480,12 @@ exists: } static int run(int argc, char *argv[]) { - static const Verb verbs[] = { - { "help", VERB_ANY, VERB_ANY, 0, help }, - { "status", VERB_ANY, 1, VERB_DEFAULT, verb_status }, - { "good", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_set }, - { "bad", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_set }, - { "indeterminate", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_set }, + { "help", VERB_ANY, VERB_ANY, 0, help }, + { "status", VERB_ANY, 1, VERB_DEFAULT, verb_status }, + { "good", VERB_ANY, 1, 0, verb_set }, + { "bad", VERB_ANY, 1, 0, verb_set }, + { "indeterminate", VERB_ANY, 1, 0, verb_set }, {} }; From 7c3ce8b5a96ef7d377fe3a3b38b547b17f0c7e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 18 Jan 2019 16:53:03 +0100 Subject: [PATCH 3/7] bootctl: drop must_be_root() checks The reasoning is the same as in previous cases. We get an error like "Failed to update EFI variable: Operation not permitted" anyway, so the check is not very useful. --- src/boot/bootctl.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/boot/bootctl.c b/src/boot/bootctl.c index 9df181a10c9..9ccf0104c16 100644 --- a/src/boot/bootctl.c +++ b/src/boot/bootctl.c @@ -1430,16 +1430,15 @@ static int verb_set_default(int argc, char *argv[], void *userdata) { } static int bootctl_main(int argc, char *argv[]) { - static const Verb verbs[] = { - { "help", VERB_ANY, VERB_ANY, 0, help }, - { "status", VERB_ANY, 1, VERB_DEFAULT, verb_status }, - { "install", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_install }, - { "update", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_install }, - { "remove", VERB_ANY, 1, VERB_MUST_BE_ROOT, verb_remove }, - { "list", VERB_ANY, 1, 0, verb_list }, - { "set-default", 2, 2, VERB_MUST_BE_ROOT, verb_set_default }, - { "set-oneshot", 2, 2, VERB_MUST_BE_ROOT, verb_set_default }, + { "help", VERB_ANY, VERB_ANY, 0, help }, + { "status", VERB_ANY, 1, VERB_DEFAULT, verb_status }, + { "install", VERB_ANY, 1, 0, verb_install }, + { "update", VERB_ANY, 1, 0, verb_install }, + { "remove", VERB_ANY, 1, 0, verb_remove }, + { "list", VERB_ANY, 1, 0, verb_list }, + { "set-default", 2, 2, 0, verb_set_default }, + { "set-oneshot", 2, 2, 0, verb_set_default }, {} }; From a004cb75c0a0ae085ebc9b470afba68375a53507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 May 2019 11:29:53 +0200 Subject: [PATCH 4/7] systemctl: drop one must_be_root_check() (before) $ build/systemctl list-machines Need to be root. $ sudo build/systemctl list-machines NAME STATE FAILED JOBS krowka (host) running 0 0 rawhide running 0 0 2 machines listed. (after) $ build/systemctl list-machines NAME STATE FAILED JOBS krowka (host) running 0 0 rawhide n/a 0 0 2 machines listed. The output for non-root is missing some bits of information, but we display what information is missing nicely, and e.g. in the case when no machines are running at all, or only VMs are running, the unprivileged output would be the same as the privileged one. --- src/systemctl/systemctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 059f7220b54..aa263c79c7f 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -1950,7 +1950,7 @@ static void output_machines_list(struct machine_info *machine_infos, unsigned n) failedlen = MAX(failedlen, DECIMAL_STR_WIDTH(m->n_failed_units)); jobslen = MAX(jobslen, DECIMAL_STR_WIDTH(m->n_jobs)); - if (!arg_plain && !streq_ptr(m->state, "running")) + if (!arg_plain && m->state && !streq(m->state, "running")) circle_len = 2; } @@ -8808,7 +8808,7 @@ static int systemctl_main(int argc, char *argv[]) { { "list-sockets", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY, list_sockets }, { "list-timers", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY, list_timers }, { "list-jobs", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY, list_jobs }, - { "list-machines", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY|VERB_MUST_BE_ROOT, list_machines }, + { "list-machines", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY, list_machines }, { "clear-jobs", VERB_ANY, 1, VERB_ONLINE_ONLY, trivial_method }, { "cancel", VERB_ANY, VERB_ANY, VERB_ONLINE_ONLY, cancel_job }, { "start", 2, VERB_ANY, VERB_ONLINE_ONLY, start_unit }, From 62bf89d7d2118d197c6c932c86012e72497c6d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 May 2019 11:33:02 +0200 Subject: [PATCH 5/7] shared/verbs: drop now-unused VERB_MUST_BE_ROOT --- src/shared/verbs.c | 8 +------- src/shared/verbs.h | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/shared/verbs.c b/src/shared/verbs.c index 7c5dcb02a24..c87b496736e 100644 --- a/src/shared/verbs.c +++ b/src/shared/verbs.c @@ -50,7 +50,7 @@ int dispatch_verb(int argc, char *argv[], const Verb verbs[], void *userdata) { const Verb *verb; const char *name; unsigned i; - int left, r; + int left; assert(verbs); assert(verbs[0].dispatch); @@ -109,12 +109,6 @@ int dispatch_verb(int argc, char *argv[], const Verb verbs[], void *userdata) { return 0; } - if (verb->flags & VERB_MUST_BE_ROOT) { - r = must_be_root(); - if (r < 0) - return r; - } - if (name) return verb->dispatch(left, argv, userdata); else { diff --git a/src/shared/verbs.h b/src/shared/verbs.h index 010c0df3fdc..c5fe6cc7c58 100644 --- a/src/shared/verbs.h +++ b/src/shared/verbs.h @@ -8,7 +8,6 @@ typedef enum VerbFlags { VERB_DEFAULT = 1 << 0, VERB_ONLINE_ONLY = 1 << 1, - VERB_MUST_BE_ROOT = 1 << 2, } VerbFlags; typedef struct { From 4bea24696a6fb5cf3c9306e83ead5e9efc27239e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 8 May 2019 11:48:12 +0200 Subject: [PATCH 6/7] systemctl: show a hint if root privileges might yield more information --- src/systemctl/systemctl.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index aa263c79c7f..61efacf868f 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -1940,6 +1940,7 @@ static void output_machines_list(struct machine_info *machine_infos, unsigned n) statelen = STRLEN("STATE"), failedlen = STRLEN("FAILED"), jobslen = STRLEN("JOBS"); + bool state_missing; assert(machine_infos || n == 0); @@ -1989,9 +1990,12 @@ static void output_machines_list(struct machine_info *machine_infos, unsigned n) if (circle_len > 0) printf("%s%s%s ", on_state, circle ? special_glyph(SPECIAL_GLYPH_BLACK_CIRCLE) : " ", off_state); + if (!m->state) + state_missing = true; + if (m->is_host) printf("%-*s (host) %s%-*s%s %s%*" PRIu32 "%s %*" PRIu32 "\n", - (int) (namelen - (STRLEN(" (host)"))), + (int) (namelen - strlen(" (host)")), strna(m->name), on_state, statelen, strna(m->state), off_state, on_failed, failedlen, m->n_failed_units, off_failed, @@ -2004,8 +2008,12 @@ static void output_machines_list(struct machine_info *machine_infos, unsigned n) jobslen, m->n_jobs); } - if (!arg_no_legend) - printf("\n%u machines listed.\n", n); + if (!arg_no_legend) { + printf("\n"); + if (state_missing && geteuid() != 0) + printf("Notice: some information only available to privileged users was not shown.\n"); + printf("%u machines listed.\n", n); + } } static int list_machines(int argc, char *argv[], void *userdata) { From 97afc0351a96e0daa83964df33937967c75c644f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 5 Jun 2019 09:54:54 +0200 Subject: [PATCH 7/7] udevadm trigger: log errors and return first failure When udevadm trigger is called, the list of devices to trigger is always generated through enumeration, and devices can come and go, so we should not treat -ENOENT as a failure. But other types of failure should be logged. It seems they were logged until baa30fbc2c04b23209d0b8fb3c86cd15ef9ea81a. Also, return the first error. (I'm not sure if there are other failure modes which we want to ignore. If they are, they'll need to be whitelisted like -ENOENT.). --- src/udev/udevadm-trigger.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/udev/udevadm-trigger.c b/src/udev/udevadm-trigger.c index 144b464d24f..f14010a2d0d 100644 --- a/src/udev/udevadm-trigger.c +++ b/src/udev/udevadm-trigger.c @@ -25,7 +25,7 @@ static bool arg_dry_run = false; static int exec_list(sd_device_enumerator *e, const char *action, Set *settle_set) { sd_device *d; - int r; + int r, ret = 0; FOREACH_DEVICE_AND_SUBSYSTEM(e, d) { _cleanup_free_ char *filename = NULL; @@ -45,7 +45,10 @@ static int exec_list(sd_device_enumerator *e, const char *action, Set *settle_se r = write_string_file(filename, action, WRITE_STRING_FILE_DISABLE_BUFFER); if (r < 0) { - log_debug_errno(r, "Failed to write '%s' to '%s', ignoring: %m", action, filename); + log_full_errno(r == -ENOENT ? LOG_DEBUG : LOG_ERR, r, + "Failed to write '%s' to '%s': %m", action, filename); + if (ret == 0 && r != -ENOENT) + ret = r; continue; } @@ -56,7 +59,7 @@ static int exec_list(sd_device_enumerator *e, const char *action, Set *settle_se } } - return 0; + return ret; } static int device_monitor_handler(sd_device_monitor *m, sd_device *dev, void *userdata) {