From 4ac08d8ad6fb905ef8cccf7545304ac60a6c2d0d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 26 Jan 2024 18:41:24 +0100 Subject: [PATCH] tree-wide: port various things over to new pidref helpers THis not only mkaes a lot of code shorter, but also safer, as we pin the clients via a pidfd. --- src/core/dbus-manager.c | 59 ++++++----------- src/core/dbus-scope.c | 24 +++---- src/core/dbus-unit.c | 17 ++--- src/core/dbus.c | 30 ++++----- src/core/selinux-access.c | 2 +- src/login/logind-dbus.c | 132 ++++++++++++++++---------------------- 6 files changed, 108 insertions(+), 156 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 65524912613..31b2f1daef4 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -11,6 +11,7 @@ #include "bus-common-errors.h" #include "bus-get-properties.h" #include "bus-log-control-api.h" +#include "bus-util.h" #include "chase.h" #include "confidential-virt.h" #include "data-fd-util.h" @@ -464,18 +465,13 @@ static int bus_get_unit_by_name(Manager *m, sd_bus_message *message, const char * its sleeve: if the name is specified empty we use the client's unit. */ if (isempty(name)) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - pid_t pid; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + r = bus_query_sender_pidref(message, &pidref); if (r < 0) return r; - r = sd_bus_creds_get_pid(creds, &pid); - if (r < 0) - return r; - - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) return sd_bus_error_set(error, BUS_ERROR_NO_SUCH_UNIT, "Client not member of any unit."); } else { @@ -542,7 +538,7 @@ static int method_get_unit(sd_bus_message *message, void *userdata, sd_bus_error static int method_get_unit_by_pid(sd_bus_message *message, void *userdata, sd_bus_error *error) { Manager *m = ASSERT_PTR(userdata); - pid_t pid; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; Unit *u; int r; @@ -552,27 +548,20 @@ static int method_get_unit_by_pid(sd_bus_message *message, void *userdata, sd_bu /* Anyone can call this method */ - r = sd_bus_message_read(message, "u", &pid); + r = sd_bus_message_read(message, "u", &pidref.pid); if (r < 0) return r; - if (pid < 0) - return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PID " PID_FMT, pid); - - if (pid == 0) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); - if (r < 0) - return r; - - r = sd_bus_creds_get_pid(creds, &pid); + if (pidref.pid < 0) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PID " PID_FMT, pidref.pid); + if (pidref.pid == 0) { + r = bus_query_sender_pidref(message, &pidref); if (r < 0) return r; } - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) - return sd_bus_error_setf(error, BUS_ERROR_NO_UNIT_FOR_PID, "PID "PID_FMT" does not belong to any loaded unit.", pid); + return sd_bus_error_setf(error, BUS_ERROR_NO_UNIT_FOR_PID, "PID "PID_FMT" does not belong to any loaded unit.", pidref.pid); return reply_unit_path(u, message, error); } @@ -601,21 +590,16 @@ static int method_get_unit_by_invocation_id(sd_bus_message *message, void *userd return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid invocation ID"); if (sd_id128_is_null(id)) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - pid_t pid; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + r = bus_query_sender_pidref(message, &pidref); if (r < 0) return r; - r = sd_bus_creds_get_pid(creds, &pid); - if (r < 0) - return r; - - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, - "Client " PID_FMT " not member of any unit.", pid); + "Client " PID_FMT " not member of any unit.", pidref.pid); } else { u = hashmap_get(m->units_by_invocation_id, &id); if (!u) @@ -2934,7 +2918,7 @@ static int method_dump_unit_descriptor_store(sd_bus_message *message, void *user } static int aux_scope_from_message(Manager *m, sd_bus_message *message, Unit **ret_scope, sd_bus_error *error) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + _cleanup_(pidref_done) PidRef sender_pidref = PIDREF_NULL; _cleanup_free_ PidRef *pidrefs = NULL; const char *name; Unit *from, *scope; @@ -2942,20 +2926,15 @@ static int aux_scope_from_message(Manager *m, sd_bus_message *message, Unit **re CGroupContext *cc; size_t n_pids = 0; uint64_t flags; - pid_t pid; int r; assert(ret_scope); - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + r = bus_query_sender_pidref(message, &sender_pidref); if (r < 0) return r; - r = sd_bus_creds_get_pid(creds, &pid); - if (r < 0) - return r; - - from = manager_get_unit_by_pid(m, pid); + from = manager_get_unit_by_pidref(m, &sender_pidref); if (!from) return sd_bus_error_set(error, BUS_ERROR_NO_SUCH_UNIT, "Client not member of any unit."); diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c index 78196a1a08c..165aa65a7b9 100644 --- a/src/core/dbus-scope.c +++ b/src/core/dbus-scope.c @@ -3,6 +3,7 @@ #include "alloc-util.h" #include "bus-common-errors.h" #include "bus-get-properties.h" +#include "bus-util.h" #include "dbus-cgroup.h" #include "dbus-kill.h" #include "dbus-manager.h" @@ -84,7 +85,7 @@ static int bus_scope_set_transient_property( return bus_set_transient_oom_policy(u, name, &s->oom_policy, message, flags, error); if (streq(name, "PIDs")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + _cleanup_(pidref_done) PidRef sender_pidref = PIDREF_NULL; unsigned n = 0; r = sd_bus_message_enter_container(message, 'a', "u"); @@ -94,7 +95,7 @@ static int bus_scope_set_transient_property( for (;;) { _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; uint32_t upid; - pid_t pid; + PidRef *p; r = sd_bus_message_read(message, "u", &upid); if (r < 0) @@ -103,28 +104,27 @@ static int bus_scope_set_transient_property( break; if (upid == 0) { - if (!creds) { - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + if (!pidref_is_set(&sender_pidref)) { + r = bus_query_sender_pidref(message, &sender_pidref); if (r < 0) return r; } - r = sd_bus_creds_get_pid(creds, &pid); + p = &sender_pidref; + } else { + r = pidref_set_pid(&pidref, upid); if (r < 0) return r; - } else - pid = (uid_t) upid; - r = pidref_set_pid(&pidref, pid); - if (r < 0) - return r; + p = &pidref; + } - r = unit_pid_attachable(u, &pidref, error); + r = unit_pid_attachable(u, p, error); if (r < 0) return r; if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_watch_pidref(u, &pidref, /* exclusive= */ false); + r = unit_watch_pidref(u, p, /* exclusive= */ false); if (r < 0 && r != -EEXIST) return r; } diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 83d2302faad..4b017bff48e 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -7,6 +7,7 @@ #include "bus-common-errors.h" #include "bus-get-properties.h" #include "bus-polkit.h" +#include "bus-util.h" #include "cgroup-util.h" #include "condition.h" #include "dbus-job.h" @@ -1485,7 +1486,7 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(u))) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Unit is not active, refusing."); - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID, &creds); + r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID|SD_BUS_CREDS_PIDFD, &creds); if (r < 0) return r; @@ -1496,7 +1497,6 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd _cleanup_(pidref_freep) PidRef *pidref = NULL; uid_t process_uid, sender_uid; uint32_t upid; - pid_t pid; r = sd_bus_message_read(message, "u", &upid); if (r < 0) @@ -1505,13 +1505,14 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd break; if (upid == 0) { - r = sd_bus_creds_get_pid(creds, &pid); + _cleanup_(pidref_done) PidRef p = PIDREF_NULL; + r = bus_creds_get_pidref(creds, &p); if (r < 0) return r; - } else - pid = (uid_t) upid; - r = pidref_new_from_pid(pid, &pidref); + r = pidref_dup(&p, &pidref); + } else + r = pidref_new_from_pid(upid, &pidref); if (r < 0) return r; @@ -1537,9 +1538,9 @@ int bus_unit_method_attach_processes(sd_bus_message *message, void *userdata, sd return sd_bus_error_set_errnof(error, r, "Failed to retrieve process UID: %m"); if (process_uid != sender_uid) - return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by client's UID. Refusing.", pid); + return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by client's UID. Refusing.", pidref->pid); if (process_uid != u->ref_uid) - return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by target unit's UID. Refusing.", pid); + return sd_bus_error_setf(error, SD_BUS_ERROR_ACCESS_DENIED, "Process " PID_FMT " not owned by target unit's UID. Refusing.", pidref->pid); } r = set_ensure_consume(&pids, &pidref_hash_ops_free, TAKE_PTR(pidref)); diff --git a/src/core/dbus.c b/src/core/dbus.c index 49a80bbcfa9..a336b04daf3 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -232,6 +232,8 @@ static int mac_selinux_filter(sd_bus_message *message, void *userdata, sd_bus_er return 0; path = sd_bus_message_get_path(message); + if (!path) + return 0; if (object_path_startswith("/org/freedesktop/systemd1", path)) { r = mac_selinux_access_check(message, verb, error); @@ -241,25 +243,20 @@ static int mac_selinux_filter(sd_bus_message *message, void *userdata, sd_bus_er return 0; } - if (streq_ptr(path, "/org/freedesktop/systemd1/unit/self")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - pid_t pid; + if (streq(path, "/org/freedesktop/systemd1/unit/self")) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + r = bus_query_sender_pidref(message, &pidref); if (r < 0) return 0; - r = sd_bus_creds_get_pid(creds, &pid); - if (r < 0) - return 0; - - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); } else { r = manager_get_job_from_dbus_path(m, path, &j); if (r >= 0) u = j->unit; else - manager_load_unit_from_dbus_path(m, path, NULL, &u); + (void) manager_load_unit_from_dbus_path(m, path, NULL, &u); } if (!u) return 0; @@ -280,24 +277,19 @@ static int find_unit(Manager *m, sd_bus *bus, const char *path, Unit **unit, sd_ assert(bus); assert(path); - if (streq_ptr(path, "/org/freedesktop/systemd1/unit/self")) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; + if (streq(path, "/org/freedesktop/systemd1/unit/self")) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; sd_bus_message *message; - pid_t pid; message = sd_bus_get_current_message(bus); if (!message) return 0; - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); + r = bus_query_sender_pidref(message, &pidref); if (r < 0) return r; - r = sd_bus_creds_get_pid(creds, &pid); - if (r < 0) - return r; - - u = manager_get_unit_by_pid(m, pid); + u = manager_get_unit_by_pidref(m, &pidref); if (!u) return 0; } else { diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index 62181a6309b..1214bd8f22f 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -248,7 +248,7 @@ int mac_selinux_access_check_internal( tclass = "system"; } - sd_bus_creds_get_cmdline(creds, &cmdline); + (void) sd_bus_creds_get_cmdline(creds, &cmdline); cl = strv_join(cmdline, " "); struct audit_info audit_info = { diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 097b63246b5..199c7f9e9b8 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -759,8 +759,8 @@ static int create_session( void *userdata, sd_bus_error *error, uid_t uid, - pid_t pid, - int pidfd, + pid_t leader_pid, + int leader_pidfd, const char *service, const char *type, const char *class, @@ -793,32 +793,18 @@ static int create_session( if (flags != 0) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Flags must be zero."); - if (pidfd >= 0) { - r = pidref_set_pidfd(&leader, pidfd); - if (r < 0) - return r; - } else if (pid == 0) { - _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; - pid_t p; + if (leader_pidfd >= 0) + r = pidref_set_pidfd(&leader, leader_pidfd); + else if (leader_pid == 0) + r = bus_query_sender_pidref(message, &leader); + else { + if (leader_pid < 0) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Leader PID is not valid"); - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds); - if (r < 0) - return r; - - r = sd_bus_creds_get_pid(creds, &p); - if (r < 0) - return r; - - r = pidref_set_pid(&leader, p); - if (r < 0) - return r; - } else { - assert(pid > 0); - - r = pidref_set_pid(&leader, pid); - if (r < 0) - return r; + r = pidref_set_pid(&leader, leader_pid); } + if (r < 0) + return r; if (leader.pid == 1 || leader.pid == getpid_cached()) return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid leader PID"); @@ -1096,32 +1082,32 @@ fail: static int method_create_session(sd_bus_message *message, void *userdata, sd_bus_error *error) { const char *service, *type, *class, *cseat, *tty, *display, *remote_user, *remote_host, *desktop; - pid_t leader; + pid_t leader_pid; + uint32_t vtnr; uid_t uid; - int remote; - uint32_t vtnr = 0; - int r; + int remote, r; assert(message); assert_cc(sizeof(pid_t) == sizeof(uint32_t)); assert_cc(sizeof(uid_t) == sizeof(uint32_t)); - r = sd_bus_message_read(message, - "uusssssussbss", - &uid, - &leader, - &service, - &type, - &class, - &desktop, - &cseat, - &vtnr, - &tty, - &display, - &remote, - &remote_user, - &remote_host); + r = sd_bus_message_read( + message, + "uusssssussbss", + &uid, + &leader_pid, + &service, + &type, + &class, + &desktop, + &cseat, + &vtnr, + &tty, + &display, + &remote, + &remote_user, + &remote_host); if (r < 0) return r; @@ -1130,7 +1116,7 @@ static int method_create_session(sd_bus_message *message, void *userdata, sd_bus userdata, error, uid, - leader, + leader_pid, /* pidfd = */ -EBADF, service, type, @@ -1148,29 +1134,28 @@ static int method_create_session(sd_bus_message *message, void *userdata, sd_bus static int method_create_session_pidfd(sd_bus_message *message, void *userdata, sd_bus_error *error) { const char *service, *type, *class, *cseat, *tty, *display, *remote_user, *remote_host, *desktop; - int leaderfd = -EBADF; - uid_t uid; - int remote; - uint32_t vtnr = 0; uint64_t flags; - int r; + uint32_t vtnr; + uid_t uid; + int leader_fd = -EBADF, remote, r; - r = sd_bus_message_read(message, - "uhsssssussbsst", - &uid, - &leaderfd, - &service, - &type, - &class, - &desktop, - &cseat, - &vtnr, - &tty, - &display, - &remote, - &remote_user, - &remote_host, - &flags); + r = sd_bus_message_read( + message, + "uhsssssussbsst", + &uid, + &leader_fd, + &service, + &type, + &class, + &desktop, + &cseat, + &vtnr, + &tty, + &display, + &remote, + &remote_user, + &remote_host, + &flags); if (r < 0) return r; @@ -1179,8 +1164,8 @@ static int method_create_session_pidfd(sd_bus_message *message, void *userdata, userdata, error, uid, - /* pid = */ 0, - leaderfd, + /* leader_pid = */ 0, + leader_fd, service, type, class, @@ -3506,7 +3491,6 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error Manager *m = ASSERT_PTR(userdata); InhibitMode mm; InhibitWhat w; - pid_t pid; uid_t uid; int r; @@ -3557,7 +3541,7 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID, &creds); + r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID|SD_BUS_CREDS_PIDFD, &creds); if (r < 0) return r; @@ -3565,14 +3549,10 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error if (r < 0) return r; - r = sd_bus_creds_get_pid(creds, &pid); + r = bus_creds_get_pidref(creds, &pidref); if (r < 0) return r; - r = pidref_set_pid(&pidref, pid); - if (r < 0) - return sd_bus_error_set_errnof(error, r, "Failed pin source process "PID_FMT": %m", pid); - if (hashmap_size(m->inhibitors) >= m->inhibitors_max) return sd_bus_error_setf(error, SD_BUS_ERROR_LIMITS_EXCEEDED, "Maximum number of inhibitors (%" PRIu64 ") reached, refusing further inhibitors.",