From 23e208e742855d28bb3b02db2093e5a8ffc102f6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 3 Mar 2023 11:27:42 +0100 Subject: [PATCH 1/2] tmpfile-util: teach link_tmpfile() to optionally replace files --- src/basic/tmpfile-util.c | 70 ++++++++++++++++++++++++++++++++------ src/basic/tmpfile-util.h | 4 +-- src/boot/bootctl-install.c | 10 +++--- src/coredump/coredump.c | 2 +- src/portable/portable.c | 2 +- src/shared/copy.c | 52 ++++++---------------------- src/test/test-tmpfiles.c | 20 +++++++++-- 7 files changed, 97 insertions(+), 63 deletions(-) diff --git a/src/basic/tmpfile-util.c b/src/basic/tmpfile-util.c index 95adf9d374..379d81d5c8 100644 --- a/src/basic/tmpfile-util.c +++ b/src/basic/tmpfile-util.c @@ -14,6 +14,7 @@ #include "path-util.h" #include "process-util.h" #include "random-util.h" +#include "stat-util.h" #include "stdio-util.h" #include "string-util.h" #include "tmpfile-util.h" @@ -328,24 +329,71 @@ int fopen_tmpfile_linkable(const char *target, int flags, char **ret_path, FILE return 0; } -int link_tmpfile(int fd, const char *path, const char *target) { +static int link_fd(int fd, int newdirfd, const char *newpath) { + int r; + + assert(fd >= 0); + assert(newdirfd >= 0 || newdirfd == AT_FDCWD); + assert(newpath); + + /* Try symlinking via /proc/fd/ first. */ + r = RET_NERRNO(linkat(AT_FDCWD, FORMAT_PROC_FD_PATH(fd), newdirfd, newpath, AT_SYMLINK_FOLLOW)); + if (r != -ENOENT) + return r; + + /* Fall back to symlinking via AT_EMPTY_PATH as fallback (this requires CAP_DAC_READ_SEARCH and a + * more recent kernel, but does not require /proc/ mounted) */ + if (proc_mounted() != 0) + return r; + + return RET_NERRNO(linkat(fd, "", newdirfd, newpath, AT_EMPTY_PATH)); +} + +int link_tmpfile(int fd, const char *path, const char *target, bool replace) { + _cleanup_free_ char *tmp = NULL; + int r; + assert(fd >= 0); assert(target); - /* Moves a temporary file created with open_tmpfile() above into its final place. if "path" is NULL an fd - * created with O_TMPFILE is assumed, and linkat() is used. Otherwise it is assumed O_TMPFILE is not supported - * on the directory, and renameat2() is used instead. - * - * Note that in both cases we will not replace existing files. This is because linkat() does not support this - * operation currently (renameat2() does), and there is no nice way to emulate this. */ + /* Moves a temporary file created with open_tmpfile() above into its final place. If "path" is NULL + * an fd created with O_TMPFILE is assumed, and linkat() is used. Otherwise it is assumed O_TMPFILE + * is not supported on the directory, and renameat2() is used instead. */ + + if (path) { + if (replace) + return RET_NERRNO(rename(path, target)); - if (path) return rename_noreplace(AT_FDCWD, path, AT_FDCWD, target); + } - return RET_NERRNO(linkat(AT_FDCWD, FORMAT_PROC_FD_PATH(fd), AT_FDCWD, target, AT_SYMLINK_FOLLOW)); + r = link_fd(fd, AT_FDCWD, target); + if (r != -EEXIST || !replace) + return r; + + /* So the target already exists and we were asked to replace it. That sucks a bit, since the kernel's + * linkat() logic does not allow that. We work-around this by linking the file to a random name + * first, and then renaming that to the final name. This reintroduces the race O_TMPFILE kinda is + * trying to fix, but at least the vulnerability window (i.e. where the file is linked into the file + * system under a temporary name) is very short. */ + + r = tempfn_random(target, NULL, &tmp); + if (r < 0) + return r; + + if (link_fd(fd, AT_FDCWD, tmp) < 0) + return -EEXIST; /* propagate original error */ + + r = RET_NERRNO(rename(tmp, target)); + if (r < 0) { + (void) unlink(tmp); + return r; + } + + return 0; } -int flink_tmpfile(FILE *f, const char *path, const char *target) { +int flink_tmpfile(FILE *f, const char *path, const char *target, bool replace) { int fd, r; assert(f); @@ -359,7 +407,7 @@ int flink_tmpfile(FILE *f, const char *path, const char *target) { if (r < 0) return r; - return link_tmpfile(fd, path, target); + return link_tmpfile(fd, path, target, replace); } int mkdtemp_malloc(const char *template, char **ret) { diff --git a/src/basic/tmpfile-util.h b/src/basic/tmpfile-util.h index e5b7709e3f..4665dafb24 100644 --- a/src/basic/tmpfile-util.h +++ b/src/basic/tmpfile-util.h @@ -25,8 +25,8 @@ int open_tmpfile_unlinkable(const char *directory, int flags); int open_tmpfile_linkable(const char *target, int flags, char **ret_path); int fopen_tmpfile_linkable(const char *target, int flags, char **ret_path, FILE **ret_file); -int link_tmpfile(int fd, const char *path, const char *target); -int flink_tmpfile(FILE *f, const char *path, const char *target); +int link_tmpfile(int fd, const char *path, const char *target, bool replace); +int flink_tmpfile(FILE *f, const char *path, const char *target, bool replace); int mkdtemp_malloc(const char *template, char **ret); int mkdtemp_open(const char *template, int flags, char **ret); diff --git a/src/boot/bootctl-install.c b/src/boot/bootctl-install.c index ebb0d486c9..624b88abe3 100644 --- a/src/boot/bootctl-install.c +++ b/src/boot/bootctl-install.c @@ -426,12 +426,14 @@ static int install_binaries(const char *esp_path, const char *arch, bool force) static int install_loader_config(const char *esp_path) { _cleanup_(unlink_and_freep) char *t = NULL; _cleanup_fclose_ FILE *f = NULL; - const char *p; + _cleanup_free_ char *p = NULL; int r; assert(arg_make_entry_directory >= 0); - p = prefix_roota(esp_path, "/loader/loader.conf"); + p = path_join(esp_path, "/loader/loader.conf"); + if (!p) + return log_oom(); if (access(p, F_OK) >= 0) /* Silently skip creation if the file already exists (early check) */ return 0; @@ -447,7 +449,7 @@ static int install_loader_config(const char *esp_path) { fprintf(f, "default %s-*\n", arg_entry_token); } - r = flink_tmpfile(f, t, p); + r = flink_tmpfile(f, t, p, /* replace= */ false); if (r == -EEXIST) return 0; /* Silently skip creation if the file exists now (recheck) */ if (r < 0) @@ -476,7 +478,7 @@ static int install_loader_specification(const char *root) { fprintf(f, "type1\n"); - r = flink_tmpfile(f, t, p); + r = flink_tmpfile(f, t, p, /* replace= */ false); if (r == -EEXIST) return 0; /* Silently skip creation if the file exists now (recheck) */ if (r < 0) diff --git a/src/coredump/coredump.c b/src/coredump/coredump.c index d9db98bf32..d5d1f49d08 100644 --- a/src/coredump/coredump.c +++ b/src/coredump/coredump.c @@ -277,7 +277,7 @@ static int fix_permissions( if (r < 0) return log_error_errno(r, "Failed to sync coredump %s: %m", coredump_tmpfile_name(filename)); - r = link_tmpfile(fd, filename, target); + r = link_tmpfile(fd, filename, target, /* replace= */ false); if (r < 0) return log_error_errno(r, "Failed to move coredump %s into place: %m", target); diff --git a/src/portable/portable.c b/src/portable/portable.c index 7811833fac..377e1a9937 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -1188,7 +1188,7 @@ static int attach_unit_file( if (fchmod(fd, 0644) < 0) return log_debug_errno(errno, "Failed to change unit file access mode for '%s': %m", path); - r = link_tmpfile(fd, tmp, path); + r = link_tmpfile(fd, tmp, path, /* replace= */ false); if (r < 0) return log_debug_errno(r, "Failed to install unit file '%s': %m", path); diff --git a/src/shared/copy.c b/src/shared/copy.c index 9963e10f4d..146293acdf 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -1452,43 +1452,16 @@ int copy_file_atomic_full( assert(from); assert(to); - /* We try to use O_TMPFILE here to create the file if we can. Note that this only works if COPY_REPLACE is not - * set though as we need to use linkat() for linking the O_TMPFILE file into the file system but that system - * call can't replace existing files. Hence, if COPY_REPLACE is set we create a temporary name in the file - * system right-away and unconditionally which we then can renameat() to the right name after we completed - * writing it. */ - - if (copy_flags & COPY_REPLACE) { - _cleanup_free_ char *f = NULL; - - r = tempfn_random(to, NULL, &f); + if (copy_flags & COPY_MAC_CREATE) { + r = mac_selinux_create_file_prepare(to, S_IFREG); if (r < 0) return r; - - if (copy_flags & COPY_MAC_CREATE) { - r = mac_selinux_create_file_prepare(to, S_IFREG); - if (r < 0) - return r; - } - fdt = open(f, O_CREAT|O_EXCL|O_NOFOLLOW|O_NOCTTY|O_WRONLY|O_CLOEXEC, 0600); - if (copy_flags & COPY_MAC_CREATE) - mac_selinux_create_file_clear(); - if (fdt < 0) - return -errno; - - t = TAKE_PTR(f); - } else { - if (copy_flags & COPY_MAC_CREATE) { - r = mac_selinux_create_file_prepare(to, S_IFREG); - if (r < 0) - return r; - } - fdt = open_tmpfile_linkable(to, O_WRONLY|O_CLOEXEC, &t); - if (copy_flags & COPY_MAC_CREATE) - mac_selinux_create_file_clear(); - if (fdt < 0) - return fdt; } + fdt = open_tmpfile_linkable(to, O_WRONLY|O_CLOEXEC, &t); + if (copy_flags & COPY_MAC_CREATE) + mac_selinux_create_file_clear(); + if (fdt < 0) + return fdt; if (chattr_mask != 0) (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); @@ -1506,14 +1479,9 @@ int copy_file_atomic_full( return -errno; } - if (copy_flags & COPY_REPLACE) { - if (renameat(AT_FDCWD, t, AT_FDCWD, to) < 0) - return -errno; - } else { - r = link_tmpfile(fdt, t, to); - if (r < 0) - return r; - } + r = link_tmpfile(fdt, t, to, copy_flags & COPY_REPLACE); + if (r < 0) + return r; t = mfree(t); diff --git a/src/test/test-tmpfiles.c b/src/test/test-tmpfiles.c index 83d11f5cf6..0a856200c7 100644 --- a/src/test/test-tmpfiles.c +++ b/src/test/test-tmpfiles.c @@ -52,12 +52,28 @@ TEST(tmpfiles) { assert_se(write(fd, "foobar\n", 7) == 7); assert_se(touch(d) >= 0); - assert_se(link_tmpfile(fd, tmp, d) == -EEXIST); + assert_se(link_tmpfile(fd, tmp, d, /* replace= */ false) == -EEXIST); assert_se(unlink(d) >= 0); - assert_se(link_tmpfile(fd, tmp, d) >= 0); + assert_se(link_tmpfile(fd, tmp, d, /* replace= */ false) >= 0); assert_se(read_one_line_file(d, &line) >= 0); assert_se(streq(line, "foobar")); + + fd = safe_close(fd); + tmp = mfree(tmp); + + fd = open_tmpfile_linkable(d, O_RDWR|O_CLOEXEC, &tmp); + assert_se(fd >= 0); + + assert_se(write(fd, "waumiau\n", 8) == 8); + + assert_se(link_tmpfile(fd, tmp, d, /* replace= */ false) == -EEXIST); + assert_se(link_tmpfile(fd, tmp, d, /* replace= */ true) >= 0); + + line = mfree(line); + assert_se(read_one_line_file(d, &line) >= 0); + assert_se(streq(line, "waumiau")); + assert_se(unlink(d) >= 0); } From acbab8697ed7402f7a7bd688531561e76f2e254b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 3 Mar 2023 11:30:13 +0100 Subject: [PATCH 2/2] hwdb: port to flink_tmpfile() And modernize heavily while doing so. Fixes: #21787 (Strictly speaking, this leaves a race window open: the the system is powered off in the short interval when we linked in the prepared hwdb file into the dir under a temporary name and are about to rename it to the final name, then the file might be left over after all. But this minimizes the window so much that this shouldn't be an issue in real-life. Key after all is that with this change we'll build up the hwdb file under O_TMPFILE, and thus are robust to power loss during the slow operation) --- src/shared/hwdb-util.c | 97 ++++++++++++++++++++---------------------- 1 file changed, 47 insertions(+), 50 deletions(-) diff --git a/src/shared/hwdb-util.c b/src/shared/hwdb-util.c index 45910a36e3..785611f8c4 100644 --- a/src/shared/hwdb-util.c +++ b/src/shared/hwdb-util.c @@ -274,7 +274,6 @@ static int trie_insert(struct trie *trie, struct trie_node *node, const char *se } struct trie_f { - FILE *f; struct trie *trie; uint64_t strings_off; @@ -295,15 +294,14 @@ static void trie_store_nodes_size(struct trie_f *trie, struct trie_node *node, b trie->strings_off += compat ? sizeof(struct trie_value_entry_f) : sizeof(struct trie_value_entry2_f); } -static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node, bool compat) { - struct trie_node_f n = { - .prefix_off = htole64(trie->strings_off + node->prefix_off), - .children_count = node->children_count, - .values_count = htole64(node->values_count), - }; +static int64_t trie_store_nodes(struct trie_f *trie, FILE *f, struct trie_node *node, bool compat) { _cleanup_free_ struct trie_child_entry_f *children = NULL; int64_t node_off; + assert(trie); + assert(f); + assert(node); + if (node->children_count) { children = new(struct trie_child_entry_f, node->children_count); if (!children) @@ -314,7 +312,7 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node, boo for (uint64_t i = 0; i < node->children_count; i++) { int64_t child_off; - child_off = trie_store_nodes(trie, node->children[i].child, compat); + child_off = trie_store_nodes(trie, f, node->children[i].child, compat); if (child_off < 0) return child_off; @@ -324,14 +322,20 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node, boo }; } + struct trie_node_f n = { + .prefix_off = htole64(trie->strings_off + node->prefix_off), + .children_count = node->children_count, + .values_count = htole64(node->values_count), + }; + /* write node */ - node_off = ftello(trie->f); - fwrite(&n, sizeof(struct trie_node_f), 1, trie->f); + node_off = ftello(f); + fwrite(&n, sizeof(struct trie_node_f), 1, f); trie->nodes_count++; /* append children array */ if (node->children_count) { - fwrite(children, sizeof(struct trie_child_entry_f), node->children_count, trie->f); + fwrite(children, sizeof(struct trie_child_entry_f), node->children_count, f); trie->children_count += node->children_count; } @@ -345,7 +349,7 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node, boo .file_priority = htole16(node->values[i].file_priority), }; - fwrite(&v, compat ? sizeof(struct trie_value_entry_f) : sizeof(struct trie_value_entry2_f), 1, trie->f); + fwrite(&v, compat ? sizeof(struct trie_value_entry_f) : sizeof(struct trie_value_entry2_f), 1, f); } trie->values_count += node->values_count; @@ -355,11 +359,26 @@ static int64_t trie_store_nodes(struct trie_f *trie, struct trie_node *node, boo static int trie_store(struct trie *trie, const char *filename, bool compat) { struct trie_f t = { .trie = trie, + .strings_off = sizeof(struct trie_header_f), }; - _cleanup_free_ char *filename_tmp = NULL; - int64_t pos; - int64_t root_off; - int64_t size; + _cleanup_(unlink_and_freep) char *filename_tmp = NULL; + _cleanup_fclose_ FILE *f = NULL; + int64_t pos, root_off, size; + int r; + + assert(trie); + assert(filename); + + /* calculate size of header, nodes, children entries, value entries */ + trie_store_nodes_size(&t, trie->root, compat); + + r = fopen_tmpfile_linkable(filename, O_WRONLY|O_CLOEXEC, &filename_tmp, &f); + if (r < 0) + return r; + + if (fchmod(fileno(f), 0444) < 0) + return -errno; + struct trie_header_f h = { .signature = HWDB_SIG, .tool_version = htole64(PROJECT_VERSION), @@ -368,48 +387,32 @@ static int trie_store(struct trie *trie, const char *filename, bool compat) { .child_entry_size = htole64(sizeof(struct trie_child_entry_f)), .value_entry_size = htole64(compat ? sizeof(struct trie_value_entry_f) : sizeof(struct trie_value_entry2_f)), }; - int r; - - /* calculate size of header, nodes, children entries, value entries */ - t.strings_off = sizeof(struct trie_header_f); - trie_store_nodes_size(&t, trie->root, compat); - - r = fopen_temporary(filename, &t.f, &filename_tmp); - if (r < 0) - return r; - (void) fchmod(fileno(t.f), 0444); /* write nodes */ - if (fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET) < 0) - goto error_fclose; + if (fseeko(f, sizeof(struct trie_header_f), SEEK_SET) < 0) + return -errno; - root_off = trie_store_nodes(&t, trie->root, compat); + root_off = trie_store_nodes(&t, f, trie->root, compat); h.nodes_root_off = htole64(root_off); - pos = ftello(t.f); + pos = ftello(f); h.nodes_len = htole64(pos - sizeof(struct trie_header_f)); /* write string buffer */ - fwrite(trie->strings->buf, trie->strings->len, 1, t.f); + fwrite(trie->strings->buf, trie->strings->len, 1, f); h.strings_len = htole64(trie->strings->len); /* write header */ - size = ftello(t.f); + size = ftello(f); h.file_size = htole64(size); - if (fseeko(t.f, 0, SEEK_SET) < 0) - goto error_fclose; - fwrite(&h, sizeof(struct trie_header_f), 1, t.f); + if (fseeko(f, 0, SEEK_SET) < 0) + return -errno; + fwrite(&h, sizeof(struct trie_header_f), 1, f); - if (ferror(t.f)) - goto error_fclose; - if (fflush(t.f) < 0) - goto error_fclose; - if (fsync(fileno(t.f)) < 0) - goto error_fclose; - if (rename(filename_tmp, filename) < 0) - goto error_fclose; + r = flink_tmpfile(f, filename_tmp, filename, /* replace= */ true); + if (r < 0) + return r; /* write succeeded */ - fclose(t.f); log_debug("=== trie on-disk ==="); log_debug("size: %8"PRIi64" bytes", size); @@ -423,12 +426,6 @@ static int trie_store(struct trie *trie, const char *filename, bool compat) { log_debug("string store: %8zu bytes", trie->strings->len); log_debug("strings start: %8"PRIu64, t.strings_off); return 0; - - error_fclose: - r = -errno; - fclose(t.f); - (void) unlink(filename_tmp); - return r; } static int insert_data(struct trie *trie, char **match_list, char *line, const char *filename,