From b9daaedbb9e725a4864056ff3950265f98ac821d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Sep 2020 16:48:07 +0200 Subject: [PATCH 1/4] udev-util: simplify device_is_renaming() error handling --- src/shared/udev-util.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index 8b27a6d0d0..31fbfac9cf 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -288,10 +288,12 @@ int device_is_renaming(sd_device *dev) { assert(dev); r = sd_device_get_property_value(dev, "ID_RENAMING", NULL); - if (r < 0 && r != -ENOENT) + if (r == -ENOENT) + return false; + if (r < 0) return r; - return r >= 0; + return true; } bool device_for_action(sd_device *dev, DeviceAction action) { From 9271daeed73fa29e599e1e3554b0452908b94f9d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Sep 2020 21:54:20 +0200 Subject: [PATCH 2/4] selinux: add apis to set labels/fix labels per fd instead of path --- src/basic/selinux-util.c | 66 ++++++++++++++++++++++++++++++---------- src/basic/selinux-util.h | 6 ++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/basic/selinux-util.c b/src/basic/selinux-util.c index 9b3b15d387..41913fc655 100644 --- a/src/basic/selinux-util.c +++ b/src/basic/selinux-util.c @@ -205,14 +205,11 @@ static int mac_selinux_reload(int seqno) { int mac_selinux_fix_container(const char *path, const char *inside_path, LabelFixFlags flags) { -#if HAVE_SELINUX - char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; - _cleanup_freecon_ char* fcon = NULL; - _cleanup_close_ int fd = -1; - struct stat st; - int r; - assert(path); + assert(inside_path); + +#if HAVE_SELINUX + _cleanup_close_ int fd = -1; /* if mac_selinux_init() wasn't called before we are a NOOP */ if (!label_hnd) @@ -227,6 +224,27 @@ int mac_selinux_fix_container(const char *path, const char *inside_path, LabelFi return -errno; } + return mac_selinux_fix_container_fd(fd, path, inside_path, flags); +#endif + + return 0; +} + +int mac_selinux_fix_container_fd(int fd, const char *path, const char *inside_path, LabelFixFlags flags) { + + assert(fd >= 0); + assert(inside_path); + +#if HAVE_SELINUX + char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; + _cleanup_freecon_ char* fcon = NULL; + struct stat st; + int r; + + /* if mac_selinux_init() wasn't called before we are a NOOP */ + if (!label_hnd) + return 0; + if (fstat(fd, &st) < 0) return -errno; @@ -234,12 +252,11 @@ int mac_selinux_fix_container(const char *path, const char *inside_path, LabelFi mac_selinux_maybe_reload(); if (selabel_lookup_raw(label_hnd, &fcon, inside_path, st.st_mode) < 0) { - r = -errno; - /* If there's no label to set, then exit without warning */ - if (r == -ENOENT) + if (errno == ENOENT) return 0; + r = -errno; goto fail; } @@ -247,16 +264,16 @@ int mac_selinux_fix_container(const char *path, const char *inside_path, LabelFi if (setfilecon_raw(procfs_path, fcon) < 0) { _cleanup_freecon_ char *oldcon = NULL; - r = -errno; - /* If the FS doesn't support labels, then exit without warning */ - if (r == -EOPNOTSUPP) + if (ERRNO_IS_NOT_SUPPORTED(errno)) return 0; /* It the FS is read-only and we were told to ignore failures caused by that, suppress error */ - if (r == -EROFS && (flags & LABEL_IGNORE_EROFS)) + if (errno == EROFS && (flags & LABEL_IGNORE_EROFS)) return 0; + r = -errno; + /* If the old label is identical to the new one, suppress any kind of error */ if (getfilecon_raw(procfs_path, &oldcon) >= 0 && streq(fcon, oldcon)) return 0; @@ -267,7 +284,7 @@ int mac_selinux_fix_container(const char *path, const char *inside_path, LabelFi return 0; fail: - return log_enforcing_errno(r, "Unable to fix SELinux security context of %s (%s): %m", path, inside_path); + return log_enforcing_errno(r, "Unable to fix SELinux security context of %s (%s): %m", strna(path), strna(inside_path)); #endif return 0; @@ -275,11 +292,12 @@ fail: int mac_selinux_apply(const char *path, const char *label) { + assert(path); + #if HAVE_SELINUX if (!mac_selinux_use()) return 0; - assert(path); assert(label); if (setfilecon(path, label) < 0) @@ -288,6 +306,22 @@ int mac_selinux_apply(const char *path, const char *label) { return 0; } +int mac_selinux_apply_fd(int fd, const char *path, const char *label) { + + assert(fd >= 0); + +#if HAVE_SELINUX + if (!mac_selinux_use()) + return 0; + + assert(label); + + if (fsetfilecon(fd, label) < 0) + return log_enforcing_errno(errno, "Failed to set SELinux security context %s on path %s: %m", label, strna(path)); +#endif + return 0; +} + int mac_selinux_get_create_label_from_exe(const char *exe, char **label) { #if HAVE_SELINUX _cleanup_freecon_ char *mycon = NULL, *fcon = NULL; diff --git a/src/basic/selinux-util.h b/src/basic/selinux-util.h index a982289f6a..43d22b914f 100644 --- a/src/basic/selinux-util.h +++ b/src/basic/selinux-util.h @@ -28,7 +28,13 @@ static inline int mac_selinux_fix(const char *path, LabelFixFlags flags) { return mac_selinux_fix_container(path, path, flags); } +int mac_selinux_fix_container_fd(int fd, const char *path, const char *inside_path, LabelFixFlags flags); +static inline int mac_selinux_fix_fd(int fd, const char *path, LabelFixFlags flags) { + return mac_selinux_fix_container_fd(fd, path, path, flags); +} + int mac_selinux_apply(const char *path, const char *label); +int mac_selinux_apply_fd(int fd, const char *path, const char *label); int mac_selinux_get_create_label_from_exe(const char *exe, char **label); int mac_selinux_get_our_label(char **label); From f25bff5eaf6881717e873f27c26f2e8264517c16 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Sep 2020 16:40:02 +0200 Subject: [PATCH 3/4] fs-util: add new futimens_opath() helper futimens() that works for O_PATH fds. --- src/basic/fs-util.c | 19 +++++++++++++++++++ src/basic/fs-util.h | 2 ++ 2 files changed, 21 insertions(+) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index c20a29332a..587b3504ee 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -312,6 +312,25 @@ int fchmod_opath(int fd, mode_t m) { return 0; } +int futimens_opath(int fd, const struct timespec ts[2]) { + char procfs_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int)]; + + /* Similar to fchmod_path() but for futimens() */ + + xsprintf(procfs_path, "/proc/self/fd/%i", fd); + if (utimensat(AT_FDCWD, procfs_path, ts, 0) < 0) { + if (errno != ENOENT) + return -errno; + + if (proc_mounted() == 0) + return -ENOSYS; /* if we have no /proc/, the concept is not implementable */ + + return -ENOENT; + } + + return 0; +} + int stat_warn_permissions(const char *path, const struct stat *st) { assert(path); assert(st); diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index eb6e1eee4f..cf87a1e4d5 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -38,6 +38,8 @@ int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid); int fchmod_umask(int fd, mode_t mode); int fchmod_opath(int fd, mode_t m); +int futimens_opath(int fd, const struct timespec ts[2]); + int fd_warn_permissions(const char *path, int fd); int stat_warn_permissions(const char *path, const struct stat *st); From a7fdc6cbd399acdb1a975a7f72b9be4504a38c7c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 14 Sep 2020 21:58:40 +0200 Subject: [PATCH 4/4] udev: apply access mode/ownership to device nodes with O_PATH Let's open the device node to modify with O_PATH, and then adjust it only after verifying everything is in order. This fixes a race where the a device appears, disappears and quickly reappers, while we are still running the rules for the first appearance: when going by path we'd possibly adjust half of the old and half of the new node. By O_PATH we can pin the node while we operate on it, thus removing the race. Previously, we'd do a superficial racey check if the device node changed undearneath us, and would propagate EEXIST in that case, failing the rule set. With this change we'll instead gracefully handle this, exactly like in the pre-existing case when the device node disappeared in the meantime. --- src/udev/udev-node.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/src/udev/udev-node.c b/src/udev/udev-node.c index bc259dd6f6..2cc26d29fa 100644 --- a/src/udev/udev-node.c +++ b/src/udev/udev-node.c @@ -273,9 +273,10 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, mode_t mode, uid_t uid, gid_t gid, OrderedHashmap *seclabel_list) { const char *devnode, *subsystem, *id_filename = NULL; + bool apply_mode, apply_uid, apply_gid; + _cleanup_close_ int node_fd = -1; struct stat stats; dev_t devnum; - bool apply_mode, apply_uid, apply_gid; int r; assert(dev); @@ -296,16 +297,25 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, else mode |= S_IFCHR; - if (lstat(devnode, &stats) < 0) { - if (errno == ENOENT) - return 0; /* this is necessarily racey, so ignore missing the device */ - return log_device_debug_errno(dev, errno, "cannot stat() node %s: %m", devnode); + node_fd = open(devnode, O_PATH|O_NOFOLLOW|O_CLOEXEC); + if (node_fd < 0) { + if (errno == ENOENT) { + log_device_debug_errno(dev, errno, "Device node %s is missing, skipping handling.", devnode); + return 0; /* This is necessarily racey, so ignore missing the device */ + } + + return log_device_debug_errno(dev, errno, "Cannot open node %s: %m", devnode); } - if ((mode != MODE_INVALID && (stats.st_mode & S_IFMT) != (mode & S_IFMT)) || stats.st_rdev != devnum) - return log_device_debug_errno(dev, SYNTHETIC_ERRNO(EEXIST), - "Found node '%s' with non-matching devnum %s, skip handling", - devnode, id_filename); + if (fstat(node_fd, &stats) < 0) + return log_device_debug_errno(dev, errno, "cannot stat() node %s: %m", devnode); + + if ((mode != MODE_INVALID && (stats.st_mode & S_IFMT) != (mode & S_IFMT)) || stats.st_rdev != devnum) { + log_device_debug(dev, "Found node '%s' with non-matching devnum %s, skipping handling.", + devnode, id_filename); + return 0; /* We might process a device that already got replaced by the time we have a look + * at it, handle this gracefully and step away. */ + } apply_mode = mode != MODE_INVALID && (stats.st_mode & 0777) != (mode & 0777); apply_uid = uid_is_valid(uid) && stats.st_uid != uid; @@ -322,7 +332,7 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, gid_is_valid(gid) ? gid : stats.st_gid, mode != MODE_INVALID ? mode & 0777 : stats.st_mode & 0777); - r = chmod_and_chown(devnode, mode, uid, gid); + r = fchmod_and_chown(node_fd, mode, uid, gid); if (r < 0) log_device_full_errno(dev, r == -ENOENT ? LOG_DEBUG : LOG_ERR, r, "Failed to set owner/mode of %s to uid=" UID_FMT @@ -345,7 +355,7 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, if (streq(name, "selinux")) { selinux = true; - q = mac_selinux_apply(devnode, label); + q = mac_selinux_apply_fd(node_fd, devnode, label); if (q < 0) log_device_full_errno(dev, q == -ENOENT ? LOG_DEBUG : LOG_ERR, q, "SECLABEL: failed to set SELinux label '%s': %m", label); @@ -355,7 +365,7 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, } else if (streq(name, "smack")) { smack = true; - q = mac_smack_apply(devnode, SMACK_ATTR_ACCESS, label); + q = mac_smack_apply_fd(node_fd, SMACK_ATTR_ACCESS, label); if (q < 0) log_device_full_errno(dev, q == -ENOENT ? LOG_DEBUG : LOG_ERR, q, "SECLABEL: failed to set SMACK label '%s': %m", label); @@ -368,13 +378,15 @@ static int node_permissions_apply(sd_device *dev, bool apply_mac, /* set the defaults */ if (!selinux) - (void) mac_selinux_fix(devnode, LABEL_IGNORE_ENOENT); + (void) mac_selinux_fix_fd(node_fd, devnode, LABEL_IGNORE_ENOENT); if (!smack) - (void) mac_smack_apply(devnode, SMACK_ATTR_ACCESS, NULL); + (void) mac_smack_apply_fd(node_fd, SMACK_ATTR_ACCESS, NULL); } /* always update timestamp when we re-use the node, like on media change events */ - (void) utimensat(AT_FDCWD, devnode, NULL, 0); + r = futimens_opath(node_fd, NULL); + if (r < 0) + log_device_debug_errno(dev, r, "Failed to adjust timestamp of node %s: %m", devnode); return r; }