From f14f1806e329fe92d01f15c22a384702f0cb4ae0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 Jan 2018 19:44:27 +0100 Subject: [PATCH 01/14] fs-util: add new CHASE_SAFE flag to chase_symlinks() When the flag is specified we won't transition to a privilege-owned file or directory from an unprivileged-owned one. This is useful when privileged code wants to load data from a file unprivileged users have write access to, and validates the ownership, but want's to make sure that no symlink games are played to read a root-owned system file belonging to a different context. --- src/basic/fs-util.c | 43 +++++++++++++++++++++++++++++++++++++++++ src/basic/fs-util.h | 7 ++++--- src/test/test-fs-util.c | 27 ++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 16f97893c9..a7ee785202 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -610,10 +610,22 @@ int inotify_add_watch_fd(int fd, int what, uint32_t mask) { return r; } +static bool safe_transition(const struct stat *a, const struct stat *b) { + /* Returns true if the transition from a to b is safe, i.e. that we never transition from unprivileged to + * privileged files or directories. Why bother? So that unprivileged code can't symlink to privileged files + * making us believe we read something safe even though it isn't safe in the specific context we open it in. */ + + if (a->st_uid == 0) /* Transitioning from privileged to unprivileged is always fine */ + return true; + + return a->st_uid == b->st_uid; /* Otherwise we need to stay within the same UID */ +} + int chase_symlinks(const char *path, const char *original_root, unsigned flags, char **ret) { _cleanup_free_ char *buffer = NULL, *done = NULL, *root = NULL; _cleanup_close_ int fd = -1; unsigned max_follow = 32; /* how many symlinks to follow before giving up and returning ELOOP */ + struct stat previous_stat; bool exists = true; char *todo; int r; @@ -657,6 +669,11 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fd < 0) return -errno; + if (flags & CHASE_SAFE) { + if (fstat(fd, &previous_stat) < 0) + return -errno; + } + todo = buffer; for (;;) { _cleanup_free_ char *first = NULL; @@ -718,6 +735,16 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fd_parent < 0) return -errno; + if (flags & CHASE_SAFE) { + if (fstat(fd_parent, &st) < 0) + return -errno; + + if (!safe_transition(&previous_stat, &st)) + return -EPERM; + + previous_stat = st; + } + safe_close(fd); fd = fd_parent; @@ -752,6 +779,12 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, if (fstat(child, &st) < 0) return -errno; + if ((flags & CHASE_SAFE) && + !safe_transition(&previous_stat, &st)) + return -EPERM; + + previous_stat = st; + if ((flags & CHASE_NO_AUTOFS) && fd_is_fs_type(child, AUTOFS_SUPER_MAGIC) > 0) return -EREMOTE; @@ -784,6 +817,16 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, free(done); + if (flags & CHASE_SAFE) { + if (fstat(fd, &st) < 0) + return -errno; + + if (!safe_transition(&previous_stat, &st)) + return -EPERM; + + previous_stat = st; + } + /* Note that we do not revalidate the root, we take it as is. */ if (isempty(root)) done = NULL; diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 82a5b2028d..182dc63547 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -81,9 +81,10 @@ union inotify_event_buffer { int inotify_add_watch_fd(int fd, int what, uint32_t mask); enum { - CHASE_PREFIX_ROOT = 1, /* If set, the specified path will be prefixed by the specified root before beginning the iteration */ - CHASE_NONEXISTENT = 2, /* If set, it's OK if the path doesn't actually exist. */ - CHASE_NO_AUTOFS = 4, /* If set, return -EREMOTE if autofs mount point found */ + CHASE_PREFIX_ROOT = 1U << 0, /* If set, the specified path will be prefixed by the specified root before beginning the iteration */ + CHASE_NONEXISTENT = 1U << 1, /* If set, it's OK if the path doesn't actually exist. */ + CHASE_NO_AUTOFS = 1U << 2, /* If set, return -EREMOTE if autofs mount point found */ + CHASE_SAFE = 1U << 3, /* If set, return EPERM if we ever traverse from unprivileged to privileged files or directories */ }; int chase_symlinks(const char *path_with_prefix, const char *root, unsigned flags, char **ret); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index 2ad9e71856..b5ec086d37 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -30,6 +30,7 @@ #include "rm-rf.h" #include "string-util.h" #include "strv.h" +#include "user-util.h" #include "util.h" static void test_chase_symlinks(void) { @@ -235,6 +236,32 @@ static void test_chase_symlinks(void) { r = chase_symlinks(p, NULL, 0, &result); assert_se(r == -ENOENT); + if (geteuid() == 0) { + p = strjoina(temp, "/priv1"); + assert_se(mkdir(p, 0755) >= 0); + + q = strjoina(p, "/priv2"); + assert_se(mkdir(q, 0755) >= 0); + + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); + + assert_se(chown(q, UID_NOBODY, GID_NOBODY) >= 0); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); + + assert_se(chown(p, UID_NOBODY, GID_NOBODY) >= 0); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); + + assert_se(chown(q, 0, 0) >= 0); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -EPERM); + + assert_se(rmdir(q) >= 0); + assert_se(symlink("/etc/passwd", q) >= 0); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) == -EPERM); + + assert_se(chown(p, 0, 0) >= 0); + assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); + } + assert_se(rm_rf(temp, REMOVE_ROOT|REMOVE_PHYSICAL) >= 0); } From 1ed34d75d4f21d2335c5625261954c848d176ae6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 Jan 2018 20:00:28 +0100 Subject: [PATCH 02/14] fs-util: add new chase_symlinks() flag CHASE_OPEN The new flag returns the O_PATH fd of the final component, which may be converted into a proper fd by open()ing it again through the /proc/self/fd/xyz path. Together with O_SAFE this provides us with a somewhat safe way to open() files in directories potentially owned by unprivileged code, where we want to refuse operation if any symlink tricks are played pointing to privileged files. --- src/basic/fs-util.c | 17 +++++++++++++++++ src/basic/fs-util.h | 1 + src/test/test-fs-util.c | 28 +++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index a7ee785202..7b76f33683 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -632,6 +632,10 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, assert(path); + /* Either the file may be missing, or we return an fd to the final object, but both make no sense */ + if ((flags & (CHASE_NONEXISTENT|CHASE_OPEN)) == (CHASE_NONEXISTENT|CHASE_OPEN)) + return -EINVAL; + /* This is a lot like canonicalize_file_name(), but takes an additional "root" parameter, that allows following * symlinks relative to a root directory, instead of the root of the host. * @@ -881,6 +885,19 @@ int chase_symlinks(const char *path, const char *original_root, unsigned flags, done = NULL; } + if (flags & CHASE_OPEN) { + int q; + + /* Return the O_PATH fd we currently are looking to the caller. It can translate it to a proper fd by + * opening /proc/self/fd/xyz. */ + + assert(fd >= 0); + q = fd; + fd = -1; + + return q; + } + return exists; } diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 182dc63547..4dba1ea56a 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -85,6 +85,7 @@ enum { CHASE_NONEXISTENT = 1U << 1, /* If set, it's OK if the path doesn't actually exist. */ CHASE_NO_AUTOFS = 1U << 2, /* If set, return -EREMOTE if autofs mount point found */ CHASE_SAFE = 1U << 3, /* If set, return EPERM if we ever traverse from unprivileged to privileged files or directories */ + CHASE_OPEN = 1U << 4, /* If set, return an O_PATH object to the final component */ }; int chase_symlinks(const char *path_with_prefix, const char *root, unsigned flags, char **ret); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index b5ec086d37..cda6d9cc72 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -22,12 +22,15 @@ #include "alloc-util.h" #include "fd-util.h" +#include "fd-util.h" #include "fileio.h" #include "fs-util.h" +#include "id128-util.h" #include "macro.h" #include "mkdir.h" #include "path-util.h" #include "rm-rf.h" +#include "stdio-util.h" #include "string-util.h" #include "strv.h" #include "user-util.h" @@ -37,7 +40,7 @@ static void test_chase_symlinks(void) { _cleanup_free_ char *result = NULL; char temp[] = "/tmp/test-chase.XXXXXX"; const char *top, *p, *pslash, *q, *qslash; - int r; + int r, pfd; assert_se(mkdtemp(temp)); @@ -262,6 +265,29 @@ static void test_chase_symlinks(void) { assert_se(chase_symlinks(q, NULL, CHASE_SAFE, NULL) >= 0); } + p = strjoina(temp, "/machine-id-test"); + assert_se(symlink("/usr/../etc/./machine-id", p) >= 0); + + pfd = chase_symlinks(p, NULL, CHASE_OPEN, NULL); + if (pfd != -ENOENT) { + char procfs[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(pfd) + 1]; + _cleanup_close_ int fd = -1; + sd_id128_t a, b; + + assert_se(pfd >= 0); + + xsprintf(procfs, "/proc/self/fd/%i", pfd); + + fd = open(procfs, O_RDONLY|O_CLOEXEC); + assert_se(fd >= 0); + + safe_close(pfd); + + assert_se(id128_read_fd(fd, ID128_PLAIN, &a) >= 0); + assert_se(sd_id128_get_machine(&b) >= 0); + assert_se(sd_id128_equal(a, b)); + } + assert_se(rm_rf(temp, REMOVE_ROOT|REMOVE_PHYSICAL) >= 0); } From da5fb86100250cb45a00b2742c25b0d1a4c636c2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 4 Jan 2018 21:00:10 +0100 Subject: [PATCH 03/14] manager: swap order in which we ellipsize/escape sd_notify() messages for debugging If we have to chose between truncated escape sequences and strings exploded to 4 times the desried length by fully escaping, prefer the latter. It's for debug only, hence doesn't really matter much. --- src/core/manager.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index a9c4330897..07809fea9c 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1897,9 +1897,10 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const else if (DEBUG_LOGGING) { _cleanup_free_ char *x = NULL, *y = NULL; - x = cescape(buf); + x = ellipsize(buf, 20, 90); if (x) - y = ellipsize(x, 20, 90); + y = cescape(x); + log_unit_debug(u, "Got notification message \"%s\", ignoring.", strnull(y)); } } From d6552eaa6c84da09a1bba49bd99f6df40e8ec15e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 12:17:35 +0100 Subject: [PATCH 04/14] dbus-util: properly parse timeout values This makes transient TimeoutStopSec= properties work. After all they are 64bit entitites, not 32bit ones. --- src/core/dbus-util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/dbus-util.c b/src/core/dbus-util.c index ed83a0e5f4..75bbd07604 100644 --- a/src/core/dbus-util.c +++ b/src/core/dbus-util.c @@ -93,12 +93,12 @@ int bus_set_transient_usec_internal( UnitWriteFlags flags, sd_bus_error *error) { - usec_t v; + uint64_t v; int r; assert(p); - r = sd_bus_message_read(message, "u", &v); + r = sd_bus_message_read(message, "t", &v); if (r < 0) return r; @@ -106,7 +106,7 @@ int bus_set_transient_usec_internal( char *n, ts[FORMAT_TIMESPAN_MAX]; if (fix_0) - *p = v ?: USEC_INFINITY; + *p = v != 0 ? v: USEC_INFINITY; else *p = v; From 007e4b5490d258e9fccd8e751b258ef848f7f861 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 12:18:32 +0100 Subject: [PATCH 05/14] manager: make use of NEWLINE macro where appropriate --- src/core/manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/manager.c b/src/core/manager.c index 07809fea9c..6f50b177e0 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1886,7 +1886,7 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const assert(u); assert(buf); - tags = strv_split(buf, "\n\r"); + tags = strv_split(buf, NEWLINE); if (!tags) { log_oom(); return; From 15e23e8cdf19ecb2a253d12fd53c88856a529f7d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 12:19:22 +0100 Subject: [PATCH 06/14] manager: make use of pid_is_valid() where appropriate --- src/core/manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/manager.c b/src/core/manager.c index 6f50b177e0..35cd9ee879 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1977,7 +1977,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t } } - if (!ucred || ucred->pid <= 0) { + if (!ucred || !pid_is_valid(ucred->pid)) { log_warning("Received notify message without valid credentials. Ignoring."); return 0; } From 9e1d021ee3f147486c5cfac69b3cbf6f4b36eb79 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:24:58 +0100 Subject: [PATCH 07/14] sd-dameon: also sent ucred when our UID differs from EUID Let's be explicit, and always send the messages from our UID and never our EUID. Previously this behaviour was conditionalized only on whether the PID was specified, which made this non-obvious. --- src/libsystemd/sd-daemon/sd-daemon.c | 29 ++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/libsystemd/sd-daemon/sd-daemon.c b/src/libsystemd/sd-daemon/sd-daemon.c index ba383e0b3b..83ff7d4277 100644 --- a/src/libsystemd/sd-daemon/sd-daemon.c +++ b/src/libsystemd/sd-daemon/sd-daemon.c @@ -455,7 +455,13 @@ _public_ int sd_is_mq(int fd, const char *path) { return 1; } -_public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char *state, const int *fds, unsigned n_fds) { +_public_ int sd_pid_notify_with_fds( + pid_t pid, + int unset_environment, + const char *state, + const int *fds, + unsigned n_fds) { + union sockaddr_union sockaddr = { .sa.sa_family = AF_UNIX, }; @@ -470,7 +476,7 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char _cleanup_close_ int fd = -1; struct cmsghdr *cmsg = NULL; const char *e; - bool have_pid; + bool send_ucred; int r; if (!state) { @@ -504,7 +510,7 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char goto finish; } - fd_inc_sndbuf(fd, SNDBUF_SIZE); + (void) fd_inc_sndbuf(fd, SNDBUF_SIZE); iovec.iov_len = strlen(state); @@ -514,13 +520,16 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char msghdr.msg_namelen = SOCKADDR_UN_LEN(sockaddr.un); - have_pid = pid != 0 && pid != getpid_cached(); + send_ucred = + (pid != 0 && pid != getpid_cached()) || + getuid() != geteuid() || + getgid() != getegid(); - if (n_fds > 0 || have_pid) { + if (n_fds > 0 || send_ucred) { /* CMSG_SPACE(0) may return value different than zero, which results in miscalculated controllen. */ msghdr.msg_controllen = (n_fds > 0 ? CMSG_SPACE(sizeof(int) * n_fds) : 0) + - (have_pid ? CMSG_SPACE(sizeof(struct ucred)) : 0); + (send_ucred ? CMSG_SPACE(sizeof(struct ucred)) : 0); msghdr.msg_control = alloca0(msghdr.msg_controllen); @@ -532,11 +541,11 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char memcpy(CMSG_DATA(cmsg), fds, sizeof(int) * n_fds); - if (have_pid) + if (send_ucred) assert_se(cmsg = CMSG_NXTHDR(&msghdr, cmsg)); } - if (have_pid) { + if (send_ucred) { struct ucred *ucred; cmsg->cmsg_level = SOL_SOCKET; @@ -544,7 +553,7 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char cmsg->cmsg_len = CMSG_LEN(sizeof(struct ucred)); ucred = (struct ucred*) CMSG_DATA(cmsg); - ucred->pid = pid; + ucred->pid = pid != 0 ? pid : getpid_cached(); ucred->uid = getuid(); ucred->gid = getgid(); } @@ -557,7 +566,7 @@ _public_ int sd_pid_notify_with_fds(pid_t pid, int unset_environment, const char } /* If that failed, try with our own ucred instead */ - if (have_pid) { + if (send_ucred) { msghdr.msg_controllen -= CMSG_SPACE(sizeof(struct ucred)); if (msghdr.msg_controllen == 0) msghdr.msg_control = NULL; From 65c6b99094580afa186199d8091cd7536900526c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:26:38 +0100 Subject: [PATCH 08/14] notify: add new --uid= command The new --uid= switch allows selecting the UID from which the notificaiton messages shall originate. This is primarily useful for testing purposes, but might have other uses. --- man/systemd-notify.xml | 9 +++++++++ src/notify/notify.c | 38 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/man/systemd-notify.xml b/man/systemd-notify.xml index acd4ee41fc..874bdd882b 100644 --- a/man/systemd-notify.xml +++ b/man/systemd-notify.xml @@ -122,6 +122,15 @@ sd_notify3. + + USER + + Set the user ID to send the notification from. Takes a UNIX user name or numeric UID. When + specified the notification message will be sent with the specified UID as sender, in place of the user the + command was invoked as. This option requires sufficient privileges in order to be able manipulate the user + identity of the process. + + diff --git a/src/notify/notify.c b/src/notify/notify.c index 4a34e1e9db..d58a45cdd2 100644 --- a/src/notify/notify.c +++ b/src/notify/notify.c @@ -33,12 +33,15 @@ #include "parse-util.h" #include "string-util.h" #include "strv.h" +#include "user-util.h" #include "util.h" static bool arg_ready = false; static pid_t arg_pid = 0; static const char *arg_status = NULL; static bool arg_booted = false; +static uid_t arg_uid = UID_INVALID; +static gid_t arg_gid = GID_INVALID; static void help(void) { printf("%s [OPTIONS...] [VARIABLE=VALUE...]\n\n" @@ -46,7 +49,8 @@ static void help(void) { " -h --help Show this help\n" " --version Show package version\n" " --ready Inform the init system about service start-up completion\n" - " --pid[=PID] Set main pid of daemon\n" + " --pid[=PID] Set main PID of daemon\n" + " --uid=USER Set user to send from\n" " --status=TEXT Set status text\n" " --booted Check if the system was booted up with systemd\n", program_invocation_short_name); @@ -60,6 +64,7 @@ static int parse_argv(int argc, char *argv[]) { ARG_PID, ARG_STATUS, ARG_BOOTED, + ARG_UID, }; static const struct option options[] = { @@ -69,10 +74,11 @@ static int parse_argv(int argc, char *argv[]) { { "pid", optional_argument, NULL, ARG_PID }, { "status", required_argument, NULL, ARG_STATUS }, { "booted", no_argument, NULL, ARG_BOOTED }, + { "uid", required_argument, NULL, ARG_UID }, {} }; - int c; + int c, r; assert(argc >= 0); assert(argv); @@ -112,6 +118,18 @@ static int parse_argv(int argc, char *argv[]) { arg_booted = true; break; + case ARG_UID: { + const char *u = optarg; + + r = get_user_creds(&u, &arg_uid, &arg_gid, NULL, NULL); + if (r == -ESRCH) /* If the user doesn't exist, then accept it anyway as numeric */ + r = parse_uid(u, &arg_uid); + if (r < 0) + return log_error_errno(r, "Can't resolve user %s: %m", optarg); + + break; + } + case '?': return -EINVAL; @@ -190,6 +208,22 @@ int main(int argc, char* argv[]) { goto finish; } + /* If this is requested change to the requested UID/GID. Note thta we only change the real UID here, and leave + the effective UID in effect (which is 0 for this to work). That's because we want the privileges to fake the + ucred data, and sd_pid_notify() uses the real UID for filling in ucred. */ + + if (arg_gid != GID_INVALID) + if (setregid(arg_gid, (gid_t) -1) < 0) { + r = log_error_errno(errno, "Failed to change GID: %m"); + goto finish; + } + + if (arg_uid != UID_INVALID) + if (setreuid(arg_uid, (uid_t) -1) < 0) { + r = log_error_errno(errno, "Failed to change UID: %m"); + goto finish; + } + r = sd_pid_notify(arg_pid ? arg_pid : getppid(), false, n); if (r < 0) { log_error_errno(r, "Failed to notify init system: %m"); From db256aab13d8a89d583ecd2bacf0aca87c66effc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 12:20:22 +0100 Subject: [PATCH 09/14] core: be stricter when handling PID files and MAINPID sd_notify() messages Let's be more restrictive when validating PID files and MAINPID= messages: don't accept PIDs that make no sense, and if the configuration source is not trusted, don't accept out-of-cgroup PIDs. A configuratin source is considered trusted when the PID file is owned by root, or the message was received from root. This should lock things down a bit, in case service authors write out PID files from unprivileged code or use NotifyAccess=all with unprivileged code. Note that doing so was always problematic, just now it's a bit less problematic. When we open the PID file we'll now use the CHASE_SAFE chase_symlinks() logic, to ensure that we won't follow an unpriviled-owned symlink to a privileged-owned file thinking this was a valid privileged PID file, even though it really isn't. Fixes: #6632 --- man/systemd.service.xml | 18 ++-- src/core/manager.c | 17 ++- src/core/service.c | 139 ++++++++++++++++++------ src/core/unit.h | 2 +- test/TEST-20-MAINPIDGAMES/Makefile | 4 + test/TEST-20-MAINPIDGAMES/test.sh | 42 ++++++++ test/TEST-20-MAINPIDGAMES/testsuite.sh | 141 +++++++++++++++++++++++++ test/test-functions | 2 +- 8 files changed, 314 insertions(+), 51 deletions(-) create mode 100644 test/TEST-20-MAINPIDGAMES/Makefile create mode 100755 test/TEST-20-MAINPIDGAMES/test.sh create mode 100755 test/TEST-20-MAINPIDGAMES/testsuite.sh diff --git a/man/systemd.service.xml b/man/systemd.service.xml index b98264c986..e796245ae5 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -264,16 +264,14 @@ PIDFile= - Takes an absolute filename pointing to the - PID file of this daemon. Use of this option is recommended for - services where Type= is set to - . systemd will read the PID of the - main process of the daemon after start-up of the service. - systemd will not write to the file configured here, although - it will remove the file after the service has shut down if it - still exists. - - + Takes an absolute path referring to the PID file of the service. Usage of this option is + recommended for services where Type= is set to . The service manager + will read the PID of the main process of the service from this file after start-up of the service. The service + manager will not write to the file configured here, although it will remove the file after the service has shut + down if it still exists. The PID file does not need to be owned by a privileged user, but if it is owned by an + unprivileged user additional safety restrictions are enforced: the file may not be a symlink to a file owned by + a different user (neither directly nor indirectly), and the PID file must refer to a process already belonging + to the service. diff --git a/src/core/manager.c b/src/core/manager.c index 35cd9ee879..f9222020eb 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1879,11 +1879,18 @@ static int manager_dispatch_cgroups_agent_fd(sd_event_source *source, int fd, ui return 0; } -static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const char *buf, FDSet *fds) { +static void manager_invoke_notify_message( + Manager *m, + Unit *u, + const struct ucred *ucred, + const char *buf, + FDSet *fds) { + _cleanup_strv_free_ char **tags = NULL; assert(m); assert(u); + assert(ucred); assert(buf); tags = strv_split(buf, NEWLINE); @@ -1893,7 +1900,7 @@ static void manager_invoke_notify_message(Manager *m, Unit *u, pid_t pid, const } if (UNIT_VTABLE(u)->notify_message) - UNIT_VTABLE(u)->notify_message(u, pid, tags, fds); + UNIT_VTABLE(u)->notify_message(u, ucred, tags, fds); else if (DEBUG_LOGGING) { _cleanup_free_ char *x = NULL, *y = NULL; @@ -2001,15 +2008,15 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t * to avoid notifying the same one multiple times. */ u1 = manager_get_unit_by_pid_cgroup(m, ucred->pid); if (u1) - manager_invoke_notify_message(m, u1, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u1, ucred, buf, fds); u2 = hashmap_get(m->watch_pids1, PID_TO_PTR(ucred->pid)); if (u2 && u2 != u1) - manager_invoke_notify_message(m, u2, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u2, ucred, buf, fds); u3 = hashmap_get(m->watch_pids2, PID_TO_PTR(ucred->pid)); if (u3 && u3 != u2 && u3 != u1) - manager_invoke_notify_message(m, u3, ucred->pid, buf, fds); + manager_invoke_notify_message(m, u3, ucred, buf, fds); if (!u1 && !u2 && !u3) log_warning("Cannot find unit for notify message of PID "PID_FMT".", ucred->pid); diff --git a/src/core/service.c b/src/core/service.c index c6835a4d6c..fde6406b31 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -866,9 +866,45 @@ static void service_dump(Unit *u, FILE *f, const char *prefix) { cgroup_context_dump(&s->cgroup_context, f, prefix); } +static int service_is_suitable_main_pid(Service *s, pid_t pid, int prio) { + Unit *owner; + + assert(s); + assert(pid_is_valid(pid)); + + /* Checks whether the specified PID is suitable as main PID for this service. returns negative if not, 0 if the + * PID is questionnable but should be accepted if the source of configuration is trusted. > 0 if the PID is + * good */ + + if (pid == getpid_cached() || pid == 1) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" is the manager, refusing.", pid); + return -EPERM; + } + + if (pid == s->control_pid) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" is the control process, refusing.", pid); + return -EPERM; + } + + if (!pid_is_alive(pid)) { + log_unit_full(UNIT(s), prio, 0, "New main PID "PID_FMT" does not exist or is a zombie.", pid); + return -ESRCH; + } + + owner = manager_get_unit_by_pid(UNIT(s)->manager, pid); + if (owner == UNIT(s)) { + log_unit_debug(UNIT(s), "New main PID "PID_FMT" belongs to service, we are happy.", pid); + return 1; /* Yay, it's definitely a good PID */ + } + + return 0; /* Hmm it's a suspicious PID, let's accept it if configuration source is trusted */ +} + static int service_load_pid_file(Service *s, bool may_warn) { + char procfs[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; _cleanup_free_ char *k = NULL; - int r; + _cleanup_close_ int fd = -1; + int r, prio; pid_t pid; assert(s); @@ -876,30 +912,47 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (!s->pid_file) return -ENOENT; - r = read_one_line_file(s->pid_file, &k); - if (r < 0) { - if (may_warn) - log_unit_info_errno(UNIT(s), r, "PID file %s not readable (yet?) after %s: %m", s->pid_file, service_state_to_string(s->state)); - return r; - } + prio = may_warn ? LOG_INFO : LOG_DEBUG; + + fd = chase_symlinks(s->pid_file, NULL, CHASE_OPEN|CHASE_SAFE, NULL); + if (fd == -EPERM) + return log_unit_full(UNIT(s), prio, fd, "Permission denied while opening PID file or unsafe symlink chain: %s", s->pid_file); + if (fd < 0) + return log_unit_full(UNIT(s), prio, fd, "Can't open PID file %s (yet?) after %s: %m", s->pid_file, service_state_to_string(s->state)); + + /* Let's read the PID file now that we chased it down. But we need to convert the O_PATH fd chase_symlinks() returned us into a proper fd first. */ + xsprintf(procfs, "/proc/self/fd/%i", fd); + r = read_one_line_file(procfs, &k); + if (r < 0) + return log_unit_error_errno(UNIT(s), r, "Can't convert PID files %s O_PATH file descriptor to proper file descriptor: %m", s->pid_file); r = parse_pid(k, &pid); - if (r < 0) { - if (may_warn) - log_unit_info_errno(UNIT(s), r, "Failed to read PID from file %s: %m", s->pid_file); - return r; - } + if (r < 0) + return log_unit_full(UNIT(s), prio, r, "Failed to parse PID from file %s: %m", s->pid_file); - if (!pid_is_alive(pid)) { - if (may_warn) - log_unit_info(UNIT(s), "PID "PID_FMT" read from file %s does not exist or is a zombie.", pid, s->pid_file); - return -ESRCH; + if (s->main_pid_known && pid == s->main_pid) + return 0; + + r = service_is_suitable_main_pid(s, pid, prio); + if (r < 0) + return r; + if (r == 0) { + struct stat st; + + /* Hmm, it's not clear if the new main PID is safe. Let's allow this if the PID file is owned by root */ + + if (fstat(fd, &st) < 0) + return log_unit_error_errno(UNIT(s), errno, "Failed to fstat() PID file O_PATH fd: %m"); + + if (st.st_uid != 0) { + log_unit_error(UNIT(s), "New main PID "PID_FMT" does not belong to service, and PID file is not owned by root. Refusing.", pid); + return -EPERM; + } + + log_unit_debug(UNIT(s), "New main PID "PID_FMT" does not belong to service, but we'll accept it since PID file is owned by root.", pid); } if (s->main_pid_known) { - if (pid == s->main_pid) - return 0; - log_unit_debug(UNIT(s), "Main PID changing: "PID_FMT" -> "PID_FMT, s->main_pid, pid); service_unwatch_main_pid(s); @@ -915,7 +968,7 @@ static int service_load_pid_file(Service *s, bool may_warn) { if (r < 0) /* FIXME: we need to do something here */ return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", pid); - return 0; + return 1; } static void service_search_main_pid(Service *s) { @@ -2981,7 +3034,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { /* Forking services may occasionally move to a new PID. * As long as they update the PID file before exiting the old * PID, they're fine. */ - if (service_load_pid_file(s, false) == 0) + if (service_load_pid_file(s, false) > 0) return; s->main_pid = 0; @@ -3406,37 +3459,55 @@ static bool service_notify_message_authorized(Service *s, pid_t pid, char **tags return true; } -static void service_notify_message(Unit *u, pid_t pid, char **tags, FDSet *fds) { +static void service_notify_message( + Unit *u, + const struct ucred *ucred, + char **tags, + FDSet *fds) { + Service *s = SERVICE(u); bool notify_dbus = false; const char *e; char **i; + int r; assert(u); + assert(ucred); - if (!service_notify_message_authorized(SERVICE(u), pid, tags, fds)) + if (!service_notify_message_authorized(SERVICE(u), ucred->pid, tags, fds)) return; if (DEBUG_LOGGING) { _cleanup_free_ char *cc = NULL; cc = strv_join(tags, ", "); - log_unit_debug(u, "Got notification message from PID "PID_FMT" (%s)", pid, isempty(cc) ? "n/a" : cc); + log_unit_debug(u, "Got notification message from PID "PID_FMT" (%s)", ucred->pid, isempty(cc) ? "n/a" : cc); } /* Interpret MAINPID= */ e = strv_find_startswith(tags, "MAINPID="); if (e && IN_SET(s->state, SERVICE_START, SERVICE_START_POST, SERVICE_RUNNING, SERVICE_RELOAD)) { - if (parse_pid(e, &pid) < 0) - log_unit_warning(u, "Failed to parse MAINPID= field in notification message: %s", e); - else if (pid == s->control_pid) - log_unit_warning(u, "A control process cannot also be the main process"); - else if (pid == getpid_cached() || pid == 1) - log_unit_warning(u, "Service manager can't be main process, ignoring sd_notify() MAINPID= field"); - else if (pid != s->main_pid) { - service_set_main_pid(s, pid); - unit_watch_pid(UNIT(s), pid); - notify_dbus = true; + pid_t new_main_pid; + + if (parse_pid(e, &new_main_pid) < 0) + log_unit_warning(u, "Failed to parse MAINPID= field in notification message, ignoring: %s", e); + else if (!s->main_pid_known || new_main_pid != s->main_pid) { + + r = service_is_suitable_main_pid(s, new_main_pid, LOG_WARNING); + if (r == 0) { + /* The new main PID is a bit suspicous, which is OK if the sender is privileged. */ + + if (ucred->uid == 0) { + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, but we'll accept it as the request to change it came from a privileged process.", new_main_pid); + r = 1; + } else + log_unit_debug(u, "New main PID "PID_FMT" does not belong to service, refusing.", new_main_pid); + } + if (r > 0) { + service_set_main_pid(s, new_main_pid); + unit_watch_pid(UNIT(s), new_main_pid); + notify_dbus = true; + } } } diff --git a/src/core/unit.h b/src/core/unit.h index 80585ac1c8..f8a12947cf 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -506,7 +506,7 @@ struct UnitVTable { void (*notify_cgroup_empty)(Unit *u); /* Called whenever a process of this unit sends us a message */ - void (*notify_message)(Unit *u, pid_t pid, char **tags, FDSet *fds); + void (*notify_message)(Unit *u, const struct ucred *ucred, char **tags, FDSet *fds); /* Called whenever a name this Unit registered for comes or goes away. */ void (*bus_name_owner_change)(Unit *u, const char *name, const char *old_owner, const char *new_owner); diff --git a/test/TEST-20-MAINPIDGAMES/Makefile b/test/TEST-20-MAINPIDGAMES/Makefile new file mode 100644 index 0000000000..34d7cc6cdf --- /dev/null +++ b/test/TEST-20-MAINPIDGAMES/Makefile @@ -0,0 +1,4 @@ +BUILD_DIR=$(shell ../../tools/find-build-dir.sh) + +all setup clean run: + @basedir=../.. TEST_BASE_DIR=../ BUILD_DIR=$(BUILD_DIR) ./test.sh --$@ diff --git a/test/TEST-20-MAINPIDGAMES/test.sh b/test/TEST-20-MAINPIDGAMES/test.sh new file mode 100755 index 0000000000..b14083a256 --- /dev/null +++ b/test/TEST-20-MAINPIDGAMES/test.sh @@ -0,0 +1,42 @@ +#!/bin/bash +# -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*- +# ex: ts=8 sw=4 sts=4 et filetype=sh +set -e +TEST_DESCRIPTION="test changing main PID" + +. $TEST_BASE_DIR/test-functions + +test_setup() { + create_empty_image + mkdir -p $TESTDIR/root + mount ${LOOPDEV}p1 $TESTDIR/root + + ( + LOG_LEVEL=5 + eval $(udevadm info --export --query=env --name=${LOOPDEV}p2) + + setup_basic_environment + + # setup the testsuite service + cat >$initdir/etc/systemd/system/testsuite.service </tmp/mainpid.sh < /run/mainpidsh/pid +EOF +chmod +x /tmp/mainpid.sh + +systemd-run --unit=mainpidsh.service -p StandardOutput=tty -p StandardError=tty -p Type=forking -p RuntimeDirectory=mainpidsh -p PIDFile=/run/mainpidsh/pid /tmp/mainpid.sh +test `systemctl show -p MainPID --value mainpidsh.service` -eq `cat /run/mainpidsh/pid` + +cat >/tmp/mainpid2.sh < /run/mainpidsh2/pid +chown 1001:1001 /run/mainpidsh2/pid +EOF +chmod +x /tmp/mainpid2.sh + +systemd-run --unit=mainpidsh2.service -p StandardOutput=tty -p StandardError=tty -p Type=forking -p RuntimeDirectory=mainpidsh2 -p PIDFile=/run/mainpidsh2/pid /tmp/mainpid2.sh +test `systemctl show -p MainPID --value mainpidsh2.service` -eq `cat /run/mainpidsh2/pid` + +cat >/dev/shm/mainpid3.sh < /testok + +exit 0 diff --git a/test/test-functions b/test/test-functions index a2f82725d1..018bdca888 100644 --- a/test/test-functions +++ b/test/test-functions @@ -21,7 +21,7 @@ if ! ROOTLIBDIR=$(pkg-config --variable=systemdutildir systemd); then ROOTLIBDIR=/usr/lib/systemd fi -BASICTOOLS="test sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe sed cmp tee rm true false" +BASICTOOLS="test sh bash setsid loadkeys setfont login sulogin gzip sleep echo mount umount cryptsetup date dmsetup modprobe sed cmp tee rm true false chmod chown ln" DEBUGTOOLS="df free ls stty cat ps ln ip route dmesg dhclient mkdir cp ping dhclient strace less grep id tty touch du sort hostname find" STATEDIR="${BUILD_DIR:-.}/test/$(basename $(dirname $(realpath $0)))" From 89e1ba0ab232f22cf24932375493c43f4ab1d693 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:32:39 +0100 Subject: [PATCH 10/14] ethtool-util: no need for memcpy() where normal assignment works too --- src/udev/net/ethtool-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index 8921ca9290..9c2dd438eb 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -417,7 +417,7 @@ static int get_glinksettings(int *fd, struct ifreq *ifr, struct ethtool_link_use if (!u) return -ENOMEM; - memcpy(&u->base, &ecmd.req, sizeof(struct ethtool_link_settings)); + ecmd.req = u->base; offset = 0; memcpy(u->link_modes.supported, &ecmd.link_mode_data[offset], 4 * ecmd.req.link_mode_masks_nwords); @@ -480,7 +480,7 @@ static int set_slinksettings(int *fd, struct ifreq *ifr, const struct ethtool_li if (u->base.cmd != ETHTOOL_GLINKSETTINGS || u->base.link_mode_masks_nwords <= 0) return -EINVAL; - memcpy(&ecmd.req, &u->base, sizeof(ecmd.req)); + ecmd.req = u->base; ecmd.req.cmd = ETHTOOL_SLINKSETTINGS; offset = 0; memcpy(&ecmd.link_mode_data[offset], u->link_modes.supported, 4 * ecmd.req.link_mode_masks_nwords); From ab1263d774da5aedd1e2976ae9e1154ab1b0d578 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:33:10 +0100 Subject: [PATCH 11/14] ethtool-util: order includes properly --- src/udev/net/ethtool-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index 9c2dd438eb..ffa1713f58 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -25,13 +25,13 @@ #include "conf-parser.h" #include "ethtool-util.h" -#include "log.h" #include "link-config.h" +#include "log.h" +#include "missing.h" #include "socket-util.h" #include "string-table.h" #include "strxcpyx.h" #include "util.h" -#include "missing.h" static const char* const duplex_table[_DUP_MAX] = { [DUP_FULL] = "full", From 20d4e99524cece9778e707741ae4f44e13f68b80 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:33:22 +0100 Subject: [PATCH 12/14] ethtool-util: fix weird whitespace --- src/udev/net/ethtool-util.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index ffa1713f58..4fbfc0bc2d 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -557,7 +557,6 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l r = get_glinksettings(fd, &ifr, &u); if (r < 0) { - r = get_gset(fd, &ifr, &u); if (r < 0) return log_warning_errno(r, "link_config: Cannot get device settings for %s : %m", ifname); @@ -570,7 +569,7 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l u->base.duplex = link->duplex; if (link->port != _NET_DEV_PORT_INVALID) - u->base.port = link->port; + u->base.port = link->port; u->base.autoneg = link->autonegotiation; @@ -578,7 +577,6 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l r = set_slinksettings(fd, &ifr, u); else r = set_sset(fd, &ifr, u); - if (r < 0) return log_warning_errno(r, "link_config: Cannot set device settings for %s : %m", ifname); From 2b44daaa201299f347ac539f111b8df66a1d5389 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:36:38 +0100 Subject: [PATCH 13/14] ethtool-util: don't pass fds as pointers if we don't have to Passing them as pointers is just weird, hence don't do it --- src/udev/net/ethtool-util.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index 4fbfc0bc2d..1401584956 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -83,6 +83,7 @@ int ethtool_connect(int *ret) { fd = socket_ioctl_fd(); if (fd < 0) return fd; + *ret = fd; return 0; @@ -265,7 +266,7 @@ int ethtool_set_wol(int *fd, const char *ifname, WakeOnLan wol) { return 0; } -static int ethtool_get_stringset(int *fd, struct ifreq *ifr, int stringset_id, struct ethtool_gstrings **gstrings) { +static int get_stringset(int fd, struct ifreq *ifr, int stringset_id, struct ethtool_gstrings **gstrings) { _cleanup_free_ struct ethtool_gstrings *strings = NULL; struct { struct ethtool_sset_info info; @@ -281,7 +282,7 @@ static int ethtool_get_stringset(int *fd, struct ifreq *ifr, int stringset_id, s ifr->ifr_data = (void *) &buffer.info; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -300,7 +301,7 @@ static int ethtool_get_stringset(int *fd, struct ifreq *ifr, int stringset_id, s ifr->ifr_data = (void *) strings; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -335,7 +336,7 @@ int ethtool_set_features(int *fd, const char *ifname, NetDevFeature *features) { strscpy(ifr.ifr_name, IFNAMSIZ, ifname); - r = ethtool_get_stringset(fd, &ifr, ETH_SS_FEATURES, &strings); + r = get_stringset(*fd, &ifr, ETH_SS_FEATURES, &strings); if (r < 0) return log_warning_errno(r, "link_config: could not get ethtool features for %s", ifname); @@ -374,7 +375,7 @@ int ethtool_set_features(int *fd, const char *ifname, NetDevFeature *features) { return 0; } -static int get_glinksettings(int *fd, struct ifreq *ifr, struct ethtool_link_usettings **g) { +static int get_glinksettings(int fd, struct ifreq *ifr, struct ethtool_link_usettings **g) { struct ecmd { struct ethtool_link_settings req; __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; @@ -395,7 +396,7 @@ static int get_glinksettings(int *fd, struct ifreq *ifr, struct ethtool_link_use ifr->ifr_data = (void *) &ecmd; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -406,7 +407,7 @@ static int get_glinksettings(int *fd, struct ifreq *ifr, struct ethtool_link_use ifr->ifr_data = (void *) &ecmd; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -433,7 +434,7 @@ static int get_glinksettings(int *fd, struct ifreq *ifr, struct ethtool_link_use return 0; } -static int get_gset(int *fd, struct ifreq *ifr, struct ethtool_link_usettings **u) { +static int get_gset(int fd, struct ifreq *ifr, struct ethtool_link_usettings **u) { struct ethtool_link_usettings *e; struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET, @@ -442,7 +443,7 @@ static int get_gset(int *fd, struct ifreq *ifr, struct ethtool_link_usettings ** ifr->ifr_data = (void *) &ecmd; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -469,7 +470,7 @@ static int get_gset(int *fd, struct ifreq *ifr, struct ethtool_link_usettings ** return 0; } -static int set_slinksettings(int *fd, struct ifreq *ifr, const struct ethtool_link_usettings *u) { +static int set_slinksettings(int fd, struct ifreq *ifr, const struct ethtool_link_usettings *u) { struct { struct ethtool_link_settings req; __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32]; @@ -493,14 +494,14 @@ static int set_slinksettings(int *fd, struct ifreq *ifr, const struct ethtool_li ifr->ifr_data = (void *) &ecmd; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; return 0; } -static int set_sset(int *fd, struct ifreq *ifr, const struct ethtool_link_usettings *u) { +static int set_sset(int fd, struct ifreq *ifr, const struct ethtool_link_usettings *u) { struct ethtool_cmd ecmd = { .cmd = ETHTOOL_SSET, }; @@ -523,7 +524,7 @@ static int set_sset(int *fd, struct ifreq *ifr, const struct ethtool_link_usetti ifr->ifr_data = (void *) &ecmd; - r = ioctl(*fd, SIOCETHTOOL, ifr); + r = ioctl(fd, SIOCETHTOOL, ifr); if (r < 0) return -errno; @@ -555,9 +556,9 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l strscpy(ifr.ifr_name, IFNAMSIZ, ifname); - r = get_glinksettings(fd, &ifr, &u); + r = get_glinksettings(*fd, &ifr, &u); if (r < 0) { - r = get_gset(fd, &ifr, &u); + r = get_gset(*fd, &ifr, &u); if (r < 0) return log_warning_errno(r, "link_config: Cannot get device settings for %s : %m", ifname); } @@ -574,9 +575,9 @@ int ethtool_set_glinksettings(int *fd, const char *ifname, struct link_config *l u->base.autoneg = link->autonegotiation; if (u->base.cmd == ETHTOOL_GLINKSETTINGS) - r = set_slinksettings(fd, &ifr, u); + r = set_slinksettings(*fd, &ifr, u); else - r = set_sset(fd, &ifr, u); + r = set_sset(*fd, &ifr, u); if (r < 0) return log_warning_errno(r, "link_config: Cannot set device settings for %s : %m", ifname); From 6b44a121c15584f340f73cf9ef8aa144ddcc22b2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 5 Jan 2018 13:41:33 +0100 Subject: [PATCH 14/14] cocci: there's not ENOTSUP, there's only EOPNOTSUPP On Linux the former is a compat alias to the latter, and that's really weird, as inside the kernel the two are distinct. Which means we really should stay away from it. --- coccinelle/enotsup.cocci | 4 ++++ src/udev/net/ethtool-util.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 coccinelle/enotsup.cocci diff --git a/coccinelle/enotsup.cocci b/coccinelle/enotsup.cocci new file mode 100644 index 0000000000..c65734d382 --- /dev/null +++ b/coccinelle/enotsup.cocci @@ -0,0 +1,4 @@ +@@ +@@ +- ENOTSUP ++ EOPNOTSUPP diff --git a/src/udev/net/ethtool-util.c b/src/udev/net/ethtool-util.c index 1401584956..9bdaef8d90 100644 --- a/src/udev/net/ethtool-util.c +++ b/src/udev/net/ethtool-util.c @@ -401,7 +401,7 @@ static int get_glinksettings(int fd, struct ifreq *ifr, struct ethtool_link_uset return -errno; if (ecmd.req.link_mode_masks_nwords >= 0 || ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) - return -ENOTSUP; + return -EOPNOTSUPP; ecmd.req.link_mode_masks_nwords = -ecmd.req.link_mode_masks_nwords; @@ -412,7 +412,7 @@ static int get_glinksettings(int fd, struct ifreq *ifr, struct ethtool_link_uset return -errno; if (ecmd.req.link_mode_masks_nwords <= 0 || ecmd.req.cmd != ETHTOOL_GLINKSETTINGS) - return -ENOTSUP; + return -EOPNOTSUPP; u = new0(struct ethtool_link_usettings , 1); if (!u)