From 9e096259ce259f4cb19d949daa71bf4b399c567b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 06:36:31 +0900 Subject: [PATCH 1/8] fileio: fix verification on failure in write_string_file_full() Fixes a bug introduced by 0ab5e2a4b4f5d435cb66b591ef5c700894663fd3. --- src/basic/fileio.c | 2 +- src/basic/fileio.h | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 9f2bbc1323..61b58d39c0 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -380,7 +380,7 @@ fail: /* OK, the operation failed, but let's see if the right contents in place already. If so, eat up the * error. */ - if (verify_file(fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE) || (flags & WRITE_STRING_FILE_VERIFY_IGNORE_NEWLINE)) > 0) + if (verify_file_at(dir_fd, fn, line, !(flags & WRITE_STRING_FILE_AVOID_NEWLINE) || (flags & WRITE_STRING_FILE_VERIFY_IGNORE_NEWLINE)) > 0) return 0; return r; diff --git a/src/basic/fileio.h b/src/basic/fileio.h index bd053050e1..52d80e129e 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -90,9 +90,6 @@ static inline int read_full_stream(FILE *f, char **ret_contents, size_t *ret_siz } int verify_file_at(int dir_fd, const char *fn, const char *blob, bool accept_extra_nl); -static inline int verify_file(const char *fn, const char *blob, bool accept_extra_nl) { - return verify_file_at(AT_FDCWD, fn, blob, accept_extra_nl); -} int script_get_shebang_interpreter(const char *path, char **ret); From f3c5c2b001803818a507149f78db0a90b2376091 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 06:43:12 +0900 Subject: [PATCH 2/8] fileio: make write_string_file_at() accept O_PATH fd and an empty filename Then, introduce an inline wrapper write_string_file_fd(). --- src/basic/fileio.c | 50 +++++++++++++++++++++++++++------------------- src/basic/fileio.h | 3 +++ 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index 61b58d39c0..b691307f05 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -298,19 +298,22 @@ int write_string_file_full( _cleanup_close_ int fd = -EBADF; int r; - assert(fn); + assert(dir_fd == AT_FDCWD || dir_fd >= 0); assert(line); /* We don't know how to verify whether the file contents was already on-disk. */ assert(!((flags & WRITE_STRING_FILE_VERIFY_ON_FAILURE) && (flags & WRITE_STRING_FILE_SYNC))); if (flags & WRITE_STRING_FILE_MKDIR_0755) { + assert(fn); + r = mkdirat_parents(dir_fd, fn, 0755); if (r < 0) return r; } if (flags & WRITE_STRING_FILE_ATOMIC) { + assert(fn); assert(flags & WRITE_STRING_FILE_CREATE); r = write_string_file_atomic_at(dir_fd, fn, line, flags, ts); @@ -320,25 +323,31 @@ int write_string_file_full( return r; } - mode_t mode = write_string_file_flags_to_mode(flags); - - if (FLAGS_SET(flags, WRITE_STRING_FILE_LABEL|WRITE_STRING_FILE_CREATE)) { - r = label_ops_pre(dir_fd, label_fn ?: fn, mode); - if (r < 0) - goto fail; - - call_label_ops_post = true; - } - /* We manually build our own version of fopen(..., "we") that works without O_CREAT and with O_NOFOLLOW if needed. */ - fd = openat_report_new( - dir_fd, fn, O_CLOEXEC | O_NOCTTY | - (FLAGS_SET(flags, WRITE_STRING_FILE_NOFOLLOW) ? O_NOFOLLOW : 0) | - (FLAGS_SET(flags, WRITE_STRING_FILE_CREATE) ? O_CREAT : 0) | - (FLAGS_SET(flags, WRITE_STRING_FILE_TRUNCATE) ? O_TRUNC : 0) | - (FLAGS_SET(flags, WRITE_STRING_FILE_SUPPRESS_REDUNDANT_VIRTUAL) ? O_RDWR : O_WRONLY), - mode, - &made_file); + if (isempty(fn)) + fd = fd_reopen(ASSERT_FD(dir_fd), O_CLOEXEC | O_NOCTTY | + (FLAGS_SET(flags, WRITE_STRING_FILE_TRUNCATE) ? O_TRUNC : 0) | + (FLAGS_SET(flags, WRITE_STRING_FILE_SUPPRESS_REDUNDANT_VIRTUAL) ? O_RDWR : O_WRONLY)); + else { + mode_t mode = write_string_file_flags_to_mode(flags); + + if (FLAGS_SET(flags, WRITE_STRING_FILE_LABEL|WRITE_STRING_FILE_CREATE)) { + r = label_ops_pre(dir_fd, label_fn ?: fn, mode); + if (r < 0) + goto fail; + + call_label_ops_post = true; + } + + fd = openat_report_new( + dir_fd, fn, O_CLOEXEC | O_NOCTTY | + (FLAGS_SET(flags, WRITE_STRING_FILE_NOFOLLOW) ? O_NOFOLLOW : 0) | + (FLAGS_SET(flags, WRITE_STRING_FILE_CREATE) ? O_CREAT : 0) | + (FLAGS_SET(flags, WRITE_STRING_FILE_TRUNCATE) ? O_TRUNC : 0) | + (FLAGS_SET(flags, WRITE_STRING_FILE_SUPPRESS_REDUNDANT_VIRTUAL) ? O_RDWR : O_WRONLY), + mode, + &made_file); + } if (fd < 0) { r = fd; goto fail; @@ -442,7 +451,6 @@ int verify_file_at(int dir_fd, const char *fn, const char *blob, bool accept_ext size_t l, k; int r; - assert(fn); assert(blob); l = strlen(blob); @@ -454,7 +462,7 @@ int verify_file_at(int dir_fd, const char *fn, const char *blob, bool accept_ext if (!buf) return -ENOMEM; - r = fopen_unlocked_at(dir_fd, fn, "re", 0, &f); + r = fopen_unlocked_at(dir_fd, strempty(fn), "re", 0, &f); if (r < 0) return r; diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 52d80e129e..9139c03df4 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -56,6 +56,9 @@ int write_string_file_full(int dir_fd, const char *fn, const char *line, WriteSt static inline int write_string_file_at(int dir_fd, const char *fn, const char *line, WriteStringFileFlags flags) { return write_string_file_full(dir_fd, fn, line, flags, NULL, NULL); } +static inline int write_string_file_fd(int dir_fd, const char *line, WriteStringFileFlags flags) { + return write_string_file_at(dir_fd, NULL, line, flags); +} static inline int write_string_file(const char *fn, const char *line, WriteStringFileFlags flags) { return write_string_file_at(AT_FDCWD, fn, line, flags); } From 06503dd0dfda6348b60dee4cf16054d84a25577a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 06:45:03 +0900 Subject: [PATCH 3/8] fileio: make read_virtual_file_at() accept O_PATH file descriptor Then, merge read_virtual_file_at() and read_virtual_file_fd(), and make the latter inline. --- src/basic/fileio.c | 43 ++++++++++++++++--------------------------- src/basic/fileio.h | 4 +++- 2 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index b691307f05..622a13495a 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -482,7 +482,13 @@ int verify_file_at(int dir_fd, const char *fn, const char *blob, bool accept_ext return 1; } -int read_virtual_file_fd(int fd, size_t max_size, char **ret_contents, size_t *ret_size) { +int read_virtual_file_at( + int dir_fd, + const char *filename, + size_t max_size, + char **ret_contents, + size_t *ret_size) { + _cleanup_free_ char *buf = NULL; size_t n, size; int n_retries; @@ -501,9 +507,17 @@ int read_virtual_file_fd(int fd, size_t max_size, char **ret_contents, size_t *r * contents* may be returned. (Though the read is still done using one syscall.) Returns 0 on * partial success, 1 if untruncated contents were read. */ - assert(fd >= 0); + assert(dir_fd >= 0 || dir_fd == AT_FDCWD); assert(max_size <= READ_VIRTUAL_BYTES_MAX || max_size == SIZE_MAX); + _cleanup_close_ int fd = -EBADF; + if (isempty(filename)) + fd = fd_reopen(ASSERT_FD(dir_fd), O_RDONLY | O_NOCTTY | O_CLOEXEC); + else + fd = RET_NERRNO(openat(dir_fd, filename, O_RDONLY | O_NOCTTY | O_CLOEXEC)); + if (fd < 0) + return fd; + /* Limit the number of attempts to read the number of bytes returned by fstat(). */ n_retries = 3; @@ -627,31 +641,6 @@ int read_virtual_file_fd(int fd, size_t max_size, char **ret_contents, size_t *r return !truncated; } -int read_virtual_file_at( - int dir_fd, - const char *filename, - size_t max_size, - char **ret_contents, - size_t *ret_size) { - - _cleanup_close_ int fd = -EBADF; - - assert(dir_fd >= 0 || dir_fd == AT_FDCWD); - - if (!filename) { - if (dir_fd == AT_FDCWD) - return -EBADF; - - return read_virtual_file_fd(dir_fd, max_size, ret_contents, ret_size); - } - - fd = openat(dir_fd, filename, O_RDONLY | O_NOCTTY | O_CLOEXEC); - if (fd < 0) - return -errno; - - return read_virtual_file_fd(fd, max_size, ret_contents, ret_size); -} - int read_full_stream_full( FILE *f, const char *filename, diff --git a/src/basic/fileio.h b/src/basic/fileio.h index 9139c03df4..49da9a677c 100644 --- a/src/basic/fileio.h +++ b/src/basic/fileio.h @@ -78,8 +78,10 @@ static inline int read_full_file(const char *filename, char **ret_contents, size return read_full_file_full(AT_FDCWD, filename, UINT64_MAX, SIZE_MAX, 0, NULL, ret_contents, ret_size); } -int read_virtual_file_fd(int fd, size_t max_size, char **ret_contents, size_t *ret_size); int read_virtual_file_at(int dir_fd, const char *filename, size_t max_size, char **ret_contents, size_t *ret_size); +static inline int read_virtual_file_fd(int fd, size_t max_size, char **ret_contents, size_t *ret_size) { + return read_virtual_file_at(fd, NULL, max_size, ret_contents, ret_size); +} static inline int read_virtual_file(const char *filename, size_t max_size, char **ret_contents, size_t *ret_size) { return read_virtual_file_at(AT_FDCWD, filename, max_size, ret_contents, ret_size); } From 8d89667abaa8b569c2ac9e37752c404cebdfaaaa Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 12 Jan 2025 07:03:49 +0900 Subject: [PATCH 4/8] sd-device: chase sysattr and refuse to read/write files outside of sysfs This makes sd_device_get_sysattr_value()/sd_device_set_sysattr_value() refuse to read/write files outside of sysfs for safety. Also this makes - use chase() to resolve and open the symlink in path to sysfs attribute, - use delete_trailing_chars(), - include error code in cache entry, so we can cache more error cases, - refuse caching value written to uevent file of any devices, i.e. sd_device_set_sysattr_value(dev, "../uevent", "add") will also not cache the value "add". --- src/libsystemd/sd-device/device-private.h | 4 +- src/libsystemd/sd-device/sd-device.c | 329 +++++++++++++--------- 2 files changed, 206 insertions(+), 127 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.h b/src/libsystemd/sd-device/device-private.h index eab54203f0..11a27b01d6 100644 --- a/src/libsystemd/sd-device/device-private.h +++ b/src/libsystemd/sd-device/device-private.h @@ -9,6 +9,7 @@ #include "sd-device.h" +#include "chase.h" #include "macro.h" int device_new_from_mode_and_devnum(sd_device **ret, mode_t mode, dev_t devnum); @@ -31,8 +32,9 @@ int device_get_devnode_mode(sd_device *device, mode_t *ret); int device_get_devnode_uid(sd_device *device, uid_t *ret); int device_get_devnode_gid(sd_device *device, gid_t *ret); +int device_chase(sd_device *device, const char *path, ChaseFlags flags, char **ret_resolved, int *ret_fd); void device_clear_sysattr_cache(sd_device *device); -int device_cache_sysattr_value(sd_device *device, const char *key, char *value); +int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error); int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value); void device_seal(sd_device *device); diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index a608ae326f..2ca5a67786 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -2326,134 +2326,215 @@ void device_clear_sysattr_cache(sd_device *device) { device->sysattr_values = hashmap_free(device->sysattr_values); } -int device_cache_sysattr_value(sd_device *device, const char *key, char *value) { - _unused_ _cleanup_free_ char *old_value = NULL; - _cleanup_free_ char *new_key = NULL; +typedef struct SysAttrCacheEntry { + char *key; + char *value; + int error; +} SysAttrCacheEntry; + +static SysAttrCacheEntry* sysattr_cache_entry_free(SysAttrCacheEntry *p) { + if (!p) + return NULL; + + free(p->key); + free(p->value); + return mfree(p); +} + +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + sysattr_cache_hash_ops, + char, path_hash_func, path_compare, + SysAttrCacheEntry, sysattr_cache_entry_free); + +int device_cache_sysattr_value(sd_device *device, char *key, char *value, int error) { int r; assert(device); assert(key); + assert(value || error > 0); - /* This takes the reference of the input value. The input value may be NULL. - * This replaces the value if it already exists. */ + /* This takes the reference of the input arguments on success. The input value may be NULL. + * This replaces an already existing entry. */ - /* First, remove the old cache entry. So, we do not need to clear cache on error. */ - old_value = hashmap_remove2(device->sysattr_values, key, (void **) &new_key); - if (!new_key) { - new_key = strdup(key); - if (!new_key) - return -ENOMEM; - } + /* Remove the old cache entry. So, we do not need to clear cache on error. */ + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, key)); - r = hashmap_ensure_put(&device->sysattr_values, &path_hash_ops_free_free, new_key, value); + /* We use ENOANO as a recognizable error code when we have not read the attribute. */ + if (error == ENOANO) + error = ESTALE; + + _cleanup_free_ SysAttrCacheEntry *entry = new(SysAttrCacheEntry, 1); + if (!entry) + return -ENOMEM; + + *entry = (SysAttrCacheEntry) { + .key = key, + .value = value, + .error = error, + }; + + r = hashmap_ensure_put(&device->sysattr_values, &sysattr_cache_hash_ops, entry->key, entry); if (r < 0) return r; - TAKE_PTR(new_key); - + TAKE_PTR(entry); return 0; } int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) { - const char *k = NULL, *value; + SysAttrCacheEntry *entry; assert(device); assert(key); - value = hashmap_get2(device->sysattr_values, key, (void **) &k); - if (!k) - return -ESTALE; /* We have not read the attribute. */ - if (!value) - return -ENOENT; /* We have looked up the attribute before and it did not exist. */ + entry = hashmap_get(device->sysattr_values, key); + if (!entry) + return -ENOANO; /* We have not read the attribute. */ + if (!entry->value) { + /* We have looked up the attribute before and failed. Return the cached error code. */ + assert(entry->error > 0); + return -entry->error; + } if (ret_value) - *ret_value = value; + *ret_value = entry->value; return 0; } -/* We cache all sysattr lookups. If an attribute does not exist, it is stored - * with a NULL value in the cache, otherwise the returned string is stored */ -_public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **ret_value) { - _cleanup_free_ char *value = NULL, *path = NULL; +int device_chase(sd_device *device, const char *path, ChaseFlags flags, char **ret_resolved, int *ret_fd) { + int r; + + assert(device); + assert(path); + const char *syspath; - struct stat statbuf; + r = sd_device_get_syspath(device, &syspath); + if (r < 0) + return r; + + /* Here, CHASE_PREFIX_ROOT is borrowed. If the flag is set or the specified path is relative, then + * the path will be prefixed with the syspath. Note, we do not pass CHASE_PREFIX_ROOT flag with + * syspath as root to chase(), but we manually concatenate the specified path with syspath before + * calling chase(). Otherwise, we cannot set/get attributes of parent or sibling devices. */ + _cleanup_free_ char *prefixed = NULL; + if (FLAGS_SET(flags, CHASE_PREFIX_ROOT) || !path_is_absolute(path)) { + prefixed = path_join(syspath, path); + if (!prefixed) + return -ENOMEM; + path = prefixed; + flags &= ~CHASE_PREFIX_ROOT; + } + + _cleanup_free_ char *resolved = NULL; + _cleanup_close_ int fd = -EBADF; + r = chase(path, /* root = */ NULL, CHASE_NO_AUTOFS | flags, &resolved, ret_fd ? &fd : NULL); + if (r < 0) + return r; + + /* Refuse to reading/writing files outside of sysfs. */ + if (!path_startswith(resolved, "/sys/")) + return -EINVAL; + + if (ret_resolved) { + /* Always return relative path. */ + r = path_make_relative(syspath, resolved, ret_resolved); + if (r < 0) + return r; + } + + if (ret_fd) + *ret_fd = TAKE_FD(fd); + + return 0; +} + +_public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **ret_value) { + _cleanup_free_ char *resolved = NULL, *value = NULL; + _cleanup_close_ int fd = -EBADF; int r; assert_return(device, -EINVAL); assert_return(sysattr, -EINVAL); - /* look for possibly already cached result */ + /* Look for possibly already cached result. */ r = device_get_cached_sysattr_value(device, sysattr, ret_value); - if (r != -ESTALE) + if (r != -ENOANO) return r; - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; + /* Special cases: read the symlink and return the last component of the value. Some core links return + * only the last element of the target path, these are just values, the paths should not be exposed. */ + if (STR_IN_SET(sysattr, "driver", "subsystem", "module")) { + _cleanup_free_ char *prefixed = NULL; + const char *syspath; - path = path_join(syspath, sysattr); - if (!path) - return -ENOMEM; - - if (lstat(path, &statbuf) < 0) { - int k; - - r = -errno; - - /* remember that we could not access the sysattr */ - k = device_cache_sysattr_value(device, sysattr, NULL); - if (k < 0) - log_device_debug_errno(device, k, - "sd-device: failed to cache attribute '%s' with NULL, ignoring: %m", - sysattr); - - return r; - } else if (S_ISLNK(statbuf.st_mode)) { - /* Some core links return only the last element of the target path, - * these are just values, the paths should not be exposed. */ - if (STR_IN_SET(sysattr, "driver", "subsystem", "module")) { - r = readlink_value(path, &value); - if (r < 0) - return r; - } else - return -EINVAL; - } else if (S_ISDIR(statbuf.st_mode)) - /* skip directories */ - return -EISDIR; - else if (!(statbuf.st_mode & S_IRUSR)) - /* skip non-readable files */ - return -EPERM; - else { - size_t size; - - /* Read attribute value, Some attributes contain embedded '\0'. So, it is necessary to - * also get the size of the result. See issue #20025. */ - r = read_full_virtual_file(path, &value, &size); + r = sd_device_get_syspath(device, &syspath); if (r < 0) return r; - /* drop trailing newlines */ - while (size > 0 && strchr(NEWLINE, value[--size])) - value[size] = '\0'; + prefixed = path_join(syspath, sysattr); + if (!prefixed) + return -ENOMEM; + + r = readlink_value(prefixed, &value); + if (r != -EINVAL) /* -EINVAL means the path is not a symlink. */ + goto cache_result; } - /* Unfortunately, we need to return 'const char*' instead of 'char*'. Hence, failure in caching - * sysattr value is critical unlike the other places. */ - r = device_cache_sysattr_value(device, sysattr, value); - if (r < 0) { - log_device_debug_errno(device, r, - "sd-device: failed to cache attribute '%s' with '%s'%s: %m", - sysattr, value, ret_value ? "" : ", ignoring"); - if (ret_value) - return r; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT, &resolved, &fd); + if (r < 0) + goto cache_result; - return 0; + /* Look for cacheed result again with the resolved path. */ + r = device_get_cached_sysattr_value(device, resolved, ret_value); + if (r != -ENOANO) + return r; + + /* Read attribute value, Some attributes contain embedded '\0'. So, it is necessary to also get the + * size of the result. See issue #20025. */ + size_t size; + r = read_virtual_file_fd(fd, SIZE_MAX, &value, &size); + if (r < 0) + goto cache_result; + + delete_trailing_chars(value, NEWLINE); + r = 0; + +cache_result: + if (r == -ENOMEM) + return r; /* Do not cache -ENOMEM, as the failure may be transient. */ + + if (!resolved) { + /* If we have not or could not chase the path, assume 'sysattr' is normalized. */ + resolved = strdup(sysattr); + if (!resolved) + return RET_GATHER(r, -ENOMEM); } - if (ret_value) + int k = device_cache_sysattr_value(device, resolved, value, -r); + if (k < 0) { + if (r < 0) + log_device_debug_errno(device, k, + "sd-device: failed to cache error code (%i) in reading attribute '%s', ignoring: %m", + -r, resolved); + else { + /* Unfortunately, we need to return 'const char*' instead of 'char*'. Hence, failure in caching + * sysattr value is critical unlike the other places. */ + log_device_debug_errno(device, k, + "sd-device: failed to cache attribute '%s' with '%s'%s: %m", + resolved, value, ret_value ? "" : ", ignoring"); + if (ret_value) + return k; + } + + return r; + } + + if (ret_value && r >= 0) *ret_value = value; + /* device_cache_sysattr_value() takes 'resolved' and 'value' on success. */ + TAKE_PTR(resolved); TAKE_PTR(value); - return 0; + return r; } int device_get_sysattr_int(sd_device *device, const char *sysattr, int *ret_value) { @@ -2527,19 +2608,22 @@ int device_get_sysattr_bool(sd_device *device, const char *sysattr) { return parse_boolean(value); } -static void device_remove_cached_sysattr_value(sd_device *device, const char *_key) { - _cleanup_free_ char *key = NULL; +static int device_remove_cached_sysattr_value(sd_device *device, const char *sysattr) { + int r; assert(device); - assert(_key); + assert(sysattr); - free(hashmap_remove2(device->sysattr_values, _key, (void **) &key)); + _cleanup_free_ char *resolved = NULL; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &resolved, /* ret_fd = */ NULL); + if (r < 0) + return r; + + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, resolved)); + return 0; } -_public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, const char *_value) { - _cleanup_free_ char *value = NULL, *path = NULL; - const char *syspath; - size_t len; +_public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, const char *value) { int r; assert_return(device, -EINVAL); @@ -2547,52 +2631,47 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, /* Set the attribute and save it in the cache. */ - if (!_value) { + if (!value) /* If input value is NULL, then clear cache and not write anything. */ - device_remove_cached_sysattr_value(device, sysattr); - return 0; + return device_remove_cached_sysattr_value(device, sysattr); + + _cleanup_free_ char *resolved = NULL; + _cleanup_close_ int fd = -EBADF; + r = device_chase(device, sysattr, CHASE_PREFIX_ROOT, &resolved, &fd); + if (r < 0) { + /* On failure, clear cache entry, hopefully, 'sysattr' is normalized. */ + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, sysattr)); + return r; } - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; - - path = path_join(syspath, sysattr); - if (!path) + /* value length is limited to 4k */ + _cleanup_free_ char *copied = strndup(value, 4096); + if (!copied) return -ENOMEM; - len = strlen(_value); - /* drop trailing newlines */ - while (len > 0 && strchr(NEWLINE, _value[len - 1])) - len--; + delete_trailing_chars(copied, NEWLINE); - /* value length is limited to 4k */ - if (len > 4096) - return -EINVAL; - - value = strndup(_value, len); - if (!value) - return -ENOMEM; - - r = write_string_file(path, value, WRITE_STRING_FILE_DISABLE_BUFFER | WRITE_STRING_FILE_NOFOLLOW); + r = write_string_file_fd(fd, copied, WRITE_STRING_FILE_DISABLE_BUFFER | WRITE_STRING_FILE_AVOID_NEWLINE); if (r < 0) { /* On failure, clear cache entry, as we do not know how it fails. */ - device_remove_cached_sysattr_value(device, sysattr); + sysattr_cache_entry_free(hashmap_remove(device->sysattr_values, resolved)); return r; } /* Do not cache action string written into uevent file. */ - if (streq(sysattr, "uevent")) + if (streq(last_path_component(resolved), "uevent")) return 0; - r = device_cache_sysattr_value(device, sysattr, value); + r = device_cache_sysattr_value(device, resolved, copied, 0); if (r < 0) log_device_debug_errno(device, r, - "sd-device: failed to cache attribute '%s' with '%s', ignoring: %m", - sysattr, value); - else - TAKE_PTR(value); + "sd-device: failed to cache written attribute '%s' with '%s', ignoring: %m", + resolved, copied); + else { + TAKE_PTR(resolved); + TAKE_PTR(copied); + } return 0; } @@ -2605,10 +2684,8 @@ _public_ int sd_device_set_sysattr_valuef(sd_device *device, const char *sysattr assert_return(device, -EINVAL); assert_return(sysattr, -EINVAL); - if (!format) { - device_remove_cached_sysattr_value(device, sysattr); - return 0; - } + if (!format) + return device_remove_cached_sysattr_value(device, sysattr); va_start(ap, format); r = vasprintf(&value, format, ap); From 6ebbdcc0ddbacce732001823cf2be2a1d4381c60 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 10:33:19 +0900 Subject: [PATCH 5/8] sd-device: use sd_device_get_sysattr_value() to read special symlinks Then, cached result may be used. No functional change, just refactoring. --- src/libsystemd/sd-device/sd-device.c | 39 ++++++++-------------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 2ca5a67786..c6d293793c 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1215,35 +1215,24 @@ _public_ int sd_device_get_subsystem(sd_device *device, const char **ret) { assert_return(device, -EINVAL); if (!device->subsystem_set) { - _cleanup_free_ char *subsystem = NULL; - const char *syspath; - char *path; + const char *subsystem; - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; - - /* read 'subsystem' link */ - path = strjoina(syspath, "/subsystem"); - r = readlink_value(path, &subsystem); + r = sd_device_get_sysattr_value(device, "subsystem", &subsystem); if (r < 0 && r != -ENOENT) return log_device_debug_errno(device, r, "sd-device: Failed to read subsystem for %s: %m", device->devpath); - - if (subsystem) + if (r >= 0) r = device_set_subsystem(device, subsystem); /* use implicit names */ else if (!isempty(path_startswith(device->devpath, "/module/"))) r = device_set_subsystem(device, "module"); - else if (strstr(syspath, "/drivers/") || endswith(syspath, "/drivers")) + else if (strstr(device->devpath, "/drivers/") || endswith(device->devpath, "/drivers")) r = device_set_drivers_subsystem(device); else if (!isempty(PATH_STARTSWITH_SET(device->devpath, "/class/", "/bus/"))) r = device_set_subsystem(device, "subsystem"); - else { - device->subsystem_set = true; - r = 0; - } + else + r = device_set_subsystem(device, NULL); if (r < 0) return log_device_debug_errno(device, r, "sd-device: Failed to set subsystem for %s: %m", @@ -1352,23 +1341,17 @@ int device_set_driver(sd_device *device, const char *driver) { } _public_ int sd_device_get_driver(sd_device *device, const char **ret) { + int r; + assert_return(device, -EINVAL); if (!device->driver_set) { - _cleanup_free_ char *driver = NULL; - const char *syspath; - char *path; - int r; + const char *driver = NULL; - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; - - path = strjoina(syspath, "/driver"); - r = readlink_value(path, &driver); + r = sd_device_get_sysattr_value(device, "driver", &driver); if (r < 0 && r != -ENOENT) return log_device_debug_errno(device, r, - "sd-device: readlink(\"%s\") failed: %m", path); + "sd-device: Failed to read driver: %m"); r = device_set_driver(device, driver); if (r < 0) From 17dc9ec4b6e82fc2a4f9809011e154f538eb8b9c Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 10:39:21 +0900 Subject: [PATCH 6/8] sd-device: use sd_device_get_sysattr_value() to read uevent file This also replaces the custom parser with strv_split_newlines_full(). No functional change, just refactoring. --- src/libsystemd/sd-device/sd-device.c | 89 ++++++++-------------------- 1 file changed, 26 insertions(+), 63 deletions(-) diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index c6d293793c..b74ea6a1c1 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -787,89 +787,52 @@ static int handle_uevent_line( } int device_read_uevent_file(sd_device *device) { - _cleanup_free_ char *uevent = NULL; - const char *syspath, *key = NULL, *value = NULL, *major = NULL, *minor = NULL; - char *path; - size_t uevent_len; int r; - enum { - PRE_KEY, - KEY, - PRE_VALUE, - VALUE, - INVALID_LINE, - } state = PRE_KEY; - assert(device); if (device->uevent_loaded || device->sealed) return 0; - r = sd_device_get_syspath(device, &syspath); - if (r < 0) - return r; - device->uevent_loaded = true; - path = strjoina(syspath, "/uevent"); - - r = read_full_virtual_file(path, &uevent, &uevent_len); - if (r == -EACCES || ERRNO_IS_NEG_DEVICE_ABSENT(r)) + const char *uevent; + r = sd_device_get_sysattr_value(device, "uevent", &uevent); + if (ERRNO_IS_NEG_PRIVILEGE(r) || ERRNO_IS_NEG_DEVICE_ABSENT(r)) /* The uevent files may be write-only, the device may be already removed, or the device * may not have the uevent file. */ return 0; if (r < 0) - return log_device_debug_errno(device, r, "sd-device: Failed to read uevent file '%s': %m", path); + return log_device_debug_errno(device, r, "sd-device: Failed to read uevent file: %m"); - for (size_t i = 0; i < uevent_len; i++) - switch (state) { - case PRE_KEY: - if (!strchr(NEWLINE, uevent[i])) { - key = &uevent[i]; + _cleanup_strv_free_ char **v = NULL; + r = strv_split_newlines_full(&v, uevent, EXTRACT_RETAIN_ESCAPE); + if (r < 0) + return log_device_debug_errno(device, r, "sd-device: Failed to parse uevent file: %m"); - state = KEY; - } - - break; - case KEY: - if (uevent[i] == '=') { - uevent[i] = '\0'; - - state = PRE_VALUE; - } else if (strchr(NEWLINE, uevent[i])) { - uevent[i] = '\0'; - log_device_debug(device, "sd-device: Invalid uevent line '%s', ignoring", key); - - state = PRE_KEY; - } - - break; - case PRE_VALUE: - value = &uevent[i]; - state = VALUE; - - _fallthrough_; /* to handle empty property */ - case VALUE: - if (strchr(NEWLINE, uevent[i])) { - uevent[i] = '\0'; - - r = handle_uevent_line(device, key, value, &major, &minor); - if (r < 0) - log_device_debug_errno(device, r, "sd-device: Failed to handle uevent entry '%s=%s', ignoring: %m", key, value); - - state = PRE_KEY; - } - - break; - default: - assert_not_reached(); + const char *major = NULL, *minor = NULL; + STRV_FOREACH(s, v) { + char *eq = strchr(*s, '='); + if (!eq) { + log_device_debug(device, "sd-device: Invalid uevent line, ignoring: %s", *s); + continue; } + *eq = '\0'; + + r = handle_uevent_line(device, *s, eq + 1, &major, &minor); + if (r < 0) + log_device_debug_errno(device, r, + "sd-device: Failed to handle uevent entry '%s=%s', ignoring: %m", + *s, eq + 1); + } + if (major) { r = device_set_devnum(device, major, minor); if (r < 0) - log_device_debug_errno(device, r, "sd-device: Failed to set 'MAJOR=%s' or 'MINOR=%s' from '%s', ignoring: %m", major, strna(minor), path); + log_device_debug_errno(device, r, + "sd-device: Failed to set 'MAJOR=%s' and/or 'MINOR=%s' from uevent, ignoring: %m", + major, strna(minor)); } return 0; From 640f8e9c4df01572ee43941d3f64042c7b771b0a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 10:40:32 +0900 Subject: [PATCH 7/8] sd-device: use specific setters for read entries from uevent file Previously, if e.g. DRIVER=foo is specified in uevent file, the value is only saved as property, but was not set to sd_device.driver. That was inconsistent to the case when a device is created through netlink uevent. Let's always set when we get e.g. sd_device.driver when DRIVER=foo from both uevent file and netlink uevent. --- src/libsystemd/sd-device/device-internal.h | 2 ++ src/libsystemd/sd-device/device-private.c | 16 +++++++++++++-- src/libsystemd/sd-device/sd-device.c | 23 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/libsystemd/sd-device/device-internal.h b/src/libsystemd/sd-device/device-internal.h index a465eb25fd..c79181d5bb 100644 --- a/src/libsystemd/sd-device/device-internal.h +++ b/src/libsystemd/sd-device/device-internal.h @@ -106,6 +106,8 @@ static inline int device_add_property_internal(sd_device *device, const char *ke int device_set_syspath(sd_device *device, const char *_syspath, bool verify); int device_set_ifindex(sd_device *device, const char *ifindex); +int device_set_devuid(sd_device *device, const char *uid); +int device_set_devgid(sd_device *device, const char *gid); int device_set_devmode(sd_device *device, const char *devmode); int device_set_devname(sd_device *device, const char *devname); int device_set_devtype(sd_device *device, const char *devtype); diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 1c148b8573..21847de541 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -116,6 +116,10 @@ int device_get_devnode_mode(sd_device *device, mode_t *ret) { assert(device); + r = device_read_uevent_file(device); + if (r < 0) + return r; + r = device_read_db(device); if (r < 0) return r; @@ -134,6 +138,10 @@ int device_get_devnode_uid(sd_device *device, uid_t *ret) { assert(device); + r = device_read_uevent_file(device); + if (r < 0) + return r; + r = device_read_db(device); if (r < 0) return r; @@ -147,7 +155,7 @@ int device_get_devnode_uid(sd_device *device, uid_t *ret) { return 0; } -static int device_set_devuid(sd_device *device, const char *uid) { +int device_set_devuid(sd_device *device, const char *uid) { uid_t u; int r; @@ -172,6 +180,10 @@ int device_get_devnode_gid(sd_device *device, gid_t *ret) { assert(device); + r = device_read_uevent_file(device); + if (r < 0) + return r; + r = device_read_db(device); if (r < 0) return r; @@ -185,7 +197,7 @@ int device_get_devnode_gid(sd_device *device, gid_t *ret) { return 0; } -static int device_set_devgid(sd_device *device, const char *gid) { +int device_set_devgid(sd_device *device, const char *gid) { gid_t g; int r; diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index b74ea6a1c1..5f456de25c 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -766,16 +766,24 @@ static int handle_uevent_line( assert(major); assert(minor); + if (streq(key, "SUBSYSTEM")) + return device_set_subsystem(device, value); if (streq(key, "DEVTYPE")) return device_set_devtype(device, value); if (streq(key, "IFINDEX")) return device_set_ifindex(device, value); if (streq(key, "DEVNAME")) return device_set_devname(device, value); + if (streq(key, "DEVUID")) + return device_set_devuid(device, value); + if (streq(key, "DEVGID")) + return device_set_devgid(device, value); if (streq(key, "DEVMODE")) return device_set_devmode(device, value); if (streq(key, "DISKSEQ")) return device_set_diskseq(device, value); + if (streq(key, "DRIVER")) + return device_set_driver(device, value); if (streq(key, "MAJOR")) *major = value; else if (streq(key, "MINOR")) @@ -835,6 +843,13 @@ int device_read_uevent_file(sd_device *device) { major, strna(minor)); } + if (device_in_subsystem(device, "drivers")) { + r = device_set_drivers_subsystem(device); + if (r < 0) + log_device_debug_errno(device, r, + "sd-device: Failed to set driver subsystem, ignoring: %m"); + } + return 0; } @@ -1177,6 +1192,10 @@ _public_ int sd_device_get_subsystem(sd_device *device, const char **ret) { assert_return(device, -EINVAL); + r = device_read_uevent_file(device); + if (r < 0) + return r; + if (!device->subsystem_set) { const char *subsystem; @@ -1308,6 +1327,10 @@ _public_ int sd_device_get_driver(sd_device *device, const char **ret) { assert_return(device, -EINVAL); + r = device_read_uevent_file(device); + if (r < 0) + return r; + if (!device->driver_set) { const char *driver = NULL; From 1fe5b063639e4ee5e3376c5f05aa9abfb0878940 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 18 Jan 2025 10:41:06 +0900 Subject: [PATCH 8/8] sd-device: use device_in_subsystem() at more places --- src/libsystemd/sd-device/device-private.c | 5 +++-- src/libsystemd/sd-device/sd-device.c | 20 ++++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/libsystemd/sd-device/device-private.c b/src/libsystemd/sd-device/device-private.c index 21847de541..463eb7ea9e 100644 --- a/src/libsystemd/sd-device/device-private.c +++ b/src/libsystemd/sd-device/device-private.c @@ -441,10 +441,11 @@ static int device_verify(sd_device *device) { return log_device_debug_errno(device, SYNTHETIC_ERRNO(EINVAL), "sd-device: Device created from strv or nulstr lacks devpath, subsystem, action or seqnum."); - if (streq(device->subsystem, "drivers")) { + if (device_in_subsystem(device, "drivers")) { r = device_set_drivers_subsystem(device); if (r < 0) - return r; + return log_device_debug_errno(device, r, + "sd-device: Failed to set driver subsystem: %m"); } device->sealed = true; diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index 5f456de25c..63265df34e 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1671,18 +1671,13 @@ _public_ int sd_device_get_device_id(sd_device *device, const char **ret) { if (!device->device_id) { _cleanup_free_ char *id = NULL; - const char *subsystem; dev_t devnum; int ifindex, r; - r = sd_device_get_subsystem(device, &subsystem); - if (r < 0) - return r; - if (sd_device_get_devnum(device, &devnum) >= 0) { /* use dev_t — b259:131072, c254:0 */ if (asprintf(&id, "%c" DEVNUM_FORMAT_STR, - streq(subsystem, "block") ? 'b' : 'c', + device_in_subsystem(device, "block") ? 'b' : 'c', DEVNUM_FORMAT_VAL(devnum)) < 0) return -ENOMEM; } else if (sd_device_get_ifindex(device, &ifindex) >= 0) { @@ -1700,13 +1695,18 @@ _public_ int sd_device_get_device_id(sd_device *device, const char **ret) { if (r == O_DIRECTORY) return -EINVAL; - if (streq(subsystem, "drivers")) { + if (device_in_subsystem(device, "drivers")) /* the 'drivers' pseudo-subsystem is special, and needs the real * subsystem encoded as well */ - assert(device->driver_subsystem); - id = strjoin("+drivers:", device->driver_subsystem, ":", sysname); - } else + id = strjoin("+drivers:", ASSERT_PTR(device->driver_subsystem), ":", sysname); + else { + const char *subsystem; + r = sd_device_get_subsystem(device, &subsystem); + if (r < 0) + return r; + id = strjoin("+", subsystem, ":", sysname); + } if (!id) return -ENOMEM; }