From 2c6dc792538260a8087ac5b22c31b3b8e47c85d6 Mon Sep 17 00:00:00 2001 From: Norbert Szetei Date: Sat, 22 Aug 2026 14:29:00 +0200 Subject: landlock: Fix use-after-free of the source's parent directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit current_check_refer_path() reads old_dentry->d_parent without holding a reference nor a lock on it, and then dereferences it in collect_domain_accesses() and in the audit record. A reference on a child does not pin its parent: __d_move() reassigns dentry->d_parent and drops the reference the child held on its former parent. hook_path_rename() is not affected because the rename path calls lock_rename() before the hook, so the source cannot be reparented under it. hook_path_link() has no such protection: filename_linkat() holds a reference on the source dentry but neither locks nor references its parent, so a concurrent rename(2) can reparent the source while security_path_link() runs, and the former parent can then be removed and freed while the hook walks it. A process can trigger this after entering a Landlock domain that handles at least one filesystem access right. The process can then race a linkat(2) loop against rename(2) and rmdir(2): BUG: KASAN: slab-use-after-free in collect_domain_accesses+0x278/0x290 Read of size 4 at addr ffff888160bd53f4 by task llrepro2/549 collect_domain_accesses+0x278/0x290 current_check_refer_path+0x952/0x1120 security_path_link+0x1be/0x320 filename_linkat+0x342/0x6d0 __x64_sys_linkat+0xfa/0x150 Freed by task 562: kmem_cache_free+0x139/0x4c0 i_callback+0x4b/0x80 rcu_core+0x7dc/0x10a0 Take a reference on the dentry selected as the source parent, using dget() for the common-mount-root case and dget_parent() otherwise. Release it after the hierarchy walk and synchronous audit logging. Cc: stable@vger.kernel.org Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER") Signed-off-by: Norbert Szetei Reviewed-by: Günther Noack Tested-by: Günther Noack Link: https://patch.msgid.link/E9CDD9E6-E960-4DE2-B1AC-5667D52ABB3E@doyensec.com [mic: Clarify the caller, reachability, and reference handling] Signed-off-by: Mickaël Salaün --- security/landlock/fs.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 30aa6ce13590..330a1871bf94 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -1298,11 +1298,12 @@ static int current_check_refer_path(struct dentry *const old_dentry, /* * old_dentry may be the root of the common mount point and * !IS_ROOT(old_dentry) at the same time (e.g. with open_tree() and - * OPEN_TREE_CLONE). We do not need to call dget(old_parent) because - * we keep a reference to old_dentry. + * OPEN_TREE_CLONE). Pin the dentry used as old_parent in either case. + * Otherwise, dget_parent() safely fetches and pins the current parent + * against a concurrent rename(2). */ - old_parent = (old_dentry == mnt_dir.dentry) ? old_dentry : - old_dentry->d_parent; + old_parent = (old_dentry == mnt_dir.dentry) ? dget(old_dentry) : + dget_parent(old_dentry); /* new_dir->dentry is equal to new_dentry->d_parent */ allow_parent1 = collect_domain_accesses(subject->domain, mnt_dir.dentry, @@ -1311,8 +1312,10 @@ static int current_check_refer_path(struct dentry *const old_dentry, allow_parent2 = collect_domain_accesses(subject->domain, mnt_dir.dentry, new_dir->dentry, &layer_masks_parent2); - if (allow_parent1 && allow_parent2) + if (allow_parent1 && allow_parent2) { + dput(old_parent); return 0; + } /* * To be able to compare source and destination domain access rights, @@ -1324,8 +1327,10 @@ static int current_check_refer_path(struct dentry *const old_dentry, subject->domain, &mnt_dir, access_request_parent1, &layer_masks_parent1, &request1, old_dentry, access_request_parent2, &layer_masks_parent2, &request2, - exchange ? new_dentry : NULL)) + exchange ? new_dentry : NULL)) { + dput(old_parent); return 0; + } if (request1.access) { request1.audit.u.path.dentry = old_parent; @@ -1335,6 +1340,7 @@ static int current_check_refer_path(struct dentry *const old_dentry, request2.audit.u.path.dentry = new_dir->dentry; landlock_log_denial(subject, &request2); } + dput(old_parent); /* * This prioritizes EACCES over EXDEV for all actions, including -- cgit v1.2.3 From e7557b9ef7a87570cbd0873a163de05bde80c39b Mon Sep 17 00:00:00 2001 From: Mickaël Salaün Date: Mon, 7 Sep 2026 12:35:01 +0200 Subject: selftests/landlock: Test abstract socket trace name limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The landlock_deny_scope_abstract_unix_socket event captures binary socket names with __string_len(), whose dynamic field reserves an extra byte for the NUL terminator. The printer subtracts this byte before escaping the content. Exercise the minimum accepted address length, which has no name content, and the maximum sockaddr_un length, which has 107 content bytes. Check the exact trace output at both boundaries. The existing stream and datagram variants share this event, so the boundary variants only need the stream path. Because these boundary names are fixed, run the fixture in a private network namespace. Abstract UNIX socket names are scoped by network namespace, preventing concurrent bind() calls from colliding. The lower-bound test confirms that the subtraction recovers zero instead of underflowing. Cc: Günther Noack Link: https://patch.msgid.link/CAL4aGcVcT0VWVFmGi_vLqxxZ9KdOHfGXYZtKjBdvoUyFjbu5=A@mail.gmail.com Link: https://patch.msgid.link/20260907103503.109461-1-mic@digikod.net Signed-off-by: Mickaël Salaün --- .../selftests/landlock/scoped_abstract_unix_test.c | 76 +++++++++++++++------- 1 file changed, 54 insertions(+), 22 deletions(-) diff --git a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c index 6dbe863ea571..5dc0debacb2a 100644 --- a/tools/testing/selftests/landlock/scoped_abstract_unix_test.c +++ b/tools/testing/selftests/landlock/scoped_abstract_unix_test.c @@ -1222,7 +1222,7 @@ FIXTURE_SETUP(trace_unix) int ret; set_cap(_metadata, CAP_SYS_ADMIN); - ASSERT_EQ(0, unshare(CLONE_NEWNS)); + ASSERT_EQ(0, unshare(CLONE_NEWNS | CLONE_NEWNET)); ASSERT_EQ(0, mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL)); ret = tracefs_fixture_setup(); @@ -1252,6 +1252,11 @@ FIXTURE_TEARDOWN(trace_unix) clear_cap(_metadata, CAP_SYS_ADMIN); } +static const char + trace_unix_max_name[sizeof(((struct sockaddr_un *)0)->sun_path)] = { + [0 ... sizeof(trace_unix_max_name) - 2] = 'x', + }; + /* clang-format off */ FIXTURE_VARIANT(trace_unix) { /* clang-format on */ @@ -1259,6 +1264,8 @@ FIXTURE_VARIANT(trace_unix) { bool sandbox; bool sandbox_target; /* Peer owned by a domain: peer_domain != 0. */ int expect_denied; + const char *name; /* NULL generates a PID-based binary name. */ + size_t name_len; }; /* clang-format off */ @@ -1281,6 +1288,26 @@ FIXTURE_VARIANT_ADD(trace_unix, stream_allowed) { .sandbox_target = false, .expect_denied = 0, }; +/* Stream: lower abstract-name length boundary. */ +FIXTURE_VARIANT_ADD(trace_unix, stream_denied_empty_name) { + .sock_type = SOCK_STREAM, + .sandbox = true, + .sandbox_target = false, + .expect_denied = 1, + .name = "", + .name_len = 0, +}; + +/* Stream: upper abstract-name length boundary. */ +FIXTURE_VARIANT_ADD(trace_unix, stream_denied_max_name) { + .sock_type = SOCK_STREAM, + .sandbox = true, + .sandbox_target = false, + .expect_denied = 1, + .name = trace_unix_max_name, + .name_len = sizeof(trace_unix_max_name) - 1, +}; + /* Datagram: sandboxed client sendto() an unsandboxed peer (peer_domain=0). */ FIXTURE_VARIANT_ADD(trace_unix, dgram_denied) { .sock_type = SOCK_DGRAM, .sandbox = true, @@ -1304,12 +1331,11 @@ FIXTURE_VARIANT_ADD(trace_unix, dgram_allowed) { /* * A sandboxed thread reaching an abstract unix socket peer through connect(2) * (stream) or sendto(2) (datagram) is denied and emits - * landlock_deny_scope_abstract_unix_socket. The abstract name is crafted with - * a space and an embedded NUL followed by an "END" marker to check the - * tracepoint escaping and its length handling (a raw space would break the - * sun_path field regex; strlen() would truncate at the NUL and drop "END"). - * peer_pid is only meaningful for a stream peer (a datagram peer has no - * SO_PEERCRED), so it is asserted only there. + * landlock_deny_scope_abstract_unix_socket. The default abstract name has a + * space and an embedded NUL followed by an "END" marker to check escaping and + * binary length handling. Additional stream variants cover the minimum and + * maximum abstract-name lengths. peer_pid is only meaningful for a stream peer + * (a datagram peer has no SO_PEERCRED), so it is asserted only there. */ TEST_F(trace_unix, deny_scope_unix) { @@ -1336,12 +1362,19 @@ TEST_F(trace_unix, deny_scope_unix) ASSERT_LE(0, server_fd); addr.sun_path[0] = '\0'; - name_len = snprintf(addr.sun_path + 1, sizeof(addr.sun_path) - 1, - "landlock_trace_test_%d ", getpid()); - addr.sun_path[1 + name_len] = '\0'; - memcpy(addr.sun_path + 1 + name_len + 1, "END", 3); - addr_len = - offsetof(struct sockaddr_un, sun_path) + 1 + name_len + 1 + 3; + if (variant->name) { + ASSERT_LE(variant->name_len, sizeof(addr.sun_path) - 1); + memcpy(addr.sun_path + 1, variant->name, variant->name_len); + name_len = variant->name_len; + } else { + name_len = snprintf(addr.sun_path + 1, + sizeof(addr.sun_path) - 1, + "landlock_trace_test_%d ", getpid()); + addr.sun_path[1 + name_len] = '\0'; + memcpy(addr.sun_path + 1 + name_len + 1, "END", 3); + name_len += 1 + 3; + } + addr_len = offsetof(struct sockaddr_un, sun_path) + 1 + name_len; ASSERT_EQ(0, bind(server_fd, (struct sockaddr *)&addr, addr_len)); if (variant->sock_type == SOCK_STREAM) @@ -1430,19 +1463,18 @@ TEST_F(trace_unix, deny_scope_unix) count, buf); } - /* - * sun_path is escaped: a raw space would break this field's [^ ]*$ - * regex, so a successful extract proves the space was escaped, and its - * full length is honored: the "END" marker after the embedded NUL must - * survive (strlen() would truncate it at the NUL). - */ ASSERT_EQ(0, tracefs_extract_field( buf, REGEX_DENY_SCOPE_ABSTRACT_UNIX_SOCKET(TRACE_TASK), "sun_path", field, sizeof(field))); - EXPECT_NE(NULL, strstr(field, "END")) - { - TH_LOG("sun_path truncated or unescaped: %s", field); + if (variant->name) { + EXPECT_STREQ(variant->name, field); + } else { + /* An embedded NUL must not truncate the following marker. */ + EXPECT_NE(NULL, strstr(field, "END")) + { + TH_LOG("sun_path truncated or unescaped: %s", field); + } } /* peer_pid is the parent's PID for a stream peer (0 for datagram). */ -- cgit v1.2.3 From e1e60f71f54f2cdff687fc7a6ca3f35ad7b9c75d Mon Sep 17 00:00:00 2001 From: Mickaël Salaün Date: Mon, 7 Sep 2026 12:36:08 +0200 Subject: landlock: Clean up ruleset validation checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit landlock_merge_ruleset() checks for a NULL ruleset after dereferencing it in lockdep_assert_held(). Move the assertion after the check so the defensive path remains effective. The mask-validation comment originated in landlock_add_fs_access_mask() to explain that its WARN_ON_ONCE() checked a caller invariant. It became self-referential when this helper and its network and scope counterparts were inlined into landlock_create_ruleset(). Restate the invariant without naming the caller. Keep both as defensive callee checks. Moving the assertion preserves the NULL check's ability to warn and return -EINVAL, while invalid masks remain warned about and masked. Reported-by: Günther Noack Closes: https://patch.msgid.link/aobYhIt3vcs2xN0b@google.com Closes: https://patch.msgid.link/aobasxUDQ8b7GYXl@google.com Link: https://patch.msgid.link/20260907103609.113325-1-mic@digikod.net Signed-off-by: Mickaël Salaün --- security/landlock/domain.c | 3 ++- security/landlock/ruleset.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/security/landlock/domain.c b/security/landlock/domain.c index 93c7104fd6b2..4031b581be07 100644 --- a/security/landlock/domain.c +++ b/security/landlock/domain.c @@ -439,10 +439,11 @@ landlock_merge_ruleset(struct landlock_domain *const parent, int err; might_sleep(); - lockdep_assert_held(&ruleset->lock); if (WARN_ON_ONCE(!ruleset)) return ERR_PTR(-EINVAL); + lockdep_assert_held(&ruleset->lock); + if (parent) { if (parent->num_layers >= LANDLOCK_MAX_NUM_LAYERS) return ERR_PTR(-E2BIG); diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index 0d07707523cd..a5d135d085cb 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -58,7 +58,7 @@ landlock_create_ruleset(const access_mask_t fs_access_mask, new_ruleset->id = landlock_get_id_range(1); #endif /* CONFIG_TRACEPOINTS */ - /* Should already be checked in landlock_create_ruleset(). */ + /* The caller must only pass supported access rights and scopes. */ if (fs_access_mask) { const access_mask_t mask = fs_access_mask & LANDLOCK_MASK_ACCESS_FS; -- cgit v1.2.3