Merge pull request #7611 from poettering/bootspec-fixes

minor fixes to bootctl.c/bootspec.c to make sure the tool works cleanly on my system

src/basic/process-util.c

@@ -1053,6 +1053,15 @@ pid_t getpid_cached(void) {
         }
 }
 
+int must_be_root(void) {
+
+        if (geteuid() == 0)
+                return 0;
+
+        log_error("Need to be root.");
+        return -EPERM;
+}
+
 static const char *const ioprio_class_table[] = {
         [IOPRIO_CLASS_NONE] = "none",
         [IOPRIO_CLASS_RT] = "realtime",

src/basic/process-util.h

@@ -138,3 +138,5 @@ static inline bool pid_is_valid(pid_t p) {
 int ioprio_parse_priority(const char *s, int *ret);
 
 pid_t getpid_cached(void);
+
+int must_be_root(void);

src/basic/verbs.c

@@ -33,7 +33,7 @@ int dispatch_verb(int argc, char *argv[], const Verb verbs[], void *userdata) {
         const Verb *verb;
         const char *name;
         unsigned i;
-        int left;
+        int left, r;
 
         assert(verbs);
         assert(verbs[0].dispatch);
@@ -89,6 +89,12 @@ int dispatch_verb(int argc, char *argv[], const Verb verbs[], void *userdata) {
                 return 0;
         }
 
+        if (verb->flags & VERB_MUSTBEROOT) {
+                r = must_be_root();
+                if (r < 0)
+                        return r;
+        }
+
         if (name)
                 return verb->dispatch(left, argv + optind, userdata);
         else {

src/basic/verbs.h

@@ -21,13 +21,17 @@
 ***/
 
 #define VERB_ANY ((unsigned) -1)
-#define VERB_DEFAULT 1U
-#define VERB_NOCHROOT 2U
+
+typedef enum VerbFlags {
+        VERB_DEFAULT    = 1 << 0,
+        VERB_NOCHROOT   = 1 << 1,
+        VERB_MUSTBEROOT = 1 << 2,
+} VerbFlags;
 
 typedef struct {
         const char *verb;
         unsigned min_args, max_args;
-        unsigned flags;
+        VerbFlags flags;
         int (* const dispatch)(int argc, char *argv[], void *userdata);
 } Verb;
 

src/boot/bootctl.c

@@ -61,19 +61,33 @@ static char *arg_path = NULL;
 static bool arg_print_path = false;
 static bool arg_touch_variables = true;
 
-static int find_esp_and_warn(uint32_t *part, uint64_t *pstart, uint64_t *psize, sd_id128_t *uuid) {
+static int acquire_esp(
+                bool unprivileged_mode,
+                uint32_t *ret_part,
+                uint64_t *ret_pstart,
+                uint64_t *ret_psize,
+                sd_id128_t *ret_uuid) {
+
+        char *np;
         int r;
 
-        r = find_esp(&arg_path, part, pstart, psize, uuid);
-        if (r == -ENOENT)
+        /* Find the ESP, and log about errors. Note that find_esp_and_warn() will log in all error cases on its own,
+         * except for ENOKEY (which is good, we want to show our own message in that case, suggesting use of --path=)
+         * and EACCESS (only when we request unprivileged mode; in this case we simply eat up the error here, so that
+         * --list and --status work too, without noise about this). */
+
+        r = find_esp_and_warn(arg_path, unprivileged_mode, &np, ret_part, ret_pstart, ret_psize, ret_uuid);
+        if (r == -ENOKEY)
                 return log_error_errno(r,
-                                       "Couldn't find EFI system partition. It is recommended to mount it to /boot.\n"
+                                       "Couldn't find EFI system partition. It is recommended to mount it to /boot or /efi.\n"
                                        "Alternatively, use --path= to specify path to mount point.");
-        else if (r < 0)
-                return log_error_errno(r,
-                                       "Couldn't find EFI system partition: %m");
+        if (r < 0)
+                return r;
+
+        free_and_replace(arg_path, np);
 
         log_debug("Using EFI System Partition at %s.", arg_path);
+
         return 0;
 }
 
@@ -911,23 +925,17 @@ static void read_loader_efi_var(const char *name, char **var) {
                 log_warning_errno(r, "Failed to read EFI variable %s: %m", name);
 }
 
-static int must_be_root(void) {
-
-        if (geteuid() == 0)
-                return 0;
-
-        log_error("Need to be root.");
-        return -EPERM;
-}
-
 static int verb_status(int argc, char *argv[], void *userdata) {
 
         sd_id128_t uuid = SD_ID128_NULL;
         int r, k;
 
-        r = find_esp_and_warn(NULL, NULL, NULL, &uuid);
+        r = acquire_esp(geteuid() != 0, NULL, NULL, NULL, &uuid);
 
         if (arg_print_path) {
+                if (r == -EACCES) /* If we couldn't acquire the ESP path, log about access errors (which is the only
+                                   * error the find_esp_and_warn() won't log on its own) */
+                        return log_error_errno(r, "Failed to determine ESP: %m");
                 if (r < 0)
                         return r;
 
@@ -935,6 +943,9 @@ static int verb_status(int argc, char *argv[], void *userdata) {
                 return 0;
         }
 
+        r = 0; /* If we couldn't determine the path, then don't consider that a problem from here on, just show what we
+                * can show */
+
         if (is_efi_boot()) {
                 _cleanup_free_ char *fw_type = NULL, *fw_info = NULL, *loader = NULL, *loader_path = NULL;
                 sd_id128_t loader_part_uuid = SD_ID128_NULL;
@@ -979,9 +990,11 @@ static int verb_status(int argc, char *argv[], void *userdata) {
         } else
                 printf("System:\n    Not booted with EFI\n\n");
 
-        k = status_binaries(arg_path, uuid);
-        if (k < 0)
-                r = k;
+        if (arg_path) {
+                k = status_binaries(arg_path, uuid);
+                if (k < 0)
+                        r = k;
+        }
 
         if (is_efi_boot()) {
                 k = status_variables();
@@ -989,21 +1002,28 @@ static int verb_status(int argc, char *argv[], void *userdata) {
                         r = k;
         }
 
-        k = status_entries(arg_path, uuid);
-        if (k < 0)
-                r = k;
+        if (arg_path) {
+                k = status_entries(arg_path, uuid);
+                if (k < 0)
+                        r = k;
+        }
 
         return r;
 }
 
 static int verb_list(int argc, char *argv[], void *userdata) {
-        sd_id128_t uuid = SD_ID128_NULL;
-        int r;
-        unsigned n;
-
         _cleanup_(boot_config_free) BootConfig config = {};
+        sd_id128_t uuid = SD_ID128_NULL;
+        unsigned n;
+        int r;
 
-        r = find_esp_and_warn(NULL, NULL, NULL, &uuid);
+        /* If we lack privileges we invoke find_esp_and_warn() in "unprivileged mode" here, which does two things: turn
+         * off logging about access errors and turn off potentially privileged device probing. Here we're interested in
+         * the latter but not the former, hence request the mode, and log about EACCES. */
+
+        r = acquire_esp(geteuid() != 0, NULL, NULL, NULL, &uuid);
+        if (r == -EACCES) /* We really need the ESP path for this call, hence also log about access errors */
+                return log_error_errno(r, "Failed to determine ESP: %m");
         if (r < 0)
                 return r;
 
@@ -1067,11 +1087,7 @@ static int verb_install(int argc, char *argv[], void *userdata) {
         bool install;
         int r;
 
-        r = must_be_root();
-        if (r < 0)
-                return r;
-
-        r = find_esp_and_warn(&part, &pstart, &psize, &uuid);
+        r = acquire_esp(false, &part, &pstart, &psize, &uuid);
         if (r < 0)
                 return r;
 
@@ -1102,11 +1118,7 @@ static int verb_remove(int argc, char *argv[], void *userdata) {
         sd_id128_t uuid = SD_ID128_NULL;
         int r;
 
-        r = must_be_root();
-        if (r < 0)
-                return r;
-
-        r = find_esp_and_warn(NULL, NULL, NULL, &uuid);
+        r = acquire_esp(false, NULL, NULL, NULL, &uuid);
         if (r < 0)
                 return r;
 
@@ -1126,12 +1138,12 @@ static int verb_remove(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  },
-                { "list",            VERB_ANY, 1,        0,            verb_list    },
-                { "install",         VERB_ANY, 1,        0,            verb_install },
-                { "update",          VERB_ANY, 1,        0,            verb_install },
-                { "remove",          VERB_ANY, 1,        0,            verb_remove  },
+                { "help",            VERB_ANY, VERB_ANY, 0,               help         },
+                { "status",          VERB_ANY, 1,        VERB_DEFAULT,    verb_status  },
+                { "list",            VERB_ANY, 1,        0,               verb_list    },
+                { "install",         VERB_ANY, 1,        VERB_MUSTBEROOT, verb_install },
+                { "update",          VERB_ANY, 1,        VERB_MUSTBEROOT, verb_install },
+                { "remove",          VERB_ANY, 1,        VERB_MUSTBEROOT, verb_remove  },
                 {}
         };
 

src/nspawn/nspawn.c

@@ -3783,11 +3783,10 @@ int main(int argc, char *argv[]) {
         if (r <= 0)
                 goto finish;
 
-        if (geteuid() != 0) {
-                log_error("Need to be root.");
-                r = -EPERM;
+        r = must_be_root();
+        if (r < 0)
                 goto finish;
-        }
+
         r = determine_names();
         if (r < 0)
                 goto finish;

src/shared/bootspec.c

@@ -36,8 +36,9 @@
 #include "virt.h"
 
 void boot_entry_free(BootEntry *entry) {
-        free(entry->filename);
+        assert(entry);
 
+        free(entry->filename);
         free(entry->title);
         free(entry->show_title);
         free(entry->version);
@@ -56,6 +57,9 @@ int boot_entry_load(const char *path, BootEntry *entry) {
         _cleanup_(boot_entry_free) BootEntry tmp = {};
         int r;
 
+        assert(path);
+        assert(entry);
+
         f = fopen(path, "re");
         if (!f)
                 return log_error_errno(errno, "Failed to open \"%s\": %m", path);
@@ -123,6 +127,8 @@ int boot_entry_load(const char *path, BootEntry *entry) {
 void boot_config_free(BootConfig *config) {
         unsigned i;
 
+        assert(config);
+
         free(config->default_pattern);
         free(config->timeout);
         free(config->editor);
@@ -140,6 +146,9 @@ int boot_loader_read_conf(const char *path, BootConfig *config) {
         unsigned line = 1;
         int r;
 
+        assert(path);
+        assert(config);
+
         f = fopen(path, "re");
         if (!f)
                 return log_error_errno(errno, "Failed to open \"%s\": %m", path);
@@ -251,14 +260,17 @@ static int boot_entry_compare(const void *a, const void *b) {
         return str_verscmp(aa->filename, bb->filename);
 }
 
-int boot_entries_find(const char *dir, BootEntry **entries, size_t *n_entries) {
+int boot_entries_find(const char *dir, BootEntry **ret_entries, size_t *ret_n_entries) {
         _cleanup_strv_free_ char **files = NULL;
         char **f;
         int r;
-
         BootEntry *array = NULL;
         size_t n_allocated = 0, n = 0;
 
+        assert(dir);
+        assert(ret_entries);
+        assert(ret_n_entries);
+
         r = conf_files_list(&files, ".conf", NULL, 0, dir, NULL);
         if (r < 0)
                 return log_error_errno(r, "Failed to list files in \"%s\": %m", dir);
@@ -276,8 +288,9 @@ int boot_entries_find(const char *dir, BootEntry **entries, size_t *n_entries) {
 
         qsort_safe(array, n, sizeof(BootEntry), boot_entry_compare);
 
-        *entries = array;
-        *n_entries = n;
+        *ret_entries = array;
+        *ret_n_entries = n;
+
         return 0;
 }
 
@@ -285,6 +298,9 @@ static bool find_nonunique(BootEntry *entries, size_t n_entries, bool *arr) {
         unsigned i, j;
         bool non_unique = false;
 
+        assert(entries || n_entries == 0);
+        assert(arr || n_entries == 0);
+
         for (i = 0; i < n_entries; i++)
                 arr[i] = false;
 
@@ -303,6 +319,8 @@ static int boot_entries_uniquify(BootEntry *entries, size_t n_entries) {
         int r;
         bool arr[n_entries];
 
+        assert(entries || n_entries == 0);
+
         /* Find _all_ non-unique titles */
         if (!find_nonunique(entries, n_entries, arr))
                 return 0;
@@ -349,6 +367,8 @@ static int boot_entries_uniquify(BootEntry *entries, size_t n_entries) {
 static int boot_entries_select_default(const BootConfig *config) {
         int i;
 
+        assert(config);
+
         if (config->entry_oneshot)
                 for (i = config->n_entries - 1; i >= 0; i--)
                         if (streq(config->entry_oneshot, config->entries[i].filename)) {
@@ -377,6 +397,7 @@ static int boot_entries_select_default(const BootConfig *config) {
                 log_debug("Found default: last entry \"%s\"", config->entries[config->n_entries - 1].filename);
         else
                 log_debug("Found no default boot entry :(");
+
         return config->n_entries - 1; /* -1 means "no default" */
 }
 
@@ -384,6 +405,9 @@ int boot_entries_load_config(const char *esp_path, BootConfig *config) {
         const char *p;
         int r;
 
+        assert(esp_path);
+        assert(config);
+
         p = strjoina(esp_path, "/loader/loader.conf");
         r = boot_loader_read_conf(p, config);
         if (r < 0)
@@ -413,8 +437,9 @@ int boot_entries_load_config(const char *esp_path, BootConfig *config) {
 /********************************************************************************/
 
 static int verify_esp(
-                bool searching,
                 const char *p,
+                bool searching,
+                bool unprivileged_mode,
                 uint32_t *ret_part,
                 uint64_t *ret_pstart,
                 uint64_t *ret_psize,
@@ -430,21 +455,19 @@ static int verify_esp(
         struct statfs sfs;
         sd_id128_t uuid = SD_ID128_NULL;
         uint32_t part = 0;
-        bool quiet;
         int r;
 
         assert(p);
 
-        /* Non-root user can only check the status, so if an error occured in the following,
-         * it does not cause any issues. Let's silence the error messages. */
-        quiet = geteuid() != 0;
+        /* Non-root user can only check the status, so if an error occured in the following, it does not cause any
+         * issues. Let's also, silence the error messages. */
 
         if (statfs(p, &sfs) < 0) {
                 /* If we are searching for the mount point, don't generate a log message if we can't find the path */
                 if (errno == ENOENT && searching)
                         return -ENOENT;
 
-                return log_full_errno(quiet && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
+                return log_full_errno(unprivileged_mode && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
                                       "Failed to check file system type of \"%s\": %m", p);
         }
 
@@ -457,7 +480,7 @@ static int verify_esp(
         }
 
         if (stat(p, &st) < 0)
-                return log_full_errno(quiet && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
+                return log_full_errno(unprivileged_mode && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
                                       "Failed to determine block device node of \"%s\": %m", p);
 
         if (major(st.st_dev) == 0) {
@@ -468,7 +491,7 @@ static int verify_esp(
         t2 = strjoina(p, "/..");
         r = stat(t2, &st2);
         if (r < 0)
-                return log_full_errno(quiet && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
+                return log_full_errno(unprivileged_mode && errno == EACCES ? LOG_DEBUG : LOG_ERR, errno,
                                       "Failed to determine block device node of parent of \"%s\": %m", p);
 
         if (st.st_dev == st2.st_dev) {
@@ -478,7 +501,7 @@ static int verify_esp(
 
         /* In a container we don't have access to block devices, skip this part of the verification, we trust the
          * container manager set everything up correctly on its own. Also skip the following verification for non-root user. */
-        if (detect_container() > 0 || geteuid() != 0)
+        if (detect_container() > 0 || unprivileged_mode)
                 goto finish;
 
 #if HAVE_BLKID
@@ -579,29 +602,53 @@ finish:
         return 0;
 }
 
-int find_esp(char **path,
-             uint32_t *part, uint64_t *pstart, uint64_t *psize, sd_id128_t *uuid) {
+int find_esp_and_warn(
+                const char *path,
+                bool unprivileged_mode,
+                char **ret_path,
+                uint32_t *ret_part,
+                uint64_t *ret_pstart,
+                uint64_t *ret_psize,
+                sd_id128_t *ret_uuid) {
 
-        const char *p;
         int r;
 
-        if (*path)
-                return verify_esp(false, *path, part, pstart, psize, uuid);
+        /* This logs about all errors except:
+         *
+         *    -ENOKEY → when we can't find the partition
+         *   -EACCESS → when unprivileged_mode is true, and we can't access something
+         */
 
-        FOREACH_STRING(p, "/efi", "/boot", "/boot/efi") {
-
-                r = verify_esp(true, p, part, pstart, psize, uuid);
-                if (IN_SET(r, -ENOENT, -EADDRNOTAVAIL)) /* This one is not it */
-                        continue;
+        if (path) {
+                r = verify_esp(path, false, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid);
                 if (r < 0)
                         return r;
 
-                *path = strdup(p);
-                if (!*path)
-                        return log_oom();
-
-                return 0;
+                goto found;
         }
 
-        return -ENOENT;
+        FOREACH_STRING(path, "/efi", "/boot", "/boot/efi") {
+
+                r = verify_esp(path, true, unprivileged_mode, ret_part, ret_pstart, ret_psize, ret_uuid);
+                if (r >= 0)
+                        goto found;
+                if (!IN_SET(r, -ENOENT, -EADDRNOTAVAIL)) /* This one is not it */
+                        return r;
+        }
+
+        /* No logging here */
+        return -ENOKEY;
+
+found:
+        if (ret_path) {
+                char *c;
+
+                c = strdup(path);
+                if (!c)
+                        return log_oom();
+
+                *ret_path = c;
+        }
+
+        return 0;
 }

src/shared/bootspec.h

@@ -61,5 +61,4 @@ static inline const char* boot_entry_title(const BootEntry *entry) {
         return entry->show_title ?: entry->title ?: entry->filename;
 }
 
-int find_esp(char **path,
-             uint32_t *part, uint64_t *pstart, uint64_t *psize, sd_id128_t *uuid);
+int find_esp_and_warn(const char *path, bool unprivileged_mode, char **ret_path, uint32_t *ret_part, uint64_t *ret_pstart, uint64_t *ret_psize, sd_id128_t *ret_uuid);

src/systemctl/systemctl.c

@@ -2074,11 +2074,6 @@ static int list_machines(int argc, char *argv[], void *userdata) {
         sd_bus *bus;
         int r;
 
-        if (geteuid() != 0) {
-                log_error("Must be root.");
-                return -EPERM;
-        }
-
         r = acquire_bus(BUS_MANAGER, &bus);
         if (r < 0)
                 return r;
@@ -3497,7 +3492,7 @@ static int prepare_firmware_setup(void) {
 
 static int load_kexec_kernel(void) {
         _cleanup_(boot_config_free) BootConfig config = {};
-        _cleanup_free_ char *kernel = NULL, *initrd = NULL, *options = NULL;
+        _cleanup_free_ char *where = NULL, *kernel = NULL, *initrd = NULL, *options = NULL;
         const BootEntry *e;
         pid_t pid;
         int r;
@@ -3507,14 +3502,15 @@ static int load_kexec_kernel(void) {
                 return 0;
         }
 
-        r = find_esp(&arg_esp_path, NULL, NULL, NULL, NULL);
-        if (r < 0)
-                return log_error_errno(r, "Cannot find the ESP partition mount point: %m");
+        r = find_esp_and_warn(arg_esp_path, false, &where, NULL, NULL, NULL, NULL);
+        if (r == -ENOKEY) /* find_esp_and_warn() doesn't warn about this case */
+                return log_error_errno(r, "Cannot find the ESP partition mount point.");
+        if (r < 0) /* But it logs about all these cases, hence don't log here again */
+                return r;
 
-        r = boot_entries_load_config(arg_esp_path, &config);
+        r = boot_entries_load_config(where, &config);
         if (r < 0)
-                return log_error_errno(r, "Failed to load bootspec config from \"%s/loader\": %m",
-                                       arg_esp_path);
+                return log_error_errno(r, "Failed to load bootspec config from \"%s/loader\": %m", where);
 
         if (config.default_entry < 0) {
                 log_error("No entry suitable as default, refusing to guess.");
@@ -3527,9 +3523,9 @@ static int load_kexec_kernel(void) {
                 return -EINVAL;
         }
 
-        kernel = path_join(NULL, arg_esp_path, e->kernel);
+        kernel = path_join(NULL, where, e->kernel);
         if (!strv_isempty(e->initrd))
-                initrd = path_join(NULL, arg_esp_path, *e->initrd);
+                initrd = path_join(NULL, where, *e->initrd);
         options = strv_join(e->options, " ");
         if (!options)
                 return log_oom();
@@ -3602,9 +3598,10 @@ static int start_special(int argc, char *argv[], void *userdata) {
         if (r < 0)
                 return r;
 
-        if (arg_force >= 2 && geteuid() != 0) {
-                log_error("Must be root.");
-                return -EPERM;
+        if (arg_force >= 2) {
+                r = must_be_root();
+                if (r < 0)
+                        return r;
         }
 
         r = prepare_firmware_setup();
@@ -8407,7 +8404,7 @@ static int systemctl_main(int argc, char *argv[]) {
                 { "list-sockets",          VERB_ANY, VERB_ANY, VERB_NOCHROOT, list_sockets         },
                 { "list-timers",           VERB_ANY, VERB_ANY, VERB_NOCHROOT, list_timers          },
                 { "list-jobs",             VERB_ANY, VERB_ANY, VERB_NOCHROOT, list_jobs            },
-                { "list-machines",         VERB_ANY, VERB_ANY, VERB_NOCHROOT, list_machines        },
+                { "list-machines",         VERB_ANY, VERB_ANY, VERB_NOCHROOT|VERB_MUSTBEROOT, list_machines },
                 { "clear-jobs",            VERB_ANY, 1,        VERB_NOCHROOT, trivial_method       },
                 { "cancel",                VERB_ANY, VERB_ANY, VERB_NOCHROOT, cancel_job           },
                 { "start",                 2,        VERB_ANY, VERB_NOCHROOT, start_unit           },
@@ -8637,7 +8634,7 @@ static int halt_main(void) {
 
         if (geteuid() != 0) {
                 if (arg_dry_run || arg_force > 0) {
-                        log_error("Must be root.");
+                        (void) must_be_root();
                         return -EPERM;
                 }
 

src/udev/udevadm-control.c

@@ -63,10 +63,8 @@ static int adm_control(struct udev *udev, int argc, char *argv[]) {
                 {}
         };
 
-        if (getuid() != 0) {
-                log_error("root privileges required");
+        if (must_be_root() < 0)
                 return 1;
-        }
 
         uctrl = udev_ctrl_new(udev);
         if (uctrl == NULL)

src/udev/udevd.c

@@ -1670,10 +1670,9 @@ int main(int argc, char *argv[]) {
                 log_set_max_level(LOG_DEBUG);
         }
 
-        if (getuid() != 0) {
-                r = log_error_errno(EPERM, "root privileges required");
+        r = must_be_root();
+        if (r < 0)
                 goto exit;
-        }
 
         if (arg_children_max == 0) {
                 cpu_set_t cpu_set;