From e6e001cdd410e3de1ac5c53a745d12ac1a739531 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 16 Dec 2025 01:43:29 +0100 Subject: [PATCH 1/5] acl-util: treat pointer type as a whole Follow-up for 4976d6e9ac0c621a3182416c35350cd0fe2f1977 --- src/shared/acl-util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/acl-util.h b/src/shared/acl-util.h index ee46c575d0..d65fe0299f 100644 --- a/src/shared/acl-util.h +++ b/src/shared/acl-util.h @@ -68,7 +68,7 @@ static inline int acl_set_perm(acl_permset_t ps, acl_perm_t p, bool b) { #else -typedef void *acl_t; +typedef void* acl_t; typedef int acl_tag_t; typedef unsigned acl_type_t; From 4abf7ee02e7642bc6ff0b064a95ab57c906f4302 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 16 Dec 2025 01:47:10 +0100 Subject: [PATCH 2/5] Revert "acl-util: drop now unused fd_acl_make_writable()" This reverts commit 4175cd4f400c2e89028878dc265ccd67ca838edd. It turned out that we still need this. Preparation for later commits. --- src/shared/acl-util.c | 79 ++++++++++++++++++++++++++++++++++++++++ src/shared/acl-util.h | 6 +++ src/test/test-acl-util.c | 24 ++++++++++++ 3 files changed, 109 insertions(+) diff --git a/src/shared/acl-util.c b/src/shared/acl-util.c index bcac9a9bb8..21bbb01278 100644 --- a/src/shared/acl-util.c +++ b/src/shared/acl-util.c @@ -738,6 +738,68 @@ maybe_fallback: /* No ACLs? Then just update the regular mode_t */ return fd_acl_make_read_only_fallback(fd); } + +int fd_acl_make_writable(int fd) { + _cleanup_(acl_freep) acl_t acl = NULL; + acl_entry_t i; + int r; + + /* Safely adds the writable bit to the owner's ACL entry of this inode. (And only the owner's! – This + * not the obvious inverse of fd_acl_make_read_only() hence!) */ + + r = dlopen_libacl(); + if (r < 0) + return r; + + acl = sym_acl_get_fd(fd); + if (!acl) { + if (!ERRNO_IS_NOT_SUPPORTED(errno)) + return -errno; + + /* No ACLs? Then just update the regular mode_t */ + return fd_acl_make_writable_fallback(fd); + } + + for (r = sym_acl_get_entry(acl, ACL_FIRST_ENTRY, &i); + r > 0; + r = sym_acl_get_entry(acl, ACL_NEXT_ENTRY, &i)) { + acl_permset_t permset; + acl_tag_t tag; + int b; + + if (sym_acl_get_tag_type(i, &tag) < 0) + return -errno; + + if (tag != ACL_USER_OBJ) + continue; + + if (sym_acl_get_permset(i, &permset) < 0) + return -errno; + + b = sym_acl_get_perm(permset, ACL_WRITE); + if (b < 0) + return -errno; + + if (b) + return 0; /* Already set? Then there's nothing to do. */ + + if (sym_acl_add_perm(permset, ACL_WRITE) < 0) + return -errno; + + break; + } + if (r < 0) + return -errno; + + if (sym_acl_set_fd(fd, acl) < 0) { + if (!ERRNO_IS_NOT_SUPPORTED(errno)) + return -errno; + + return fd_acl_make_writable_fallback(fd); + } + + return 1; +} #endif int fd_acl_make_read_only_fallback(int fd) { @@ -757,6 +819,23 @@ int fd_acl_make_read_only_fallback(int fd) { return 1; } +int fd_acl_make_writable_fallback(int fd) { + struct stat st; + + assert(fd >= 0); + + if (fstat(fd, &st) < 0) + return -errno; + + if ((st.st_mode & 0200) != 0) /* already set */ + return 0; + + if (fchmod(fd, (st.st_mode & 07777) | 0200) < 0) + return -errno; + + return 1; +} + int inode_type_can_acl(mode_t mode) { return IN_SET(mode & S_IFMT, S_IFSOCK, S_IFREG, S_IFBLK, S_IFCHR, S_IFDIR, S_IFIFO); } diff --git a/src/shared/acl-util.h b/src/shared/acl-util.h index d65fe0299f..33a19f30b4 100644 --- a/src/shared/acl-util.h +++ b/src/shared/acl-util.h @@ -4,6 +4,7 @@ #include "shared-forward.h" int fd_acl_make_read_only_fallback(int fd); +int fd_acl_make_writable_fallback(int fd); #if HAVE_ACL #include /* IWYU pragma: export */ @@ -55,6 +56,7 @@ int acls_for_file(const char *path, acl_type_t type, acl_t acl, acl_t *ret); int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask); int fd_acl_make_read_only(int fd); +int fd_acl_make_writable(int fd); /* acl_free() takes multiple argument types. Multiple cleanup functions are necessary. */ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL_RENAME(acl_t, sym_acl_free, acl_freep, NULL); @@ -104,6 +106,10 @@ static inline int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask) { static inline int fd_acl_make_read_only(int fd) { return fd_acl_make_read_only_fallback(fd); } + +static inline int fd_acl_make_writable(int fd) { + return fd_acl_make_writable_fallback(fd); +} #endif int inode_type_can_acl(mode_t mode); diff --git a/src/test/test-acl-util.c b/src/test/test-acl-util.c index bf65d38b37..5967c4b406 100644 --- a/src/test/test-acl-util.c +++ b/src/test/test-acl-util.c @@ -112,6 +112,30 @@ TEST_RET(fd_acl_make_read_only) { cmd = strjoina("stat ", fn); ASSERT_OK_ZERO_ERRNO(system(cmd)); + log_info("writable"); + ASSERT_OK_POSITIVE(fd_acl_make_writable(fd)); + + ASSERT_OK_ERRNO(fstat(fd, &st)); + ASSERT_EQ(st.st_mode & 0222, 0200u); + + cmd = strjoina("getfacl -p ", fn); + ASSERT_OK_ZERO_ERRNO(system(cmd)); + + cmd = strjoina("stat ", fn); + ASSERT_OK_ZERO_ERRNO(system(cmd)); + + log_info("read-only"); + ASSERT_OK_POSITIVE(fd_acl_make_read_only(fd)); + + ASSERT_OK_ERRNO(fstat(fd, &st)); + ASSERT_EQ(st.st_mode & 0222, 0000u); + + cmd = strjoina("getfacl -p ", fn); + ASSERT_OK_ZERO_ERRNO(system(cmd)); + + cmd = strjoina("stat ", fn); + ASSERT_OK_ZERO_ERRNO(system(cmd)); + return 0; } From 5188da568ccd4f987f7762dc1d06a62064f97591 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 16 Dec 2025 02:18:25 +0100 Subject: [PATCH 3/5] acl-util: systematically fall back to fchmod() if acl is not available Same as 2a64b56da44751a3205c16ed4de6aa52c8e92929, but for fd_acl_make_writable(). While at it, un-export fd_acl_make_{read_only,writable}_fallback(), instead apply ifdeffery in .c --- src/shared/acl-util.c | 98 +++++++++++++++++++++++-------------------- src/shared/acl-util.h | 17 ++------ 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/src/shared/acl-util.c b/src/shared/acl-util.c index 21bbb01278..24384e64ad 100644 --- a/src/shared/acl-util.c +++ b/src/shared/acl-util.c @@ -668,15 +668,34 @@ int fd_add_uid_acl_permission( return 0; } +#endif + +static int fd_acl_make_read_only_fallback(int fd) { + struct stat st; + + assert(fd >= 0); + + if (fstat(fd, &st) < 0) + return -errno; + + if ((st.st_mode & 0222) == 0) + return 0; + + if (fchmod(fd, st.st_mode & 0555) < 0) + return -errno; + + return 1; +} int fd_acl_make_read_only(int fd) { + assert(fd >= 0); + +#if HAVE_ACL _cleanup_(acl_freep) acl_t acl = NULL; bool changed = false; acl_entry_t i; int r; - assert(fd >= 0); - /* Safely drops all W bits from all relevant ACL entries of the file, without changing entries which * are masked by the ACL mask */ @@ -734,12 +753,33 @@ int fd_acl_make_read_only(int fd) { maybe_fallback: if (!ERRNO_IS_NEG_NOT_SUPPORTED(r)) return r; +#endif /* No ACLs? Then just update the regular mode_t */ return fd_acl_make_read_only_fallback(fd); } +static int fd_acl_make_writable_fallback(int fd) { + struct stat st; + + assert(fd >= 0); + + if (fstat(fd, &st) < 0) + return -errno; + + if ((st.st_mode & 0200) != 0) /* already set */ + return 0; + + if (fchmod(fd, (st.st_mode & 07777) | 0200) < 0) + return -errno; + + return 1; +} + int fd_acl_make_writable(int fd) { + assert(fd >= 0); + +#if HAVE_ACL _cleanup_(acl_freep) acl_t acl = NULL; acl_entry_t i; int r; @@ -749,15 +789,12 @@ int fd_acl_make_writable(int fd) { r = dlopen_libacl(); if (r < 0) - return r; + goto maybe_fallback; acl = sym_acl_get_fd(fd); if (!acl) { - if (!ERRNO_IS_NOT_SUPPORTED(errno)) - return -errno; - - /* No ACLs? Then just update the regular mode_t */ - return fd_acl_make_writable_fallback(fd); + r = -errno; + goto maybe_fallback; } for (r = sym_acl_get_entry(acl, ACL_FIRST_ENTRY, &i); @@ -792,48 +829,19 @@ int fd_acl_make_writable(int fd) { return -errno; if (sym_acl_set_fd(fd, acl) < 0) { - if (!ERRNO_IS_NOT_SUPPORTED(errno)) - return -errno; - - return fd_acl_make_writable_fallback(fd); + r = -errno; + goto maybe_fallback; } return 1; -} + +maybe_fallback: + if (!ERRNO_IS_NEG_NOT_SUPPORTED(r)) + return r; #endif -int fd_acl_make_read_only_fallback(int fd) { - struct stat st; - - assert(fd >= 0); - - if (fstat(fd, &st) < 0) - return -errno; - - if ((st.st_mode & 0222) == 0) - return 0; - - if (fchmod(fd, st.st_mode & 0555) < 0) - return -errno; - - return 1; -} - -int fd_acl_make_writable_fallback(int fd) { - struct stat st; - - assert(fd >= 0); - - if (fstat(fd, &st) < 0) - return -errno; - - if ((st.st_mode & 0200) != 0) /* already set */ - return 0; - - if (fchmod(fd, (st.st_mode & 07777) | 0200) < 0) - return -errno; - - return 1; + /* No ACLs? Then just update the regular mode_t */ + return fd_acl_make_writable_fallback(fd); } int inode_type_can_acl(mode_t mode) { diff --git a/src/shared/acl-util.h b/src/shared/acl-util.h index 33a19f30b4..0e0423ad25 100644 --- a/src/shared/acl-util.h +++ b/src/shared/acl-util.h @@ -3,9 +3,6 @@ #include "shared-forward.h" -int fd_acl_make_read_only_fallback(int fd); -int fd_acl_make_writable_fallback(int fd); - #if HAVE_ACL #include /* IWYU pragma: export */ #include /* IWYU pragma: export */ @@ -55,9 +52,6 @@ int parse_acl( int acls_for_file(const char *path, acl_type_t type, acl_t acl, acl_t *ret); int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask); -int fd_acl_make_read_only(int fd); -int fd_acl_make_writable(int fd); - /* acl_free() takes multiple argument types. Multiple cleanup functions are necessary. */ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL_RENAME(acl_t, sym_acl_free, acl_freep, NULL); DEFINE_TRIVIAL_CLEANUP_FUNC_FULL_RENAME(char*, sym_acl_free, acl_free_charpp, NULL); @@ -102,14 +96,9 @@ static inline int devnode_acl(int fd, uid_t uid) { static inline int fd_add_uid_acl_permission(int fd, uid_t uid, unsigned mask) { return -EOPNOTSUPP; } - -static inline int fd_acl_make_read_only(int fd) { - return fd_acl_make_read_only_fallback(fd); -} - -static inline int fd_acl_make_writable(int fd) { - return fd_acl_make_writable_fallback(fd); -} #endif +int fd_acl_make_read_only(int fd); +int fd_acl_make_writable(int fd); + int inode_type_can_acl(mode_t mode); From 6c1bcdd614438b2068f62c4bab38ad3215b9dfe5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 16 Dec 2025 02:10:32 +0100 Subject: [PATCH 4/5] core/exec-credential: fix credentials plain dir exchanging Follow-up for d796c6b7c64bd47b192c5351c955e9b8f4298bd4 rename() may yield ENOTEMPTY rather than EEXIST for existing dir, so catch that too. Also, both the source and target must be writable when exchanging. Test coverage for this logic (i.e. user units with ExecStartPre=) will be added later. --- src/core/exec-credential.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/exec-credential.c b/src/core/exec-credential.c index d4ab704eb4..3d7f066400 100644 --- a/src/core/exec-credential.c +++ b/src/core/exec-credential.c @@ -980,7 +980,13 @@ static int setup_credentials_plain_dir( r = RET_NERRNO(rename(workspace, cred_dir)); if (r >= 0) workspace_rm = NULL; - if (r == -EEXIST) { + if (IN_SET(r, -ENOTEMPTY, -EEXIST)) { + _cleanup_close_ int old_dfd = open(cred_dir, O_DIRECTORY|O_CLOEXEC|O_NOFOLLOW); + if (old_dfd < 0) + return log_debug_errno(errno, "Failed to open credentials dir '%s': %m", cred_dir); + + (void) fd_acl_make_writable(old_dfd); + log_debug_errno(r, "Credential dir '%s' already populated, exchanging with workspace.", cred_dir); r = RET_NERRNO(renameat2(AT_FDCWD, workspace, AT_FDCWD, cred_dir, RENAME_EXCHANGE)); } From 3602308480ba68bf37fc4ade7cd8cf98b1e3722f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Tue, 16 Dec 2025 20:21:30 +0100 Subject: [PATCH 5/5] TEST-54-CREDS: add test case for #40108 --- test/units/TEST-54-CREDS.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/units/TEST-54-CREDS.sh b/test/units/TEST-54-CREDS.sh index 9e2cee22ce..ae16e5b3df 100755 --- a/test/units/TEST-54-CREDS.sh +++ b/test/units/TEST-54-CREDS.sh @@ -554,6 +554,9 @@ run0 -u testuser --pipe mkdir -p /home/testuser/.config/credstore.encrypted run0 -u testuser --pipe systemd-creds encrypt --user --name=brummbaer - /home/testuser/.config/credstore.encrypted/brummbaer < /tmp/brummbaer.data run0 -u testuser --pipe systemd-run --user --pipe -p ImportCredential=brummbaer systemd-creds cat brummbaer | cmp /tmp/brummbaer.data +# https://github.com/systemd/systemd/pull/40108 +run0 -u testuser --pipe systemd-run --user --wait -p ImportCredential=brummbaer -p ExecStartPre='ls -l $CREDENTIALS_DIRECTORY' bash -c 'systemd-creds cat brummbaer | cmp /tmp/brummbaer.data' + # https://github.com/systemd/systemd/pull/39651 TESTUSER_CRED_DIR="/run/user/$(id -u testuser)/credentials"