* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-26 19:22 UTC (permalink / raw)
To: david laight
Cc: Helge Deller, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <20251126104444.29002552@pumpkin>
On 11/26/25 02:44, david laight wrote:
> On Wed, 26 Nov 2025 01:11:45 -0800
> John Johansen <john.johansen@canonical.com> wrote:
>
>> On 11/25/25 13:13, Helge Deller wrote:
>>> On 11/25/25 20:20, John Johansen wrote:
>>>> On 11/25/25 07:11, Helge Deller wrote:
>>>>> * John Johansen <john.johansen@canonical.com>:
>>>>>> On 11/18/25 04:49, Helge Deller wrote:
>>>>>>> Hi Adrian,
>>>>>>>
>>>>>>> On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
>>>>>>>> On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
>>>>>>>>> My patch fixed two call sites, but I suspect you see another call site which
>>>>>>>>> hasn't been fixed yet.
>>>>>>>>>
>>>>>>>>> Can you try attached patch? It might indicate the caller of the function and
>>>>>>>>> maybe prints the struct name/address which isn't aligned.
>>>>>>>>>
>>>>>>>>> Helge
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>>>>>> index c5a91600842a..b477430c07eb 100644
>>>>>>>>> --- a/security/apparmor/match.c
>>>>>>>>> +++ b/security/apparmor/match.c
>>>>>>>>> @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>>>>>>>>> if (size < sizeof(struct table_set_header))
>>>>>>>>> goto fail;
>>>>>>>>> + if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
>>>>>>>>> + pr_warn("dfa blob stream %pS not aligned.\n", data);
>>>>>>>>> +
>>>>>>>>> if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
>>>>>>>>> goto fail;
>>>>>>>>
>>>>>>>> Here is the relevant output with the patch applied:
>>>>>>>>
>>>>>>>> [ 73.840639] ------------[ cut here ]------------
>>>>>>>> [ 73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
>>>>>>>> sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
>>>>>>>> [ 74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
>>>>>>>> [ 74.536543] Call Trace:
>>>>>>>> [ 74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
>>>>>>>> [ 74.633757] [<0000000000476438>] __warn+0xd8/0x100
>>>>>>>> [ 74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
>>>>>>>> [ 74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
>>>>>>>> [ 74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
>>>>>>>> [ 74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
>>>>>>>> [ 75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
>>>>>>>> [ 75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
>>>>>>>> [ 75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
>>>>>>>> [ 75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
>>>>>>>> [ 75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
>>>>>>>> [ 75.406932] [<0000000000767174>] sys_write+0x14/0x40
>>>>>>>> [ 75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
>>>>>>>> [ 75.548802] ---[ end trace 0000000000000000 ]---
>>>>>>>> [ 75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
>>>>>>>> [ 75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
>>>>>>>
>>>>>>> The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
>>>>>>> (via the write syscall).
>>>>>>> Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
>>>>>>> a source address which is not correctly aligned.
>>>>>>
>>>>>> the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
>>>>>> is but the userspace tooling hasn't been treating it as aligned. With that said,
>>>>>> the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
>>>>>> and there really should be some validation to catch it.
>>>>>>
>>>>>>> You should be able to debug/find the problematic code with strace from userspace.
>>>>>>> Maybe someone with apparmor knowledge here on the list has an idea?
>>>>>>>
>>>>>> This is likely an unaligned 2nd profile, being split out and loaded separately
>>>>>> from the rest of the container. Basically the loader for some reason (there
>>>>>> are a few different possible reasons) is poking into the container format and
>>>>>> pulling out the profile at some offset, this gets loaded to the kernel but
>>>>>> it would seem that its causing an issue with the dfa alignment within the container,
>>>>>> which should be aligned to the original container.
>>>>>
>>>>>
>>>>> Regarding this:
>>>>>
>>>>>> Kernel side, we are going to need to add some extra verification checks, it should
>>>>>> be catching this, as unaligned as part of the unpack. Userspace side, we will have
>>>>>> to verify my guess and fix the loader.
>>>>>
>>>>> I wonder if loading those tables are really time critical?
>>>>
>>>> no, most policy is loaded once on boot and then at package upgrades. There are some
>>>> bits that may be loaded at application startup like, snap, libvirt, lxd, basically
>>>> container managers might do some thing custom per container.
>>>>
>>>> Its the run time we want to minimize, the cost of.
>>>>
>>>> Policy already can be unaligned (the container format rework to fix this is low
>>>> priority), and is treated as untrusted. It goes through an unpack, and translation to
>>>> machine native, with as many bounds checks, necessary transforms etc done at unpack
>>>> time as possible, so that the run time costs can be minimized.
>>>>
>>>>> If not, maybe just making the kernel aware that the tables might be unaligned
>>>>> can help, e.g. with the following (untested) patch.
>>>>> Adrian, maybe you want to test?
>>>>>
>>>>
>>>>> ------------------------
>>>>>
>>>>> [PATCH] Allow apparmor to handle unaligned dfa tables
>>>>>
>>>>> The dfa tables can originate from kernel or userspace and 8-byte alignment
>>>>> isn't always guaranteed and as such may trigger unaligned memory accesses
>>>>> on various architectures.
>>>>> Work around it by using the get_unaligned_xx() helpers.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>> lgtm,
>>>>
>>>> Acked-by: John Johansen <john.johansen@canonical.com>
>>>>
>>>> I'll pull this into my tree regardless of whether it fixes the issue
>>>> for Adrian, as it definitely fixes an issue.
>>>>
>>>> We can added additional patches on top s needed.
>>>
>>> My patch does not modify the UNPACK_ARRAY() macro, which we
>>> possibly should adjust as well.
>>
>> Indeed. See the patch below. I am not surprised testing hasn't triggered this
>> case, but a malicious userspace could certainly construct a policy that would
>> trigger it. Yes it would have to be root, but I still would like to prevent
>> root from being able to trigger this.
>>
>>> Adrian's testing seems to trigger only a few unaligned accesses,
>>> so maybe it's not a issue currently.
>>>
>> I don't think the userspace compiler is generating one that is bad, but it
>> possible to construct one and get it to the point where it can trip in
>> UNPACK_ARRAY
>>
>> commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
>> Author: John Johansen <john.johansen@canonical.com>
>> Date: Tue Nov 25 13:59:32 2025 -0800
>>
>> apparmor: fix unaligned memory access of UNPACK_ARRAY
>>
>> The UNPACK_ARRAY macro has the potential to have unaligned memory
>> access when the unpacking an unaligned profile, which is caused by
>> userspace splitting up a profile container before sending it to the
>> kernel.
>>
>> While this is corner case, policy loaded from userspace should be
>> treated as untrusted so ensure that userspace can not trigger an
>> unaligned access.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b1..203f7c07529f5 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -104,7 +104,7 @@ struct aa_dfa {
>> struct table_header *tables[YYTD_ID_TSIZE];
>> };
>>
>> -#define byte_to_byte(X) (X)
>> +#define byte_to_byte(X) *(X)
>
> Even though is is only used once that ought to be (*(X))
>
>>
>> #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
>> do { \
>> @@ -112,7 +112,7 @@ struct aa_dfa {
>> TTYPE *__t = (TTYPE *) TABLE; \
>> BTYPE *__b = (BTYPE *) BLOB; \
>> for (__i = 0; __i < LEN; __i++) { \
>> - __t[__i] = NTOHX(__b[__i]); \
>> + __t[__i] = NTOHX(&__b[__i]); \
>> } \
>> } while (0)
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index 26e82ba879d44..3dcc342337aca 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>> u8, u8, byte_to_byte);
>
> Is that that just memcpy() ?
>
yeah for the byte case it is and we should just replace that case of UNPACK_ARRAY
> David
>
>> else if (th.td_flags == YYTD_DATA16)
>> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
>> - u16, __be16, be16_to_cpu);
>> + u16, __be16, get_unaligned_be16);
>> else if (th.td_flags == YYTD_DATA32)
>> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
>> - u32, __be32, be32_to_cpu);
>> + u32, __be32, get_unaligned_be32);
>> else
>> goto fail;
>> /* if table was vmalloced make sure the page tables are synced
>>
>>
>>
>
^ permalink raw reply
* [PATCH v4 2/4] landlock: Improve variable scope
From: Mickaël Salaün @ 2025-11-26 19:11 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Mikhail Ivanov, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module
In-Reply-To: <20251126191159.3530363-1-mic@digikod.net>
This is now possible thanks to the disconnected directory fix.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Cc: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- New patch extracted from the previous one.
---
security/landlock/fs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 26d5c274a4c9..ee2fa7382a9b 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -837,7 +837,6 @@ static bool is_access_to_paths_allowed(
* restriction.
*/
while (true) {
- struct dentry *parent_dentry;
const struct landlock_rule *rule;
/*
@@ -930,7 +929,9 @@ static bool is_access_to_paths_allowed(
walker_path.dentry = walker_path.mnt->mnt_root;
dget(walker_path.dentry);
} else {
- parent_dentry = dget_parent(walker_path.dentry);
+ struct dentry *const parent_dentry =
+ dget_parent(walker_path.dentry);
+
dput(walker_path.dentry);
walker_path.dentry = parent_dentry;
}
--
2.51.0
^ permalink raw reply related
* [PATCH v4 4/4] selftests/landlock: Add disconnected leafs and branch test suites
From: Mickaël Salaün @ 2025-11-26 19:11 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Mikhail Ivanov, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module
In-Reply-To: <20251126191159.3530363-1-mic@digikod.net>
Test disconnected directories with two test suites and 31 variants to
cover the main corner cases.
These tests are complementary to the previous commit.
Add test_renameat() and test_exchangeat() helpers.
Test coverage for security/landlock is 92.1% of 1927 lines according to
LLVM 20.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Cc: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- Update tests to reflect the new approach:
* layout4_disconnected_leafs.s1d41_s1d42_disconnected: allow all
* layout4_disconnected_leafs.s3d1_s4d1_new_parent: allow all
* layout4_disconnected_leafs.f1_f2_f3: allow read
* layout5_disconnected_branch.s2d3_mount1_dst_parent: allow all
* layout5_disconnected_branch.s4d1_rename_parent: allow all
- Update test coverage.
Changes since v2:
- Update test coverage.
Changes since v1:
- Rename layout4_disconnected to layout4_disconnected_leafs.
- Fix variable names.
- Add layout5_disconnected_branch test suite with 19 variants to cover
potential implementation issues.
---
tools/testing/selftests/landlock/fs_test.c | 912 +++++++++++++++++++++
1 file changed, 912 insertions(+)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 032dd5dcf5eb..7f9c97219367 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -2267,6 +2267,22 @@ static int test_exchange(const char *const oldpath, const char *const newpath)
return 0;
}
+static int test_renameat(int olddirfd, const char *oldpath, int newdirfd,
+ const char *newpath)
+{
+ if (renameat2(olddirfd, oldpath, newdirfd, newpath, 0))
+ return errno;
+ return 0;
+}
+
+static int test_exchangeat(int olddirfd, const char *oldpath, int newdirfd,
+ const char *newpath)
+{
+ if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE))
+ return errno;
+ return 0;
+}
+
TEST_F_FORK(layout1, rename_file)
{
const struct rule rules[] = {
@@ -5213,6 +5229,902 @@ TEST_F_FORK(layout1_bind, path_disconnected_link)
}
}
+/*
+ * layout4_disconnected_leafs with bind mount and renames:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the bind mount]
+ * │ ├── s1d31
+ * │ │ └── s1d41 [now renamed beneath s3d1]
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d32
+ * │ └── s1d42 [now renamed beneath s4d1]
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [bind mount of s1d2]
+ * │ ├── s1d31
+ * │ │ └── s1d41 [opened FD, now renamed beneath s3d1]
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d32
+ * │ └── s1d42 [opened FD, now renamed beneath s4d1]
+ * │ ├── f3
+ * │ └── f4
+ * ├── s3d1
+ * │ └── s1d41 [renamed here]
+ * │ ├── f1
+ * │ └── f2
+ * └── s4d1
+ * └── s1d42 [renamed here]
+ * ├── f3
+ * └── f4
+ */
+/* clang-format off */
+FIXTURE(layout4_disconnected_leafs) {
+ int s2d2_fd;
+};
+/* clang-format on */
+
+FIXTURE_SETUP(layout4_disconnected_leafs)
+{
+ prepare_layout(_metadata);
+
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f1");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f2");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f3");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f4");
+ create_directory(_metadata, TMP_DIR "/s2d1/s2d2");
+ create_directory(_metadata, TMP_DIR "/s3d1");
+ create_directory(_metadata, TMP_DIR "/s4d1");
+
+ self->s2d2_fd =
+ open(TMP_DIR "/s2d1/s2d2", O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s2d2_fd);
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0, mount(TMP_DIR "/s1d1/s1d2", TMP_DIR "/s2d1/s2d2", NULL,
+ MS_BIND, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout4_disconnected_leafs)
+{
+ /* umount(TMP_DIR "/s2d1") is handled by namespace lifetime. */
+
+ /* Removes files after renames. */
+ remove_path(TMP_DIR "/s3d1/s1d41/f1");
+ remove_path(TMP_DIR "/s3d1/s1d41/f2");
+ remove_path(TMP_DIR "/s4d1/s1d42/f1");
+ remove_path(TMP_DIR "/s4d1/s1d42/f3");
+ remove_path(TMP_DIR "/s4d1/s1d42/f4");
+ remove_path(TMP_DIR "/s4d1/s1d42/f5");
+
+ cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(layout4_disconnected_leafs)
+{
+ /*
+ * Parent of the bind mount source. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d1;
+ /*
+ * Source of bind mount (to s2d2). It should always be enforced when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d2;
+ /*
+ * Original parent of s1d41. It should always be ignored when testing
+ * against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s1d31;
+ /*
+ * Original parent of s1d42. It should always be ignored when testing
+ * against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s1d32;
+ /*
+ * Opened and disconnected source directory. It should always be enforced
+ * when testing against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s1d41;
+ /*
+ * Opened and disconnected source directory. It should always be enforced
+ * when testing against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s1d42;
+ /*
+ * File in the s1d41 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_f1;
+ /*
+ * File in the s1d41 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_f2;
+ /*
+ * File in the s1d42 disconnected directory. It should always be enforced
+ * when testing against itself under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_f3;
+ /*
+ * Parent of the bind mount destination. It should always be enforced when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s2d1;
+ /*
+ * Directory covered by the bind mount. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s2d2;
+ /*
+ * New parent of the renamed s1d41. It should always be ignored when
+ * testing against files under the s1d41 disconnected directory.
+ */
+ const __u64 allowed_s3d1;
+ /*
+ * New parent of the renamed s1d42. It should always be ignored when
+ * testing against files under the s1d42 disconnected directory.
+ */
+ const __u64 allowed_s4d1;
+
+ /* Expected result of the call to open([fd:s1d41]/f1, O_RDONLY). */
+ const int expected_read_result;
+ /* Expected result of the call to renameat([fd:s1d41]/f1, [fd:s1d42]/f1). */
+ const int expected_rename_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d41]/f2, [fd:s1d42]/f3,
+ * RENAME_EXCHANGE).
+ */
+ const int expected_exchange_result;
+ /* Expected result of the call to renameat([fd:s1d42]/f4, [fd:s1d42]/f5). */
+ const int expected_same_dir_rename_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d1_mount_src_parent) {
+ /* clang-format on */
+ .allowed_s1d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_refer) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_create) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d2_mount_src_rename) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d31_s1d32_old_parent) {
+ /* clang-format on */
+ .allowed_s1d31 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d32 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s1d41_s1d42_disconnected) {
+ /* clang-format on */
+ .allowed_s1d41 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s1d42 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_create) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_refer) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d1_mount_dst_parent_mini) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s2d2_covered_by_mount) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* Tests collect_domain_accesses(). */
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, s3d1_s4d1_new_parent) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout4_disconnected_leafs, f1_f2_f3) {
+ /* clang-format on */
+ .allowed_f1 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_f2 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .allowed_f3 = LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+TEST_F_FORK(layout4_disconnected_leafs, read_rename_exchange)
+{
+ const __u64 handled_access =
+ LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_MAKE_REG;
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR "/s1d1",
+ .access = variant->allowed_s1d1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2",
+ .access = variant->allowed_s1d2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31",
+ .access = variant->allowed_s1d31,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32",
+ .access = variant->allowed_s1d32,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41",
+ .access = variant->allowed_s1d41,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32/s1d42",
+ .access = variant->allowed_s1d42,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f1",
+ .access = variant->allowed_f1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d31/s1d41/f2",
+ .access = variant->allowed_f2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d32/s1d42/f3",
+ .access = variant->allowed_f3,
+ },
+ {
+ .path = TMP_DIR "/s2d1",
+ .access = variant->allowed_s2d1,
+ },
+ /* s2d2_fd */
+ {
+ .path = TMP_DIR "/s3d1",
+ .access = variant->allowed_s3d1,
+ },
+ {
+ .path = TMP_DIR "/s4d1",
+ .access = variant->allowed_s4d1,
+ },
+ {},
+ };
+ int ruleset_fd, s1d41_bind_fd, s1d42_bind_fd;
+
+ ruleset_fd = create_ruleset(_metadata, handled_access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds rule for the covered directory. */
+ if (variant->allowed_s2d2) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s2d2_fd,
+ .allowed_access =
+ variant->allowed_s2d2,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s2d2_fd));
+
+ s1d41_bind_fd = open(TMP_DIR "/s2d1/s2d2/s1d31/s1d41",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d41_bind_fd);
+ s1d42_bind_fd = open(TMP_DIR "/s2d1/s2d2/s1d32/s1d42",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d42_bind_fd);
+
+ /* Disconnects and checks source and destination directories. */
+ EXPECT_EQ(0, test_open_rel(s1d41_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(0, test_open_rel(s1d42_bind_fd, "..", O_DIRECTORY));
+ /* Renames to make it accessible through s3d1/s1d41 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d31/s1d41",
+ AT_FDCWD, TMP_DIR "/s3d1/s1d41"));
+ /* Renames to make it accessible through s4d1/s1d42 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d32/s1d42",
+ AT_FDCWD, TMP_DIR "/s4d1/s1d42"));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d41_bind_fd, "..", O_DIRECTORY));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d42_bind_fd, "..", O_DIRECTORY));
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ EXPECT_EQ(variant->expected_read_result,
+ test_open_rel(s1d41_bind_fd, "f1", O_RDONLY));
+
+ EXPECT_EQ(variant->expected_rename_result,
+ test_renameat(s1d41_bind_fd, "f1", s1d42_bind_fd, "f1"));
+ EXPECT_EQ(variant->expected_exchange_result,
+ test_exchangeat(s1d41_bind_fd, "f2", s1d42_bind_fd, "f3"));
+
+ EXPECT_EQ(variant->expected_same_dir_rename_result,
+ test_renameat(s1d42_bind_fd, "f4", s1d42_bind_fd, "f5"));
+}
+
+/*
+ * layout5_disconnected_branch before rename:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the first bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [source of the second bind mount]
+ * │ └── s2d3
+ * │ └── s2d4 [first s1d2 bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s3d1
+ * │ └── s3d2 [second s2d2 bind mount]
+ * │ └── s2d3
+ * │ └── s2d4 [first s1d2 bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * └── s4d1
+ *
+ * After rename:
+ *
+ * tmp
+ * ├── s1d1
+ * │ └── s1d2 [source of the first bind mount]
+ * │ └── s1d3
+ * │ ├── s1d41
+ * │ │ ├── f1
+ * │ │ └── f2
+ * │ └── s1d42
+ * │ ├── f3
+ * │ └── f4
+ * ├── s2d1
+ * │ └── s2d2 [source of the second bind mount]
+ * ├── s3d1
+ * │ └── s3d2 [second s2d2 bind mount]
+ * └── s4d1
+ * └── s2d3 [renamed here]
+ * └── s2d4 [first s1d2 bind mount]
+ * └── s1d3
+ * ├── s1d41
+ * │ ├── f1
+ * │ └── f2
+ * └── s1d42
+ * ├── f3
+ * └── f4
+ *
+ * Decision path: s1d3 -> s1d2 -> s2d2 -> s3d1 -> tmp
+ * s2d3 is ignored, as well as the directories under the mount points.
+ */
+
+/* clang-format off */
+FIXTURE(layout5_disconnected_branch) {
+ int s2d4_fd, s3d2_fd;
+};
+/* clang-format on */
+
+FIXTURE_SETUP(layout5_disconnected_branch)
+{
+ prepare_layout(_metadata);
+
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f1");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f2");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f3");
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f4");
+ create_directory(_metadata, TMP_DIR "/s2d1/s2d2/s2d3/s2d4");
+ create_directory(_metadata, TMP_DIR "/s3d1/s3d2");
+ create_directory(_metadata, TMP_DIR "/s4d1");
+
+ self->s2d4_fd = open(TMP_DIR "/s2d1/s2d2/s2d3/s2d4",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s2d4_fd);
+
+ self->s3d2_fd =
+ open(TMP_DIR "/s3d1/s3d2", O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, self->s3d2_fd);
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0, mount(TMP_DIR "/s1d1/s1d2", TMP_DIR "/s2d1/s2d2/s2d3/s2d4",
+ NULL, MS_BIND, NULL));
+ ASSERT_EQ(0, mount(TMP_DIR "/s2d1/s2d2", TMP_DIR "/s3d1/s3d2", NULL,
+ MS_BIND | MS_REC, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout5_disconnected_branch)
+{
+ /* Bind mounts are handled by namespace lifetime. */
+
+ /* Removes files after renames. */
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f1");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d41/f2");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f1");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f3");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f4");
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d42/f5");
+
+ cleanup_layout(_metadata);
+}
+
+FIXTURE_VARIANT(layout5_disconnected_branch)
+{
+ /*
+ * Parent of all files. It should always be enforced when testing against
+ * files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_base;
+ /*
+ * Parent of the first bind mount source. It should always be ignored when
+ * testing against files under the s1d41 or s1d42 disconnected directories.
+ */
+ const __u64 allowed_s1d1;
+ const __u64 allowed_s1d2;
+ const __u64 allowed_s1d3;
+ const __u64 allowed_s2d1;
+ const __u64 allowed_s2d2;
+ const __u64 allowed_s2d3;
+ const __u64 allowed_s2d4;
+ const __u64 allowed_s3d1;
+ const __u64 allowed_s3d2;
+ const __u64 allowed_s4d1;
+
+ /* Expected result of the call to open([fd:s1d3]/s1d41/f1, O_RDONLY). */
+ const int expected_read_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d41/f1,
+ * [fd:s1d3]/s1d42/f1).
+ */
+ const int expected_rename_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d41/f2,
+ * [fd:s1d3]/s1d42/f3, RENAME_EXCHANGE).
+ */
+ const int expected_exchange_result;
+ /*
+ * Expected result of the call to renameat([fd:s1d3]/s1d42/f4,
+ * [fd:s1d3]/s1d42/f5).
+ */
+ const int expected_same_dir_rename_result;
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d1_mount1_src_parent) {
+ /* clang-format on */
+ .allowed_s1d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_refer) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_create) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d2_mount1_src_rename) {
+ /* clang-format on */
+ .allowed_s1d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_refer) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_create) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_rename) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s1d3_fd_full) {
+ /* clang-format on */
+ .allowed_s1d3 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d1_mount2_src_parent) {
+ /* clang-format on */
+ .allowed_s2d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_refer) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_create) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d2_mount2_src_rename) {
+ /* clang-format on */
+ .allowed_s2d2 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d3_mount1_dst_parent) {
+ /* clang-format on */
+ .allowed_s2d3 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s2d4_mount1_dst) {
+ /* clang-format on */
+ .allowed_s2d4 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_refer) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_create) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = EXDEV,
+ .expected_exchange_result = EXDEV,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d1_mount2_dst_parent_rename) {
+ /* clang-format on */
+ .allowed_s3d1 = LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s3d2_mount1_dst) {
+ /* clang-format on */
+ .allowed_s3d2 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = EACCES,
+ .expected_same_dir_rename_result = EACCES,
+ .expected_rename_result = EACCES,
+ .expected_exchange_result = EACCES,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(layout5_disconnected_branch, s4d1_rename_parent) {
+ /* clang-format on */
+ .allowed_s4d1 = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE |
+ LANDLOCK_ACCESS_FS_MAKE_REG,
+ .expected_read_result = 0,
+ .expected_same_dir_rename_result = 0,
+ .expected_rename_result = 0,
+ .expected_exchange_result = 0,
+};
+
+TEST_F_FORK(layout5_disconnected_branch, read_rename_exchange)
+{
+ const __u64 handled_access =
+ LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_MAKE_REG;
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR "/s1d1",
+ .access = variant->allowed_s1d1,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2",
+ .access = variant->allowed_s1d2,
+ },
+ {
+ .path = TMP_DIR "/s1d1/s1d2/s1d3",
+ .access = variant->allowed_s1d3,
+ },
+ {
+ .path = TMP_DIR "/s2d1",
+ .access = variant->allowed_s2d1,
+ },
+ {
+ .path = TMP_DIR "/s2d1/s2d2",
+ .access = variant->allowed_s2d2,
+ },
+ {
+ .path = TMP_DIR "/s2d1/s2d2/s2d3",
+ .access = variant->allowed_s2d3,
+ },
+ /* s2d4_fd */
+ {
+ .path = TMP_DIR "/s3d1",
+ .access = variant->allowed_s3d1,
+ },
+ /* s3d2_fd */
+ {
+ .path = TMP_DIR "/s4d1",
+ .access = variant->allowed_s4d1,
+ },
+ {},
+ };
+ int ruleset_fd, s1d3_bind_fd;
+
+ ruleset_fd = create_ruleset(_metadata, handled_access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds rules for the covered directories. */
+ if (variant->allowed_s2d4) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s2d4_fd,
+ .allowed_access =
+ variant->allowed_s2d4,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s2d4_fd));
+
+ if (variant->allowed_s3d2) {
+ ASSERT_EQ(0, landlock_add_rule(
+ ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &(struct landlock_path_beneath_attr){
+ .parent_fd = self->s3d2_fd,
+ .allowed_access =
+ variant->allowed_s3d2,
+ },
+ 0));
+ }
+ EXPECT_EQ(0, close(self->s3d2_fd));
+
+ s1d3_bind_fd = open(TMP_DIR "/s3d1/s3d2/s2d3/s2d4/s1d3",
+ O_DIRECTORY | O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, s1d3_bind_fd);
+
+ /* Disconnects and checks source and destination directories. */
+ EXPECT_EQ(0, test_open_rel(s1d3_bind_fd, "../../..", O_DIRECTORY));
+ /* Renames to make it accessible through s3d1/s1d41 */
+ ASSERT_EQ(0, test_renameat(AT_FDCWD, TMP_DIR "/s2d1/s2d2/s2d3",
+ AT_FDCWD, TMP_DIR "/s4d1/s2d3"));
+ EXPECT_EQ(ENOENT, test_open_rel(s1d3_bind_fd, "../../..", O_DIRECTORY));
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ EXPECT_EQ(variant->expected_read_result,
+ test_open_rel(s1d3_bind_fd, "s1d41/f1", O_RDONLY));
+
+ EXPECT_EQ(variant->expected_rename_result,
+ test_renameat(s1d3_bind_fd, "s1d41/f1", s1d3_bind_fd,
+ "s1d42/f1"));
+ EXPECT_EQ(variant->expected_exchange_result,
+ test_exchangeat(s1d3_bind_fd, "s1d41/f2", s1d3_bind_fd,
+ "s1d42/f3"));
+
+ EXPECT_EQ(variant->expected_same_dir_rename_result,
+ test_renameat(s1d3_bind_fd, "s1d42/f4", s1d3_bind_fd,
+ "s1d42/f5"));
+}
+
#define LOWER_BASE TMP_DIR "/lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
--
2.51.0
^ permalink raw reply related
* [PATCH v4 3/4] selftests/landlock: Add tests for access through disconnected paths
From: Mickaël Salaün @ 2025-11-26 19:11 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Al Viro, Ben Scarlato, Christian Brauner, Jann Horn, Jeff Xu,
Justin Suess, Mikhail Ivanov, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module, Mickaël Salaün
In-Reply-To: <20251126191159.3530363-1-mic@digikod.net>
From: Tingmao Wang <m@maowtm.org>
This adds tests for the edge case discussed in [1], with specific ones
for rename and link operations when the operands are through
disconnected paths, as that go through a separate code path in Landlock.
This has resulted in a warning, due to collect_domain_accesses() not
expecting to reach a different root from path->mnt:
# RUN layout1_bind.path_disconnected ...
# OK layout1_bind.path_disconnected
ok 96 layout1_bind.path_disconnected
# RUN layout1_bind.path_disconnected_rename ...
[..] ------------[ cut here ]------------
[..] WARNING: CPU: 3 PID: 385 at security/landlock/fs.c:1065 collect_domain_accesses
[..] ...
[..] RIP: 0010:collect_domain_accesses (security/landlock/fs.c:1065 (discriminator 2) security/landlock/fs.c:1031 (discriminator 2))
[..] current_check_refer_path (security/landlock/fs.c:1205)
[..] ...
[..] hook_path_rename (security/landlock/fs.c:1526)
[..] security_path_rename (security/security.c:2026 (discriminator 1))
[..] do_renameat2 (fs/namei.c:5264)
# OK layout1_bind.path_disconnected_rename
ok 97 layout1_bind.path_disconnected_rename
Move the const char definitions a bit above so that we can use the path
for s4d1 in cleanup code.
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Link: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org [1]
Signed-off-by: Tingmao Wang <m@maowtm.org>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- Update layout1_bind.path_disconnected tests according to the semantic
change of the fix.
- Update documentation for the layout1_bind hierarchy with the new s4d1
directory.
Changes since v1:
- Integrate this patch into my patch series, and change the result for
two tests with updated comments. Diff here:
https://lore.kernel.org/r/20250701183812.3201231-2-mic@digikod.net
- Replace most ASSERT with EXPECT, add extra checks, massage commit
message and comments.
- Squash Tingmao's patches:
https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org
https://lore.kernel.org/r/8ed0bfcd-aefa-44bd-86b6-e12583779187@maowtm.org
https://lore.kernel.org/r/3080e512-64b0-42cf-b379-8f52cfeff78a@maowtm.org
---
tools/testing/selftests/landlock/fs_test.c | 423 ++++++++++++++++++++-
1 file changed, 415 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index fa0f18ec62c4..032dd5dcf5eb 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4561,6 +4561,18 @@ TEST_F_FORK(ioctl, handle_file_access_file)
FIXTURE(layout1_bind) {};
/* clang-format on */
+static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
+static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
+
+/* Move targets for disconnected path tests. */
+static const char dir_s4d1[] = TMP_DIR "/s4d1";
+static const char file1_s4d1[] = TMP_DIR "/s4d1/f1";
+static const char file2_s4d1[] = TMP_DIR "/s4d1/f2";
+static const char dir_s4d2[] = TMP_DIR "/s4d1/s4d2";
+static const char file1_s4d2[] = TMP_DIR "/s4d1/s4d2/f1";
+static const char file1_name[] = "f1";
+static const char file2_name[] = "f2";
+
FIXTURE_SETUP(layout1_bind)
{
prepare_layout(_metadata);
@@ -4576,14 +4588,14 @@ FIXTURE_TEARDOWN_PARENT(layout1_bind)
{
/* umount(dir_s2d2)) is handled by namespace lifetime. */
+ remove_path(file1_s4d1);
+ remove_path(file2_s4d1);
+
remove_layout1(_metadata);
cleanup_layout(_metadata);
}
-static const char bind_dir_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3";
-static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
-
/*
* layout1_bind hierarchy:
*
@@ -4594,20 +4606,25 @@ static const char bind_file1_s1d3[] = TMP_DIR "/s2d1/s2d2/s1d3/f1";
* │ └── s1d2
* │ ├── f1
* │ ├── f2
- * │ └── s1d3
+ * │ └── s1d3 [disconnected by path_disconnected]
* │ ├── f1
* │ └── f2
* ├── s2d1
* │ ├── f1
- * │ └── s2d2
+ * │ └── s2d2 [bind mount from s1d2]
* │ ├── f1
* │ ├── f2
* │ └── s1d3
* │ ├── f1
* │ └── f2
- * └── s3d1
- * └── s3d2
- * └── s3d3
+ * ├── s3d1
+ * │ └── s3d2
+ * │ └── s3d3
+ * └── s4d1 [renamed from s1d3 by path_disconnected]
+ * ├── f1
+ * ├── f2
+ * └── s4d2
+ * └── f1
*/
TEST_F_FORK(layout1_bind, no_restriction)
@@ -4806,6 +4823,396 @@ TEST_F_FORK(layout1_bind, reparent_cross_mount)
ASSERT_EQ(0, rename(bind_file1_s1d3, file1_s2d2));
}
+/*
+ * Make sure access to file through a disconnected path works as expected.
+ * This test moves s1d3 to s4d1.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected)
+{
+ const struct rule layer1_allow_all[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_ALL,
+ },
+ {},
+ };
+ const struct rule layer2_allow_just_f1[] = {
+ {
+ .path = file1_s1d3,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+ const struct rule layer3_only_s1d2[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+
+ /* Landlock should not deny access just because it is disconnected. */
+ int ruleset_fd_l1 =
+ create_ruleset(_metadata, ACCESS_ALL, layer1_allow_all);
+
+ /* Creates the new ruleset now before we move the dir containing the file. */
+ int ruleset_fd_l2 =
+ create_ruleset(_metadata, ACCESS_RW, layer2_allow_just_f1);
+ int ruleset_fd_l3 =
+ create_ruleset(_metadata, ACCESS_RW, layer3_only_s1d2);
+ int bind_s1d3_fd;
+
+ ASSERT_LE(0, ruleset_fd_l1);
+ ASSERT_LE(0, ruleset_fd_l2);
+ ASSERT_LE(0, ruleset_fd_l3);
+
+ enforce_ruleset(_metadata, ruleset_fd_l1);
+ EXPECT_EQ(0, close(ruleset_fd_l1));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+
+ /* Tests access is possible before we move. */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
+
+ /* Makes it disconnected. */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+ strerror(errno));
+ }
+
+ /* Tests that access is still possible. */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+
+ /*
+ * Tests that ".." is not possible (not because of Landlock, but just
+ * because it's disconnected).
+ */
+ EXPECT_EQ(ENOENT,
+ test_open_rel(bind_s1d3_fd, "..", O_RDONLY | O_DIRECTORY));
+
+ /* This should still work with a narrower rule. */
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY));
+ /*
+ * Accessing a file through a disconnected file descriptor can still be
+ * allowed by a rule tied to this file, even if it is no longer visible in
+ * its mount point.
+ */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+
+ enforce_ruleset(_metadata, ruleset_fd_l3);
+ EXPECT_EQ(0, close(ruleset_fd_l3));
+
+ EXPECT_EQ(EACCES, test_open(file1_s4d1, O_RDONLY));
+ /*
+ * Accessing a file through a disconnected file descriptor can still be
+ * allowed by a rule tied to the original mount point, even if it is no
+ * longer visible in its mount point.
+ */
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+}
+
+/*
+ * Test that renameat with disconnected paths works under Landlock. This test
+ * moves s1d3 to s4d2, so that we can have a rule allowing refers on the move
+ * target's immediate parent.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_rename)
+{
+ const struct rule layer1[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_DIR |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {
+ .path = dir_s4d1,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_MAKE_DIR |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {}
+ };
+
+ /* This layer only handles LANDLOCK_ACCESS_FS_READ_FILE. */
+ const struct rule layer2_only_s1d2[] = {
+ {
+ .path = dir_s1d2,
+ .access = LANDLOCK_ACCESS_FS_READ_FILE,
+ },
+ {},
+ };
+ int ruleset_fd_l1, ruleset_fd_l2;
+ pid_t child_pid;
+ int bind_s1d3_fd, status;
+
+ ASSERT_EQ(0, mkdir(dir_s4d1, 0755))
+ {
+ TH_LOG("Failed to create %s: %s", dir_s4d1, strerror(errno));
+ }
+ ruleset_fd_l1 = create_ruleset(_metadata, ACCESS_ALL, layer1);
+ ruleset_fd_l2 = create_ruleset(_metadata, LANDLOCK_ACCESS_FS_READ_FILE,
+ layer2_only_s1d2);
+ ASSERT_LE(0, ruleset_fd_l1);
+ ASSERT_LE(0, ruleset_fd_l2);
+
+ enforce_ruleset(_metadata, ruleset_fd_l1);
+ EXPECT_EQ(0, close(ruleset_fd_l1));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+ /* Tests ENOENT priority over EACCES for disconnected directory. */
+ EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+ strerror(errno));
+ }
+ EXPECT_EQ(ENOENT, test_open_rel(bind_s1d3_fd, "..", O_DIRECTORY));
+
+ /*
+ * The file is no longer under s1d2 but we should still be able to access it
+ * with layer 2 because its mount point is evaluated as the first valid
+ * directory because it was initially a parent. Do a fork to test this so
+ * we don't prevent ourselves from renaming it back later.
+ */
+ child_pid = fork();
+ ASSERT_LE(0, child_pid);
+ if (child_pid == 0) {
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s4d2, O_RDONLY));
+
+ /*
+ * Tests that access widening checks indeed prevents us from renaming it
+ * back.
+ */
+ EXPECT_EQ(-1, rename(dir_s4d2, dir_s1d3));
+ EXPECT_EQ(EXDEV, errno);
+
+ /*
+ * Including through the now disconnected fd (but it should return
+ * EXDEV).
+ */
+ EXPECT_EQ(-1, renameat(bind_s1d3_fd, file1_name, AT_FDCWD,
+ file1_s2d2));
+ EXPECT_EQ(EXDEV, errno);
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ EXPECT_EQ(child_pid, waitpid(child_pid, &status, 0));
+ EXPECT_EQ(1, WIFEXITED(status));
+ EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+ ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+ {
+ TH_LOG("Failed to rename %s back to %s: %s", dir_s4d1, dir_s1d3,
+ strerror(errno));
+ }
+
+ /* Now checks that we can access it under l2. */
+ child_pid = fork();
+ ASSERT_LE(0, child_pid);
+ if (child_pid == 0) {
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+ _exit(_metadata->exit_code);
+ return;
+ }
+
+ EXPECT_EQ(child_pid, waitpid(child_pid, &status, 0));
+ EXPECT_EQ(1, WIFEXITED(status));
+ EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+ /*
+ * Also test that we can rename via a disconnected path. We move the
+ * dir back to the disconnected place first, then we rename file1 to
+ * file2 through our dir fd.
+ */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+ strerror(errno));
+ }
+ ASSERT_EQ(0,
+ renameat(bind_s1d3_fd, file1_name, bind_s1d3_fd, file2_name))
+ {
+ TH_LOG("Failed to rename %s to %s within disconnected %s: %s",
+ file1_name, file2_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file2_name, O_RDONLY));
+ ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
+ {
+ TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+ file2_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s2d2, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d2, O_RDONLY));
+
+ /* Move it back using the disconnected path as the target. */
+ ASSERT_EQ(0, renameat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file1_name))
+ {
+ TH_LOG("Failed to rename %s to %s through disconnected %s: %s",
+ file1_s1d2, file1_name, bind_dir_s1d3, strerror(errno));
+ }
+
+ /* Now make it connected again. */
+ ASSERT_EQ(0, rename(dir_s4d2, dir_s1d3))
+ {
+ TH_LOG("Failed to rename %s back to %s: %s", dir_s4d2, dir_s1d3,
+ strerror(errno));
+ }
+
+ /* Checks again that we can access it under l2. */
+ enforce_ruleset(_metadata, ruleset_fd_l2);
+ EXPECT_EQ(0, close(ruleset_fd_l2));
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+}
+
+/*
+ * Test that linkat(2) with disconnected paths works under Landlock. This
+ * test moves s1d3 to s4d1.
+ */
+TEST_F_FORK(layout1_bind, path_disconnected_link)
+{
+ /* Ruleset to be applied after renaming s1d3 to s4d1. */
+ const struct rule layer1[] = {
+ {
+ .path = dir_s4d1,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {
+ .path = dir_s2d2,
+ .access = LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_READ_FILE |
+ LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {}
+ };
+ int ruleset_fd, bind_s1d3_fd;
+
+ /* Removes unneeded files created by layout1, otherwise it will EEXIST. */
+ ASSERT_EQ(0, unlink(file1_s1d2));
+ ASSERT_EQ(0, unlink(file2_s1d3));
+
+ bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, bind_s1d3_fd);
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+ /* Disconnects bind_s1d3_fd. */
+ ASSERT_EQ(0, rename(dir_s1d3, dir_s4d1))
+ {
+ TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d1,
+ strerror(errno));
+ }
+
+ /* Need this later to test different parent link. */
+ ASSERT_EQ(0, mkdir(dir_s4d2, 0755))
+ {
+ TH_LOG("Failed to create %s: %s", dir_s4d2, strerror(errno));
+ }
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ /* From disconnected to connected. */
+ ASSERT_EQ(0, linkat(bind_s1d3_fd, file1_name, AT_FDCWD, file1_s2d2, 0))
+ {
+ TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+ file1_name, file1_s2d2, bind_dir_s1d3, strerror(errno));
+ }
+
+ /* Tests that we can access via the new link... */
+ EXPECT_EQ(0, test_open(file1_s2d2, O_RDONLY))
+ {
+ TH_LOG("Failed to open newly linked %s: %s", file1_s2d2,
+ strerror(errno));
+ }
+
+ /* ...as well as the old one. */
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+ {
+ TH_LOG("Failed to open original %s: %s", file1_s4d1,
+ strerror(errno));
+ }
+
+ /* From connected to disconnected. */
+ ASSERT_EQ(0, unlink(file1_s4d1));
+ ASSERT_EQ(0, linkat(AT_FDCWD, file1_s2d2, bind_s1d3_fd, file2_name, 0))
+ {
+ TH_LOG("Failed to link %s to %s via disconnected %s: %s",
+ file1_s2d2, file2_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file2_s4d1, O_RDONLY));
+ ASSERT_EQ(0, unlink(file1_s2d2));
+
+ /* From disconnected to disconnected (same parent). */
+ ASSERT_EQ(0,
+ linkat(bind_s1d3_fd, file2_name, bind_s1d3_fd, file1_name, 0))
+ {
+ TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+ file2_name, file1_name, bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s4d1, O_RDONLY))
+ {
+ TH_LOG("Failed to open newly linked %s: %s", file1_s4d1,
+ strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY))
+ {
+ TH_LOG("Failed to open %s through newly created link under disconnected path: %s",
+ file1_name, strerror(errno));
+ }
+ ASSERT_EQ(0, unlink(file2_s4d1));
+
+ /* From disconnected to disconnected (different parent). */
+ ASSERT_EQ(0,
+ linkat(bind_s1d3_fd, file1_name, bind_s1d3_fd, "s4d2/f1", 0))
+ {
+ TH_LOG("Failed to link %s to %s within disconnected %s: %s",
+ file1_name, "s4d2/f1", bind_dir_s1d3, strerror(errno));
+ }
+ EXPECT_EQ(0, test_open(file1_s4d2, O_RDONLY))
+ {
+ TH_LOG("Failed to open %s after link: %s", file1_s4d2,
+ strerror(errno));
+ }
+ EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, "s4d2/f1", O_RDONLY))
+ {
+ TH_LOG("Failed to open %s through disconnected path after link: %s",
+ "s4d2/f1", strerror(errno));
+ }
+}
+
#define LOWER_BASE TMP_DIR "/lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
--
2.51.0
^ permalink raw reply related
* [PATCH v4 1/4] landlock: Fix handling of disconnected directories
From: Mickaël Salaün @ 2025-11-26 19:11 UTC (permalink / raw)
To: Günther Noack, Tingmao Wang
Cc: Mickaël Salaün, Al Viro, Ben Scarlato,
Christian Brauner, Jann Horn, Jeff Xu, Justin Suess,
Mikhail Ivanov, Paul Moore, Song Liu, linux-fsdevel,
linux-security-module
In-Reply-To: <20251126191159.3530363-1-mic@digikod.net>
Disconnected files or directories can appear when they are visible and
opened from a bind mount, but have been renamed or moved from the source
of the bind mount in a way that makes them inaccessible from the mount
point (i.e. out of scope).
Previously, access rights tied to files or directories opened through a
disconnected directory were collected by walking the related hierarchy
down to the root of the filesystem, without taking into account the
mount point because it couldn't be found. This could lead to
inconsistent access results, potential access right widening, and
hard-to-debug renames, especially since such paths cannot be printed.
For a sandboxed task to create a disconnected directory, it needs to
have write access (i.e. FS_MAKE_REG, FS_REMOVE_FILE, and FS_REFER) to
the underlying source of the bind mount, and read access to the related
mount point. Because a sandboxed task cannot acquire more access
rights than those defined by its Landlock domain, this could lead to
inconsistent access rights due to missing permissions that should be
inherited from the mount point hierarchy, while inheriting permissions
from the filesystem hierarchy hidden by this mount point instead.
Landlock now handles files and directories opened from disconnected
directories by taking into account the filesystem hierarchy when the
mount point is not found in the hierarchy walk, and also always taking
into account the mount point from which these disconnected directories
were opened. This ensures that a rename is not allowed if it would
widen access rights [1].
The rationale is that, even if disconnected hierarchies might not be
visible or accessible to a sandboxed task, relying on the collected
access rights from them improves the guarantee that access rights will
not be widened during a rename because of the access right comparison
between the source and the destination (see LANDLOCK_ACCESS_FS_REFER).
It may look like this would grant more access on disconnected files and
directories, but the security policies are always enforced for all the
evaluated hierarchies. This new behavior should be less surprising to
users and safer from an access control perspective.
Remove a wrong WARN_ON_ONCE() canary in collect_domain_accesses() and
fix the related comment.
Because opened files have their access rights stored in the related file
security properties, there is no impact for disconnected or unlinked
files.
Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Reported-by: Tingmao Wang <m@maowtm.org>
Closes: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org
Closes: https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org
Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
Link: https://lore.kernel.org/r/b0f46246-f2c5-42ca-93ce-0d629702a987@maowtm.org [1]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- Simplify the approach by checking both the filesystem hierarchy and
the mount point hierarchy for disconnected directories. Remove the
related snapshot mechanism. See [1].
- Update erratum.
- Do not expose path_connected() anymore, and remove related Acked-by.
Changes since v2:
- Fix domain check mode reset, spotted by Tingmao. Replace a
conditional branch (when all accesses are granted) by only looking for
a rule when needed. This simplifies code and remove a call to
path_connected(). Add a new is_dom_check_bkp to safely handle race
conditions and move initial backup for layer_masks_parent1.
- Fix layer masks parent backup initialization, spotted by Tingmao.
- Add more comments, suggested by Tingmao.
- Reformat erratum.
Changes since v1:
- Remove useless else branch in is_access_to_paths_allowed().
- Update commit message and squash "landlock: Remove warning in
collect_domain_accesses()":
https://lore.kernel.org/r/20250618134734.1673254-1-mic@digikod.net
- Remove "extern" for path_connected() in fs.h, requested by Christian.
- Add Acked-by Christian.
- Fix docstring and improve doc for collect_domain_accesses().
- Remove path_connected() check for the real root.
- Fix allowed_parent* resets to be consistent with their initial values
infered from the evaluated domain.
- Add Landlock erratum.
Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack@google.com>
Cc: Song Liu <song@kernel.org>
Reported-by: Tingmao Wang <m@maowtm.org>
Closes: https://lore.kernel.org/r/027d5190-b37a-40a8-84e9-4ccbc352bcdf@maowtm.org
Closes: https://lore.kernel.org/r/09b24128f86973a6022e6aa8338945fcfb9a33e4.1749925391.git.m@maowtm.org
Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
Fixes: cb2c7d1a1776 ("landlock: Support filesystem access-control")
Link: https://lore.kernel.org/r/b0f46246-f2c5-42ca-93ce-0d629702a987@maowtm.org [1]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
Changes since v3:
- Simplify the approach by checking both the filesystem hierarchy and
the mount point hierarchy for disconnected directories. Remove the
related snapshot mechanism. See [1].
- Do not expose path_connected() anymore.
Changes since v2:
- Fix domain check mode reset, spotted by Tingmao. Replace a
conditional branch (when all accesses are granted) by only looking for
a rule when needed. This simplifies code and remove a call to
path_connected(). Add a new is_dom_check_bkp to safely handle race
conditions and move initial backup for layer_masks_parent1.
- Fix layer masks parent backup initialization, spotted by Tingmao.
- Add more comments, suggested by Tingmao.
- Reformat erratum.
Changes since v1:
- Remove useless else branch in is_access_to_paths_allowed().
- Update commit message and squash "landlock: Remove warning in
collect_domain_accesses()":
https://lore.kernel.org/r/20250618134734.1673254-1-mic@digikod.net
- Remove "extern" for path_connected() in fs.h, requested by Christian.
- Add Acked-by Christian.
- Fix docstring and improve doc for collect_domain_accesses().
- Remove path_connected() check for the real root.
- Fix allowed_parent* resets to be consistent with their initial values
infered from the evaluated domain.
- Add Landlock erratum.
---
security/landlock/errata/abi-1.h | 16 +++++++++++++
security/landlock/fs.c | 40 ++++++++++++++++++++++----------
2 files changed, 44 insertions(+), 12 deletions(-)
create mode 100644 security/landlock/errata/abi-1.h
diff --git a/security/landlock/errata/abi-1.h b/security/landlock/errata/abi-1.h
new file mode 100644
index 000000000000..e8a2bff2e5b6
--- /dev/null
+++ b/security/landlock/errata/abi-1.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/**
+ * DOC: erratum_3
+ *
+ * Erratum 3: Disconnected directory handling
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This fix addresses an issue with disconnected directories that occur when a
+ * directory is moved outside the scope of a bind mount. The change ensures
+ * that evaluated access rights include both those from the disconnected file
+ * hierarchy down to its filesystem root and those from the related mount point
+ * hierarchy. This prevents access right widening through rename or link
+ * actions.
+ */
+LANDLOCK_ERRATUM(3)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index d9c12b993fa7..26d5c274a4c9 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -909,21 +909,31 @@ static bool is_access_to_paths_allowed(
break;
}
}
+
if (unlikely(IS_ROOT(walker_path.dentry))) {
- /*
- * Stops at disconnected root directories. Only allows
- * access to internal filesystems (e.g. nsfs, which is
- * reachable through /proc/<pid>/ns/<namespace>).
- */
- if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
+ if (likely(walker_path.mnt->mnt_flags & MNT_INTERNAL)) {
+ /*
+ * Stops and allows access when reaching disconnected root
+ * directories that are part of internal filesystems (e.g. nsfs,
+ * which is reachable through /proc/<pid>/ns/<namespace>).
+ */
allowed_parent1 = true;
allowed_parent2 = true;
+ break;
}
- break;
+
+ /*
+ * We reached a disconnected root directory from a bind mount.
+ * Let's continue the walk with the current mount root.
+ */
+ dput(walker_path.dentry);
+ walker_path.dentry = walker_path.mnt->mnt_root;
+ dget(walker_path.dentry);
+ } else {
+ parent_dentry = dget_parent(walker_path.dentry);
+ dput(walker_path.dentry);
+ walker_path.dentry = parent_dentry;
}
- parent_dentry = dget_parent(walker_path.dentry);
- dput(walker_path.dentry);
- walker_path.dentry = parent_dentry;
}
path_put(&walker_path);
@@ -1021,6 +1031,9 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)
* file. While walking from @dir to @mnt_root, we record all the domain's
* allowed accesses in @layer_masks_dom.
*
+ * Because of disconnected directories, this walk may not reach @mnt_dir. In
+ * this case, the walk will continue to @mnt_dir after this call.
+ *
* This is similar to is_access_to_paths_allowed() but much simpler because it
* only handles walking on the same mount point and only checks one set of
* accesses.
@@ -1062,8 +1075,11 @@ static bool collect_domain_accesses(
break;
}
- /* We should not reach a root other than @mnt_root. */
- if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir)))
+ /*
+ * Stops at the mount point or the filesystem root for a disconnected
+ * directory.
+ */
+ if (dir == mnt_root || unlikely(IS_ROOT(dir)))
break;
parent_dentry = dget_parent(dir);
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v15 3/4] lsm: count the LSMs enabled at compile time
From: Paul Moore @ 2025-11-26 19:02 UTC (permalink / raw)
To: Andy Shevchenko, KP Singh
Cc: linux-security-module, bpf, ast, casey, andrii, keescook, daniel,
renauld, revest, song, linux, Kui-Feng Lee, John Johansen
In-Reply-To: <aSc1aAdOeSuuoKTH@black.igk.intel.com>
On November 26, 2025 12:14:21 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> On Fri, Aug 16, 2024 at 05:43:06PM +0200, KP Singh wrote:
>> These macros are a clever trick to determine a count of the number of
>> LSMs that are enabled in the config to ascertain the maximum number of
>> static calls that need to be configured per LSM hook.
>>
>> Without this one would need to generate static calls for the total
>> number of LSMs in the kernel (even if they are not compiled) times the
>> number of LSM hooks which ends up being quite wasteful.
>
> ...
>
>> -/* This counts to 12. Any more, it will return 13th argument. */
>> -#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
>> _12, _n, X...) _n
>> -#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4,
>> 3, 2, 1, 0)
>> +/* This counts to 15. Any more, it will return 16th argument. */
>> +#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11,
>> _12, _13, _14, _15, _n, X...) _n
>> +#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 15, 14, 13, 12, 11, 10, 9, 8,
>> 7, 6, 5, 4, 3, 2, 1, 0)
>
> You forgot to update _12 in the upper comment.
Do you plan to send a patch to fix this?
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-26 17:26 UTC (permalink / raw)
To: Helge Deller, Helge Deller, John Johansen, david laight
Cc: linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <0d03ddb9-d01f-435e-a57c-fbea32844b66@gmx.de>
Hi Helge,
On Wed, 2025-11-26 at 17:58 +0100, Helge Deller wrote:
> On 11/26/25 17:16, John Paul Adrian Glaubitz wrote:
> > Hi Helge,
> >
> > On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
> > > Like this (untested!) patch:
> > >
> > > [PATCH] apparmor: Optimize table creation from possibly unaligned memory
> > >
> > > Source blob may come from userspace and might be unaligned.
> > > Try to optize the copying process by avoiding unaligned memory accesses.
> > >
> > > Signed-off-by: Helge Deller <deller@gmx.de>
> > >
> > > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > > index 1fbe82f5021b..225df6495c84 100644
> > > --- a/security/apparmor/include/match.h
> > > +++ b/security/apparmor/include/match.h
> > > @@ -111,9 +111,14 @@ struct aa_dfa {
> > > typeof(LEN) __i; \
> > > TTYPE *__t = (TTYPE *) TABLE; \
> > > BTYPE *__b = (BTYPE *) BLOB; \
> > > - for (__i = 0; __i < LEN; __i++) { \
> > > - __t[__i] = NTOHX(__b[__i]); \
> > > - } \
> > > + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
> > > + /* copy to naturally aligned table address */ \
> > > + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> > > + /* convert from big-endian if necessary */ \
> > > + if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> > > + for (__i = 0; __i < LEN; __i++, __t++) { \
> > > + *__t = NTOHX(*__t); \
> > > + } \
> > > } while (0)
> > >
> > > static inline size_t table_size(size_t len, size_t el_size)
> >
> > So, is this patch supposed to replace all the other proposed patches?
>
> It just replaces the patch from John.
> But please test the v2 patch I sent instead...
OK, so we need your previous patch that I already tested and your v2 of John's patch?
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH v15 3/4] lsm: count the LSMs enabled at compile time
From: Andy Shevchenko @ 2025-11-26 17:14 UTC (permalink / raw)
To: KP Singh
Cc: linux-security-module, bpf, ast, paul, casey, andrii, keescook,
daniel, renauld, revest, song, linux, Kui-Feng Lee, John Johansen
In-Reply-To: <20240816154307.3031838-4-kpsingh@kernel.org>
On Fri, Aug 16, 2024 at 05:43:06PM +0200, KP Singh wrote:
> These macros are a clever trick to determine a count of the number of
> LSMs that are enabled in the config to ascertain the maximum number of
> static calls that need to be configured per LSM hook.
>
> Without this one would need to generate static calls for the total
> number of LSMs in the kernel (even if they are not compiled) times the
> number of LSM hooks which ends up being quite wasteful.
...
> -/* This counts to 12. Any more, it will return 13th argument. */
> -#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
> -#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
> +/* This counts to 15. Any more, it will return 16th argument. */
> +#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _n, X...) _n
> +#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
You forgot to update _12 in the upper comment.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-26 16:58 UTC (permalink / raw)
To: John Paul Adrian Glaubitz, Helge Deller, John Johansen,
david laight
Cc: linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <78f1ff26f06c0e61d71c1a7f05a7c03c4ec03bef.camel@physik.fu-berlin.de>
On 11/26/25 17:16, John Paul Adrian Glaubitz wrote:
> Hi Helge,
>
> On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
>> Like this (untested!) patch:
>>
>> [PATCH] apparmor: Optimize table creation from possibly unaligned memory
>>
>> Source blob may come from userspace and might be unaligned.
>> Try to optize the copying process by avoiding unaligned memory accesses.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b..225df6495c84 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -111,9 +111,14 @@ struct aa_dfa {
>> typeof(LEN) __i; \
>> TTYPE *__t = (TTYPE *) TABLE; \
>> BTYPE *__b = (BTYPE *) BLOB; \
>> - for (__i = 0; __i < LEN; __i++) { \
>> - __t[__i] = NTOHX(__b[__i]); \
>> - } \
>> + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
>> + /* copy to naturally aligned table address */ \
>> + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
>> + /* convert from big-endian if necessary */ \
>> + if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
>> + for (__i = 0; __i < LEN; __i++, __t++) { \
>> + *__t = NTOHX(*__t); \
>> + } \
>> } while (0)
>>
>> static inline size_t table_size(size_t len, size_t el_size)
>
> So, is this patch supposed to replace all the other proposed patches?
It just replaces the patch from John.
But please test the v2 patch I sent instead...
Helge
^ permalink raw reply
* [PATCH RESEND] apparmor: Replace deprecated strcpy with memcpy in gen_symlink_name
From: Thorsten Blum @ 2025-11-26 16:57 UTC (permalink / raw)
To: John Johansen, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, apparmor, linux-security-module, linux-kernel
strcpy() is deprecated; use memcpy() instead. Unlike strcpy(), memcpy()
does not copy the NUL terminator from the source string, which would be
overwritten anyway on every iteration when using strcpy(). snprintf()
then ensures that 'char *s' is NUL-terminated.
Replace the hard-coded path length to remove the magic number 6, and add
a comment explaining the extra 11 bytes.
Link: https://github.com/KSPP/linux/issues/88
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/apparmorfs.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 391a586d0557..4b2752200ce2 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -1602,16 +1602,20 @@ static char *gen_symlink_name(int depth, const char *dirname, const char *fname)
{
char *buffer, *s;
int error;
- int size = depth * 6 + strlen(dirname) + strlen(fname) + 11;
+ const char *path = "../../";
+ size_t path_len = strlen(path);
+ int size;
+ /* Extra 11 bytes: "raw_data" (9) + two slashes "//" (2) */
+ size = depth * path_len + strlen(dirname) + strlen(fname) + 11;
s = buffer = kmalloc(size, GFP_KERNEL);
if (!buffer)
return ERR_PTR(-ENOMEM);
for (; depth > 0; depth--) {
- strcpy(s, "../../");
- s += 6;
- size -= 6;
+ memcpy(s, path, path_len);
+ s += path_len;
+ size -= path_len;
}
error = snprintf(s, size, "raw_data/%s/%s", dirname, fname);
--
2.51.1
^ permalink raw reply related
* Re: [PATCH v3 5/5] landlock: landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 16:26 UTC (permalink / raw)
To: utilityemal77; +Cc: gnoack, jack, linux-security-module, m, mic, xandfury
In-Reply-To: <20251126122039.3832162-7-utilityemal77@gmail.com>
Hi,
Apologies. Thi is a duplicate of the above patch
v3 5/5 I sent accidentally.
I forgot to clear the old .patch file from my directory after I changed the
commit message, and didn't catch it when doing git send-email.
The contents are identical, I just changed the commit message from
a typo.
"landlock: landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT"
(typo)
to
"landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT"
(correct commit message)
Please ignore this duplicate. I promise to be more careful in the
future.
Regards,
Justin Suess
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Paul Adrian Glaubitz @ 2025-11-26 16:16 UTC (permalink / raw)
To: Helge Deller, John Johansen, david laight
Cc: linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <aSblGNyoSV4LfKji@carbonx1>
Hi Helge,
On Wed, 2025-11-26 at 12:31 +0100, Helge Deller wrote:
> Like this (untested!) patch:
>
> [PATCH] apparmor: Optimize table creation from possibly unaligned memory
>
> Source blob may come from userspace and might be unaligned.
> Try to optize the copying process by avoiding unaligned memory accesses.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b..225df6495c84 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -111,9 +111,14 @@ struct aa_dfa {
> typeof(LEN) __i; \
> TTYPE *__t = (TTYPE *) TABLE; \
> BTYPE *__b = (BTYPE *) BLOB; \
> - for (__i = 0; __i < LEN; __i++) { \
> - __t[__i] = NTOHX(__b[__i]); \
> - } \
> + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
> + /* copy to naturally aligned table address */ \
> + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
> + /* convert from big-endian if necessary */ \
> + if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
> + for (__i = 0; __i < LEN; __i++, __t++) { \
> + *__t = NTOHX(*__t); \
> + } \
> } while (0)
>
> static inline size_t table_size(size_t len, size_t el_size)
So, is this patch supposed to replace all the other proposed patches?
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
^ permalink raw reply
* Re: [PATCH v3 2/4] landlock: Fix handling of disconnected directories
From: Mickaël Salaün @ 2025-11-26 15:32 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Jann Horn, John Johansen, Al Viro,
Ben Scarlato, Christian Brauner, Daniel Burgener, Jeff Xu,
NeilBrown, Paul Moore, Ryan Sullivan, Song Liu, linux-fsdevel,
linux-security-module, Abhinav Saxena
In-Reply-To: <b0f46246-f2c5-42ca-93ce-0d629702a987@maowtm.org>
On Mon, Jul 28, 2025 at 01:13:10AM +0100, Tingmao Wang wrote:
> On 7/23/25 22:01, Mickaël Salaün wrote:
> > On Tue, Jul 22, 2025 at 07:04:02PM +0100, Tingmao Wang wrote:
> >> On 7/19/25 11:42, Mickaël Salaün wrote:
> >>> [...]
> >>> @@ -784,12 +787,18 @@ static bool is_access_to_paths_allowed(
> >>> if (WARN_ON_ONCE(!layer_masks_parent1))
> >>> return false;
> >>>
> >>> - allowed_parent1 = is_layer_masks_allowed(layer_masks_parent1);
> >>> -
> >>> if (unlikely(layer_masks_parent2)) {
> >>> if (WARN_ON_ONCE(!dentry_child1))
> >>> return false;
> >>>
> >>> + /*
> >>> + * Creates a backup of the initial layer masks to be able to restore
> >>> + * them if we find out that we were walking a disconnected directory,
> >>> + * which would make the collected access rights inconsistent (cf.
> >>> + * reset_to_mount_root).
> >>> + */
> >>
> >> This comment is duplicate with the one below, is this intentional?
> >>
> >>> [...]
> >>
> >> On the other hand, I'm still a bit uncertain about the domain check
> >> semantics. While it would not cause a rename to be allowed if it is
> >> otherwise not allowed by any rules on or above the mountpoint, this gets a
> >> bit weird if we have a situation where renames are allowed on the
> >> mountpoint or everywhere, but not read/writes, however read/writes are
> >> allowed directly on a file, but the dir containing that file gets
> >> disconnected so the sandboxed application can't read or write to it.
> >> (Maybe someone would set up such a policy where renames are allowed,
> >> expecting Landlock to always prevent renames where additional permissions
> >> would be exposed?)
> >>
> >> In the above situation, if the file is then moved to a connected
> >> directory, it will become readable/writable again.
> >
> > We can generalize this issue to not only the end file but any component
> > of the path: disconnected directories. In fact, the main issue is the
> > potential inconsistency of access checks over time (e.g. between two
> > renames). This could be exploited to bypass the security checks done
> > for FS_REFER.
>
> Hi Mickaël,
>
> (replying to this one even though I've read the further replies, since I
> don't have anything to address there)
>
> I spent some time thinking about how this could be exploited and I can see
> one concrete situation where disconnected directories can be taken
> advantaged of, which is when an application has rename access across the
> "source" of a bind mount, as well as some place outside of it, but only
> has read/write access on certain subdirs of it, in which case it can use
> this to move files under the bind mount that it does not have access to,
> to a place where it has, by first making the target destination
> disconnected, then connecting it back:
>
> / # mkdir bind_src bind_dst outside
> / # mount --bind bind_src bind_dst
> / # mkdir -p bind_src/d1
> / # echo secret > /bind_src/foo
> / # LL_FS_RO=/usr:/bin:/lib:/etc LL_FS_RW=/bind_src/d1:/outside LL_FS_CREATE_DELETE_REFER=/ /sandboxer bash
> ## (sandboxer patched with ability to add just create/delete/refer
> rule on a path - maybe we can add this to the sandboxer but
> call it something like LL_FS_DIR_RW?)
> Executing the sandboxed command...
> / # cat /bind_src/foo /bind_dst/foo
> cat: /bind_src/foo: Permission denied
> cat: /bind_dst/foo: Permission denied
> / # cd /bind_dst/d1
> /bind_dst/d1 # mv -v /bind_src/d1 /outside
> renamed '/bind_src/d1' -> '/outside/d1'
> /bind_dst/d1 # ls ..
> ls: cannot access '..': No such file or directory
> /bind_dst/d1 # mv -v /bind_dst/foo .
> renamed '/bind_dst/foo' -> './foo'
> /bind_dst/d1 # mv -v /outside/d1 /bind_src/d1
> renamed '/outside/d1' -> '/bind_src/d1'
> /bind_dst/d1 # cat foo
> secret
Good catch! This is definitely an issue.
>
> (I think this is probably not an issue with the existing Landlock code, as
> the application would need to have rules outside the bind mount in order
> for Landlock to not see it when the directory is disconnected, but in that
> case it already has access to the target file anyway due to hierarchy
> inheritance)
>
> The other way I can think of where this inconsistency could be taken
> advantaged of is for regaining access to a file within a disconnected
> directory that the application originally had access to (basically the
> situation I wrote about earlier), but that is arguably a less worrisome
> case and your option 1 would make access in this case allowed anyway. So
> far I can't think of any other way an application could use this to gain
> access it didn't have on the file before.
>
> >
> > I see two solutions:
> >
> > 1. *Always* walk down to the IS_ROOT directory, and then jump to the
> > mount point. This makes it possible to have consistent access checks
> > for renames and open/use. The first downside is that that would
> > change the current behavior for bind mounts that could get more
> > access rights (if the policy explicitly sets rights for the hidden
> > directories). The second downside is that we'll do more walk.
> >
> > 2. Return -EACCES (or -ENOENT) for actions involving disconnected
> > directories, or renames of disconnected opened files. This second
> > solution is simpler and safer but completely disables the use of
> > disconnected directories and the rename of disconnected files for
> > sandboxed processes.
>
> I think -ENOENT might be confusing if the disconnection is accidentally
> triggered (e.g. due to a bug, race etc) (I guess given that ".." doesn't
> work in disconnected dirs, nobody should be setting them up deliberately,
> so maybe any situation where a disconnected directory exists is always a
> bug, or at least a temporary race?), but on the other hand VFS already
> returns -ENOENT for ".." when in disconnected directories so maybe it's
> OK, as long as this is clarified in the doc, I guess.
>
> Also by "rename" I assume we're implicitly including links as well (and
> since one couldn't rename an opened file just from the fd anyway, any
> rename would be for files underneath).
>
> >
> > It would be much better to be able to handle opened directories as
> > (object) capabilities, but that is not currently possible because of the
> > way paths are handled by the VFS and LSM hooks.
> >
> > Tingmao, Günther, Jann, what do you think?
>
> I wonder if there is a third option where we do option 1, but only for
> disconnected dirs, so basically for any disconnected directories Landlock
> would allow rules either on the path from the target to its fs root, or on
> the path from its mountpoint to the real root. In addition, domain check
> for rename/links would be stricter, in that it would make sure there are
> no rules granting more access on the destination than the source even if
> those rules are "hidden" beneath the mountpoint, so that even if the
> destination later become disconnected access is not widened.
>
> While this does leaves inconsistency since now some "hidden" rules would
> be applied only in the disconnected case, maybe this could still be a
> better option since it prevents accidentally widening access checks in the
> common case, especially for existing code (after all, currently Landlock
> does not apply those "hidden" rules, and if an application can't move the
> files into a disconnected directory, for example because write access are
> not granted to any bind mounted places, or because it only has access to
> the mount destination but not the source (achievable by putting the rule
> above the mountpoint, and in that case it can't move a dir outside the
> mount so it will always be connected), it will not be able to surface
> them).
>
> One could argue that a reasonable policy is not supposed to have rules on
> those "hidden" directories in the first place, but in that case whether or
> not we do this extra walk to fs root for the non-disconnected case makes
> no difference.
>
> The advantage of this option is also that we don't increase the
> performance overhead for the non-disconnected case which is the vast
> majority of cases (and this perf benefit might be significant since we're
> likely not backporting any ref-less pathwalk). Also, while I'm not sure
> whether this option or option 1 has any difference in terms of backporting
> difficulty, I think option 1 is a more significant change compared to
> this, and maybe in the future if we do get to have opened directory
> capabilities, keeping the changes introduced by the current "workaround"
> small would be better? This would probably also make it easier to adopt
> Song's path iterator too.
>
> If we do this, then we would probably have a warning in e.g.
> Documentation/userspace-api/landlock.rst about the fact that Landlock will
> do this walk to the fs root if the directory is disconnected (either
> caused by the sandboxed application or by other means).
Thinking more about this, I think this third option is the best
approach. In theory, for a direct access (i.e. not a rename/link), it
may grant more access on disconnected directories because the collected
access rights are the union of N hierarchies, but each of these
hierarchies are defined by the security policy which explicitly allows
such access. This means that in practice these access rights should
already be granted when independently accessing the target file N times
through these N hierarchies. This approach also improves guarantees
about not widening access rights with rename/link, and it only impacts
access to disconnected directories. This will also simplify backports
and reduce merge conflicts.
The main downside is about the inconsistency with access to a connected
directory, but access to disconnected directories is already
inconsistent anyway and the VFS cannot do much about this case.
I'll send a new version today.
>
> Just thoughts and suggestions tho, my opinions aren't very strong, and I
> probably don't have a good grasp of how Landlock is currently used in
> practice.
>
> Best,
> Tingmao
>
> >
> > It looks like AppArmor also denies access to disconnected path in some
> > cases, but it tries to reconstruct the path for known internal
> > filesystems, and it seems to specifically handle the case of chroot. I
> > don't know when PATH_CONNECT_PATH is set though.
> >
> > John, could you please clarify how disconnected directories and files
> > are handled by AppArmor?
> >
> >>
> >> Here is an example test, using the layout1_bind fixture for flexibility
> >> [...]
>
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-26 15:12 UTC (permalink / raw)
To: david laight
Cc: Helge Deller, John Johansen, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <20251126142201.27e23076@pumpkin>
* david laight <david.laight@runbox.com>:
> On Wed, 26 Nov 2025 12:03:03 +0100
> Helge Deller <deller@gmx.de> wrote:
>
> > On 11/26/25 11:44, david laight wrote:
> ...
> > >> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > >> index 26e82ba879d44..3dcc342337aca 100644
> > >> --- a/security/apparmor/match.c
> > >> +++ b/security/apparmor/match.c
> > >> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > >> u8, u8, byte_to_byte);
> > >
> > > Is that that just memcpy() ?
> >
> > No, it's memcpy() only on big-endian machines.
>
> You've misread the quoting...
> The 'data8' case that was only half there is a memcpy().
>
> > On little-endian machines it converts from big-endian
> > 16/32-bit ints to little-endian 16/32-bit ints.
> >
> > But I see some potential for optimization here:
> > a) on big-endian machines just use memcpy()
>
> true
>
> > b) on little-endian machines use memcpy() to copy from possibly-unaligned
> > memory to then known-to-be-aligned destination. Then use a loop with
> > be32_to_cpu() instead of get_unaligned_xx() as it's faster.
>
> There is a function that does a loop byteswap of a buffer - no reason
> to re-invent it.
I assumed there must be something, but I did not see it. Which one?
> But I doubt it is always (if ever) faster to do a copy and then byteswap.
> The loop control and extra memory accesses kill performance.
Yes, you are probably right.
> Not that I've seen a fast get_unaligned() - I don't think gcc or clang
> generate optimal code - For LE I think it is something like:
> low = *(addr & ~3);
> high = *((addr + 3) & ~3);
> shift = (addr & 3) * 8;
> value = low << shift | high >> (32 - shift);
> Note that it is only 2 aligned memory reads - even for 64bit.
Ok, then maybe we should keep it simple like this patch:
[PATCH v2] apparmor: Optimize table creation from possibly unaligned memory
Source blob may come from userspace and might be unaligned.
Try to optize the copying process by avoiding unaligned memory accesses.
Signed-off-by: Helge Deller <deller@gmx.de>
diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
index 1fbe82f5021b..386da2023d50 100644
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -104,16 +104,20 @@ struct aa_dfa {
struct table_header *tables[YYTD_ID_TSIZE];
};
-#define byte_to_byte(X) (X)
+#define byte_to_byte(X) (*(X))
#define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
do { \
typeof(LEN) __i; \
TTYPE *__t = (TTYPE *) TABLE; \
BTYPE *__b = (BTYPE *) BLOB; \
- for (__i = 0; __i < LEN; __i++) { \
- __t[__i] = NTOHX(__b[__i]); \
- } \
+ BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
+ if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN) || sizeof(BTYPE) == 1) \
+ memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
+ else /* copy & convert convert from big-endian */ \
+ for (__i = 0; __i < LEN; __i++) { \
+ __t[__i] = NTOHX(&__b[__i]); \
+ } \
} while (0)
static inline size_t table_size(size_t len, size_t el_size)
diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index c5a91600842a..13e2f6873329 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -15,6 +15,7 @@
#include <linux/vmalloc.h>
#include <linux/err.h>
#include <linux/kref.h>
+#include <linux/unaligned.h>
#include "include/lib.h"
#include "include/match.h"
@@ -70,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
u8, u8, byte_to_byte);
else if (th.td_flags == YYTD_DATA16)
UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
- u16, __be16, be16_to_cpu);
+ u16, __be16, get_unaligned_be16);
else if (th.td_flags == YYTD_DATA32)
UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
- u32, __be32, be32_to_cpu);
+ u32, __be32, get_unaligned_be32);
else
goto fail;
/* if table was vmalloced make sure the page tables are synced
^ permalink raw reply related
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: david laight @ 2025-11-26 14:22 UTC (permalink / raw)
To: Helge Deller
Cc: John Johansen, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <4034ad19-8e09-440c-a042-a66a488c048b@gmx.de>
On Wed, 26 Nov 2025 12:03:03 +0100
Helge Deller <deller@gmx.de> wrote:
> On 11/26/25 11:44, david laight wrote:
...
> >> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> >> index 26e82ba879d44..3dcc342337aca 100644
> >> --- a/security/apparmor/match.c
> >> +++ b/security/apparmor/match.c
> >> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> >> u8, u8, byte_to_byte);
> >
> > Is that that just memcpy() ?
>
> No, it's memcpy() only on big-endian machines.
You've misread the quoting...
The 'data8' case that was only half there is a memcpy().
> On little-endian machines it converts from big-endian
> 16/32-bit ints to little-endian 16/32-bit ints.
>
> But I see some potential for optimization here:
> a) on big-endian machines just use memcpy()
true
> b) on little-endian machines use memcpy() to copy from possibly-unaligned
> memory to then known-to-be-aligned destination. Then use a loop with
> be32_to_cpu() instead of get_unaligned_xx() as it's faster.
There is a function that does a loop byteswap of a buffer - no reason
to re-invent it.
But I doubt it is always (if ever) faster to do a copy and then byteswap.
The loop control and extra memory accesses kill performance.
Not that I've seen a fast get_unaligned() - I don't think gcc or clang
generate optimal code - For LE I think it is something like:
low = *(addr & ~3);
high = *((addr + 3) & ~3);
shift = (addr & 3) * 8;
value = low << shift | high >> (32 - shift);
Note that it is only 2 aligned memory reads - even for 64bit.
David
^ permalink raw reply
* [PATCH v3 5/5] landlock: landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Add a unit test for rule_flag collection, ensuring that access masks
are properly propagated with the flags.
changes v2..v3:
* Removing erroneously misplaced code and placed in the proper
patch.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
security/landlock/ruleset.c | 89 +++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index f7b6a48bbf39..0e0de8b20dc4 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -22,6 +22,7 @@
#include <linux/spinlock.h>
#include <linux/workqueue.h>
#include <uapi/linux/landlock.h>
+#include <kunit/test.h>
#include "access.h"
#include "audit.h"
@@ -774,3 +775,91 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
}
return handled_accesses;
}
+
+#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
+
+/**
+ * test_unmask_layers_no_inherit - Test landlock_unmask_layers() with no_inherit
+ * @rule_flags: Pointer to collected_rule_flags structure to track flags.
+ */
+static void test_unmask_layers_no_inherit(struct kunit *const test)
+{
+ struct landlock_rule *rule;
+ layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS];
+ struct collected_rule_flags rule_flags;
+ const access_mask_t access_request = BIT_ULL(0) | BIT_ULL(1);
+ const layer_mask_t layers_initialized = BIT_ULL(0) | BIT_ULL(1);
+ size_t i;
+
+ rule = kzalloc(struct_size(rule, layers, 2), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, rule);
+
+ rule->num_layers = 2;
+
+ /* Layer 1: allows access 0, no_inherit */
+ rule->layers[0].level = 1;
+ rule->layers[0].access = BIT_ULL(0);
+ rule->layers[0].flags.no_inherit = 1;
+
+ /* Layer 2: allows access 1 */
+ rule->layers[1].level = 2;
+ rule->layers[1].access = BIT_ULL(1);
+
+ /* Case 1: No rule_flags provided (should behave normally) */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), NULL);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* Case 2: rule_flags provided, no existing no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* rule_flags should collect no_inherit from layer 1 */
+ KUNIT_EXPECT_EQ(test, rule_flags.no_inherit_masks, (layer_mask_t)BIT_ULL(0));
+
+ /* Case 3: rule_flags provided, layer 1 is masked by no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+ rule_flags.no_inherit_masks = BIT_ULL(0); /* Mask layer 1 */
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should NOT be unmasked by layer 1 because it is skipped */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized);
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ kfree(rule);
+}
+
+static struct kunit_case ruleset_test_cases[] = {
+ KUNIT_CASE(test_unmask_layers_no_inherit),
+ {}
+};
+
+static struct kunit_suite ruleset_test_suite = {
+ .name = "landlock_ruleset",
+ .test_cases = ruleset_test_cases,
+};
+
+kunit_test_suite(ruleset_test_suite);
+
+#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
--
2.51.0
^ permalink raw reply related
* [PATCH v3 5/5] landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Add a unit test for rule_flag collection, ensuring that access masks
are properly propagated with the flags.
changes v2..v3:
* Removing erroneously misplaced code and placed in the proper
patch.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
build.log
---
security/landlock/ruleset.c | 89 +++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index f7b6a48bbf39..0e0de8b20dc4 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -22,6 +22,7 @@
#include <linux/spinlock.h>
#include <linux/workqueue.h>
#include <uapi/linux/landlock.h>
+#include <kunit/test.h>
#include "access.h"
#include "audit.h"
@@ -774,3 +775,91 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
}
return handled_accesses;
}
+
+#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
+
+/**
+ * test_unmask_layers_no_inherit - Test landlock_unmask_layers() with no_inherit
+ * @rule_flags: Pointer to collected_rule_flags structure to track flags.
+ */
+static void test_unmask_layers_no_inherit(struct kunit *const test)
+{
+ struct landlock_rule *rule;
+ layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS];
+ struct collected_rule_flags rule_flags;
+ const access_mask_t access_request = BIT_ULL(0) | BIT_ULL(1);
+ const layer_mask_t layers_initialized = BIT_ULL(0) | BIT_ULL(1);
+ size_t i;
+
+ rule = kzalloc(struct_size(rule, layers, 2), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, rule);
+
+ rule->num_layers = 2;
+
+ /* Layer 1: allows access 0, no_inherit */
+ rule->layers[0].level = 1;
+ rule->layers[0].access = BIT_ULL(0);
+ rule->layers[0].flags.no_inherit = 1;
+
+ /* Layer 2: allows access 1 */
+ rule->layers[1].level = 2;
+ rule->layers[1].access = BIT_ULL(1);
+
+ /* Case 1: No rule_flags provided (should behave normally) */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), NULL);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* Case 2: rule_flags provided, no existing no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should be unmasked by layer 1 */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized & ~BIT_ULL(0));
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ /* rule_flags should collect no_inherit from layer 1 */
+ KUNIT_EXPECT_EQ(test, rule_flags.no_inherit_masks, (layer_mask_t)BIT_ULL(0));
+
+ /* Case 3: rule_flags provided, layer 1 is masked by no_inherit_masks */
+ for (i = 0; i < ARRAY_SIZE(layer_masks); i++)
+ layer_masks[i] = layers_initialized;
+ memset(&rule_flags, 0, sizeof(rule_flags));
+ rule_flags.no_inherit_masks = BIT_ULL(0); /* Mask layer 1 */
+
+ landlock_unmask_layers(rule, access_request, &layer_masks,
+ ARRAY_SIZE(layer_masks), &rule_flags);
+
+ /* Access 0 should NOT be unmasked by layer 1 because it is skipped */
+ KUNIT_EXPECT_EQ(test, layer_masks[0], layers_initialized);
+ /* Access 1 should be unmasked by layer 2 */
+ KUNIT_EXPECT_EQ(test, layer_masks[1], layers_initialized & ~BIT_ULL(1));
+
+ kfree(rule);
+}
+
+static struct kunit_case ruleset_test_cases[] = {
+ KUNIT_CASE(test_unmask_layers_no_inherit),
+ {}
+};
+
+static struct kunit_suite ruleset_test_suite = {
+ .name = "landlock_ruleset",
+ .test_cases = ruleset_test_cases,
+};
+
+kunit_test_suite(ruleset_test_suite);
+
+#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
--
2.51.0
^ permalink raw reply related
* [PATCH v3 4/5] selftests/landlock: Implement selftests for LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Implements 11 selftests for the flag, covering allowed and disallowed operations on parent
and child directories when this flag is set, as well as multi-layer configurations
and flag inheritance / audit logging.
v2..v3 changes:
* Also covers flag inheritance, audit logging and LANDLOCK_ADD_RULE_QUIET suppression.
* Increases number of selftests from 5 -> 11.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
tools/testing/selftests/landlock/fs_test.c | 459 ++++++++++++++++++++-
1 file changed, 447 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 6aa65d344c72..87b66ad7a0b8 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -717,16 +717,12 @@ TEST_F_FORK(layout1, rule_with_unhandled_access)
}
static void add_path_beneath(struct __test_metadata *const _metadata,
- const int ruleset_fd, const __u64 allowed_access,
- const char *const path, bool quiet)
+ const int ruleset_fd, const __u64 allowed_access,
+ const char *const path, __u32 flags)
{
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = allowed_access,
};
- __u32 flags = 0;
-
- if (quiet)
- flags |= LANDLOCK_ADD_RULE_QUIET;
path_beneath.parent_fd = open(path, O_PATH | O_CLOEXEC);
ASSERT_LE(0, path_beneath.parent_fd)
@@ -790,7 +786,7 @@ static int create_ruleset(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, ruleset_fd, rules[i].access,
- rules[i].path, false);
+ rules[i].path, 0);
}
return ruleset_fd;
}
@@ -1368,7 +1364,7 @@ TEST_F_FORK(layout1, inherit_subset)
* ANDed with the previous ones.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d2, false);
+ dir_s1d2, 0);
/*
* According to ruleset_fd, dir_s1d2 should now have the
* LANDLOCK_ACCESS_FS_READ_FILE and LANDLOCK_ACCESS_FS_WRITE_FILE
@@ -1400,7 +1396,7 @@ TEST_F_FORK(layout1, inherit_subset)
* Try to get more privileges by adding new access rights to the parent
* directory: dir_s1d1.
*/
- add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, false);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, 0);
enforce_ruleset(_metadata, ruleset_fd);
/* Same tests and results as above. */
@@ -1423,7 +1419,7 @@ TEST_F_FORK(layout1, inherit_subset)
* that there was no rule tied to it before.
*/
add_path_beneath(_metadata, ruleset_fd, LANDLOCK_ACCESS_FS_WRITE_FILE,
- dir_s1d3, false);
+ dir_s1d3, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1476,7 +1472,7 @@ TEST_F_FORK(layout1, inherit_superset)
add_path_beneath(_metadata, ruleset_fd,
LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_READ_DIR,
- dir_s1d2, false);
+ dir_s1d2, 0);
enforce_ruleset(_metadata, ruleset_fd);
ASSERT_EQ(0, close(ruleset_fd));
@@ -1488,6 +1484,111 @@ TEST_F_FORK(layout1, inherit_superset)
ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY));
}
+TEST_F_FORK(layout1, inherit_no_inherit_flag)
+{
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW,
+ };
+ int ruleset_fd;
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RW, dir_s1d1, 0);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Parent directory still grants write access to its direct children. */
+ EXPECT_EQ(0, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d1, O_WRONLY));
+
+ /* dir_s1d2 gets only its explicit read-only access rights. */
+ EXPECT_EQ(0, test_open(dir_s1d2, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d2, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
+
+ /* Descendants of dir_s1d2 inherit the reduced access mask. */
+ EXPECT_EQ(0, test_open(dir_s1d3, O_RDONLY | O_DIRECTORY));
+ EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
+ EXPECT_EQ(EACCES, test_open(file1_s1d3, O_WRONLY));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_nested_levels)
+{
+ int ruleset_fd;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW | LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR,
+ };
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Level 1: s1d1 (RW + REFER + REMOVE + NO_INHERIT) */
+ add_path_beneath(_metadata, ruleset_fd,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR,
+ dir_s1d1, LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ /* Level 2: s1d2 (RO + NO_INHERIT) */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ /* Level 3: s1d3 (RW + REFER + REMOVE + NO_INHERIT) */
+ add_path_beneath(_metadata, ruleset_fd,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REFER |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE |
+ LANDLOCK_ACCESS_FS_REMOVE_DIR,
+ dir_s1d3, LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Level 3: s1d3
+ * - RW allowed (unlink file)
+ * - REFER allowed (rename file)
+ * - REMOVE_DIR denied (parent s1d2 is part of direct parent tree)
+ */
+ ASSERT_EQ(0, unlink(file1_s1d3));
+ ASSERT_EQ(0, rename(file2_s1d3, file1_s1d3));
+ ASSERT_EQ(0, rename(file1_s1d3, file2_s1d3));
+ ASSERT_EQ(-1, rmdir(dir_s1d3));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * Level 2: s1d2
+ * - RW denied (unlink file), layer is RO
+ * - REFER denied (rename file)
+ * - REMOVE_DIR of s1d2 not allowed (parent s1d1 is part of direct parent tree)
+ */
+ ASSERT_EQ(-1, unlink(file1_s1d2));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(file2_s1d2, file1_s1d2));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rmdir(dir_s1d2));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * Level 1: s1d1
+ * - RW allowed
+ * - Rename allowed (except for direct parent tree s1d2)
+ * - REMOVE_DIR denied (parent tmp is denied)
+ */
+ ASSERT_EQ(0, unlink(file1_s1d1));
+ ASSERT_EQ(0, rename(file2_s1d1, file1_s1d1));
+ ASSERT_EQ(0, rename(file1_s1d1, file2_s1d1));
+ ASSERT_EQ(-1, rmdir(dir_s1d1));
+ ASSERT_EQ(EACCES, errno);
+}
+
TEST_F_FORK(layout0, max_layers)
{
int i, err;
@@ -4412,6 +4513,246 @@ TEST_F_FORK(layout1, named_unix_domain_socket_ioctl)
ASSERT_EQ(0, close(cli_fd));
}
+TEST_F_FORK(layout1, inherit_no_inherit_topology_dir)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {},
+ };
+ int ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds a no-inherit rule on a leaf directory. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Topology modifications of the rule path and its parents are denied.
+ */
+
+ /* Target directory s1d3 */
+ ASSERT_EQ(-1, rmdir(dir_s1d3));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d3, dir_s2d3));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Parent directory s1d2 */
+ ASSERT_EQ(-1, rmdir(dir_s1d2));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d2));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Grandparent directory s1d1 */
+ ASSERT_EQ(-1, rmdir(dir_s1d1));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d1, dir_s2d1));
+ ASSERT_EQ(EACCES, errno);
+
+ /*
+ * Sibling operations are allowed.
+ */
+ /* Sibling of s1d3 */
+ ASSERT_EQ(0, unlink(file1_s1d2));
+ /* Sibling of s1d2 */
+ ASSERT_EQ(0, unlink(file1_s1d1));
+
+ /*
+ * Content of the no-inherit directory is restricted by the rule (RO).
+ */
+ ASSERT_EQ(-1, unlink(file1_s1d3));
+ ASSERT_EQ(EACCES, errno);
+}
+
+TEST_F_FORK(layout1, no_inherit_allow_inner_removal)
+{
+ int ruleset_fd;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ };
+
+ ruleset_fd =
+ landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ add_path_beneath(_metadata, ruleset_fd,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /*
+ * Content of the no-inherit directory is mutable (RW).
+ * This checks that the no-inherit flag does not seal the content.
+ */
+ ASSERT_EQ(0, unlink(file1_s1d2));
+
+ /*
+ * Topology modifications of the rule path are denied.
+ */
+ ASSERT_EQ(-1, rmdir(dir_s1d2));
+ ASSERT_EQ(EACCES, errno);
+ ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d2));
+ ASSERT_EQ(EACCES, errno);
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_topology_unrelated)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ static const char unrelated_dir[] = TMP_DIR "/s2d1/unrelated";
+ static const char unrelated_file[] = TMP_DIR "/s2d1/unrelated/f1";
+ int ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Adds a no-inherit rule on a leaf directory unrelated to s2. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Ensure we can still create and delete files outside the sealed branch. */
+ ASSERT_EQ(0, mkdir(unrelated_dir, 0700));
+ ASSERT_EQ(0, mknod(unrelated_file, S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(unrelated_file));
+ ASSERT_EQ(0, rmdir(unrelated_dir));
+
+ /* Existing siblings in s2 remain modifiable. */
+ ASSERT_EQ(0, unlink(file1_s2d1));
+ ASSERT_EQ(0, mknod(file1_s2d1, S_IFREG | 0700, 0));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_descendant_rw)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RO,
+ },
+ {},
+ };
+ const __u64 handled_access = ACCESS_RW | LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE;
+ static const char child_file[] =
+ TMP_DIR "/s1d1/s1d2/s1d3/rw_descendant";
+ int ruleset_fd;
+
+ ruleset_fd = create_ruleset(_metadata, handled_access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+ add_path_beneath(_metadata, ruleset_fd,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ dir_s1d3, 0);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ ASSERT_EQ(0, mknod(child_file, S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(child_file));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_topology_file)
+{
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW,
+ },
+ {},
+ };
+ int ruleset_fd;
+ struct landlock_path_beneath_attr path_beneath = {
+ .allowed_access = ACCESS_RO,
+ };
+
+ ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ path_beneath.parent_fd = open(file1_s1d2, O_PATH | O_CLOEXEC);
+ ASSERT_LE(0, path_beneath.parent_fd);
+ ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+ &path_beneath,
+ LANDLOCK_ADD_RULE_NO_INHERIT));
+ ASSERT_EQ(EINVAL, errno);
+ ASSERT_EQ(0, close(path_beneath.parent_fd));
+ ASSERT_EQ(0, close(ruleset_fd));
+}
+
+TEST_F_FORK(layout1, inherit_no_inherit_layered)
+{
+ const struct rule layer1[] = {
+ {
+ .path = TMP_DIR,
+ .access = ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ },
+ {},
+ };
+ int ruleset_fd;
+ static const char unrelated_dir[] = TMP_DIR "/s2d1/unrelated";
+ static const char unrelated_file[] = TMP_DIR "/s2d1/unrelated/f1";
+
+ /* Layer 1: RW on TMP_DIR */
+ ruleset_fd = create_ruleset(_metadata,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ layer1);
+ ASSERT_LE(0, ruleset_fd);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Layer 2: Add no-inherit RO rule on s1d2 */
+ ruleset_fd = create_ruleset(_metadata,
+ ACCESS_RW | LANDLOCK_ACCESS_FS_REMOVE_FILE,
+ layer1);
+ ASSERT_LE(0, ruleset_fd);
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d2,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+ enforce_ruleset(_metadata, ruleset_fd);
+ ASSERT_EQ(0, close(ruleset_fd));
+
+ /* Operations in unrelated areas should still work */
+ ASSERT_EQ(0, mkdir(unrelated_dir, 0700));
+ ASSERT_EQ(0, mknod(unrelated_file, S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(unrelated_file));
+ ASSERT_EQ(0, rmdir(unrelated_dir));
+
+ /* Creating in s1d1 should be allowed (parent still has RW) */
+ ASSERT_EQ(0, mknod(TMP_DIR "/s1d1/newfile", S_IFREG | 0600, 0));
+ ASSERT_EQ(0, unlink(TMP_DIR "/s1d1/newfile"));
+
+ /* Content of s1d2 should be read-only */
+ ASSERT_EQ(-1, unlink(file1_s1d2));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Topology changes to s1d2 should be denied */
+ ASSERT_EQ(-1, rename(dir_s1d2, TMP_DIR "/s2d1/renamed"));
+ ASSERT_EQ(EACCES, errno);
+
+ /* Renaming s1d1 should also be denied (it's an ancestor) */
+ ASSERT_EQ(-1, rename(dir_s1d1, TMP_DIR "/s2d1/renamed"));
+ ASSERT_EQ(EACCES, errno);
+}
+
/* clang-format off */
FIXTURE(ioctl) {};
@@ -7088,6 +7429,100 @@ TEST_F(audit_layout1, write_file)
EXPECT_EQ(1, records.domain);
}
+TEST_F(audit_layout1, no_inherit_parent_is_logged)
+{
+ struct audit_records records;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW,
+ };
+ int ruleset_fd;
+
+ ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+ sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Base read-only rule at s1d1. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d1, 0);
+ /* Descendant s1d1/s1d2/s1d3 forbids inheritance but should still log. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+
+ EXPECT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY));
+ EXPECT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.write_file", file1_s1d2));
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+ EXPECT_EQ(1, records.domain);
+
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
+TEST_F(audit_layout1, no_inherit_blocks_quiet_flag_inheritence)
+{
+ struct audit_records records;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW,
+ .quiet_access_fs = ACCESS_RW,
+ };
+ int ruleset_fd;
+
+ ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+ sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Base read-only rule at tmp/s1d1 with quiet flag. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d1,
+ LANDLOCK_ADD_RULE_QUIET);
+ /* Descendant tmp/s1d1/s1d2/s1d3 forbids inheritance of quiet flag and should still log. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+
+ EXPECT_EQ(EACCES, test_open(file1_s1d3, O_WRONLY));
+ EXPECT_EQ(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.write_file", file1_s1d3));
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+ EXPECT_EQ(1, records.domain);
+
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
+TEST_F(audit_layout1, no_inherit_quiet_parent)
+{
+ struct audit_records records;
+ struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_fs = ACCESS_RW,
+ .quiet_access_fs = ACCESS_RW,
+ };
+ int ruleset_fd;
+
+ ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+ sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Base read-only rule at tmp/s1d1 with quiet flag. */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d1,
+ LANDLOCK_ADD_RULE_QUIET);
+ /* Access to dir_s1d1 shouldn't log */
+ add_path_beneath(_metadata, ruleset_fd, ACCESS_RO, dir_s1d3,
+ LANDLOCK_ADD_RULE_NO_INHERIT);
+
+ enforce_ruleset(_metadata, ruleset_fd);
+
+ EXPECT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY));
+ EXPECT_NE(0, matches_log_fs(_metadata, self->audit_fd,
+ "fs\\.write_file", file1_s1d1));
+ EXPECT_EQ(0, audit_count_records(self->audit_fd, &records));
+ EXPECT_EQ(0, records.access);
+ EXPECT_EQ(0, records.domain);
+
+ EXPECT_EQ(0, close(ruleset_fd));
+}
+
TEST_F(audit_layout1, read_file)
{
struct audit_records records;
@@ -7647,7 +8082,7 @@ static int apply_a_layer(struct __test_metadata *const _metadata,
continue;
add_path_beneath(_metadata, rs_fd, r->access, r->path,
- r->quiet);
+ r->quiet ? LANDLOCK_ADD_RULE_QUIET : 0);
}
ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
--
2.51.0
^ permalink raw reply related
* [PATCH v3 3/5] samples/landlock: Add LANDLOCK_ADD_RULE_NO_INHERIT to landlock-sandboxer
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Adds support to landlock-sandboxer with environment variables LL_FS_RO_NO_INHERIT
and LL_FS_RW_NO_INHERIT. These create the same rulesets as their non-no inherit variants,
plus the LANDLOCK_ADD_RULE_NO_INHERIT flag.
v2..v3 changes:
* Minor formatting fixes
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
samples/landlock/sandboxer.c | 37 +++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index 2d8e3e94b77b..6f6bfc4e5110 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -58,6 +58,8 @@ static inline int landlock_restrict_self(const int ruleset_fd,
#define ENV_FS_RO_NAME "LL_FS_RO"
#define ENV_FS_RW_NAME "LL_FS_RW"
+#define ENV_FS_RO_NO_INHERIT_NAME "LL_FS_RO_NO_INHERIT"
+#define ENV_FS_RW_NO_INHERIT_NAME "LL_FS_RW_NO_INHERIT"
#define ENV_FS_QUIET_NAME "LL_FS_QUIET"
#define ENV_FS_QUIET_ACCESS_NAME "LL_FS_QUIET_ACCESS"
#define ENV_TCP_BIND_NAME "LL_TCP_BIND"
@@ -121,7 +123,8 @@ static int parse_path(char *env_path, const char ***const path_list)
/* clang-format on */
static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
- const __u64 allowed_access, bool quiet)
+ const __u64 allowed_access,
+ __u32 add_rule_flags, bool mandatory)
{
int num_paths, i, ret = 1;
char *env_path_name;
@@ -132,9 +135,13 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
env_path_name = getenv(env_var);
if (!env_path_name) {
- /* Prevents users to forget a setting. */
- fprintf(stderr, "Missing environment variable %s\n", env_var);
- return 1;
+ if (mandatory) {
+ /* Prevents from forgetting to set necessary env vars. */
+ fprintf(stderr, "Missing environment variable %s\n",
+ env_var);
+ return 1;
+ }
+ return 0;
}
env_path_name = strdup(env_path_name);
unsetenv(env_var);
@@ -171,8 +178,7 @@ static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
if (!S_ISDIR(statbuf.st_mode))
path_beneath.allowed_access &= ACCESS_FILE;
if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
- &path_beneath,
- quiet ? LANDLOCK_ADD_RULE_QUIET : 0)) {
+ &path_beneath, add_rule_flags)) {
fprintf(stderr,
"Failed to update the ruleset with \"%s\": %s\n",
path_list[i], strerror(errno));
@@ -375,6 +381,8 @@ static const char help[] =
"Optional settings (when not set, their associated access check "
"is always allowed, which is different from an empty string which "
"means an empty list):\n"
+ "* " ENV_FS_RO_NO_INHERIT_NAME ": read-only paths without rule inheritance\n"
+ "* " ENV_FS_RW_NO_INHERIT_NAME ": read-write paths without rule inheritance\n"
"* " ENV_TCP_BIND_NAME ": ports allowed to bind (server)\n"
"* " ENV_TCP_CONNECT_NAME ": ports allowed to connect (client)\n"
"* " ENV_SCOPED_NAME ": actions denied on the outside of the landlock domain\n"
@@ -596,17 +604,28 @@ int main(const int argc, char *const argv[], char *const *const envp)
}
if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro,
- false)) {
+ 0, true)) {
goto err_close_ruleset;
}
if (populate_ruleset_fs(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw,
+ 0, true)) {
+ goto err_close_ruleset;
+ }
+ /* Optional no-inherit rules mirror the regular read-only/read-write sets. */
+ if (populate_ruleset_fs(ENV_FS_RO_NO_INHERIT_NAME, ruleset_fd,
+ access_fs_ro, LANDLOCK_ADD_RULE_NO_INHERIT,
+ false)) {
+ goto err_close_ruleset;
+ }
+ if (populate_ruleset_fs(ENV_FS_RW_NO_INHERIT_NAME, ruleset_fd,
+ access_fs_rw, LANDLOCK_ADD_RULE_NO_INHERIT,
false)) {
goto err_close_ruleset;
}
/* Don't require this env to be present. */
- if (quiet_supported && getenv(ENV_FS_QUIET_NAME)) {
+ if (quiet_supported) {
if (populate_ruleset_fs(ENV_FS_QUIET_NAME, ruleset_fd, 0,
- true)) {
+ LANDLOCK_ADD_RULE_QUIET, false)) {
goto err_close_ruleset;
}
}
--
2.51.0
^ permalink raw reply related
* [PATCH v3 2/5] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Implements the syscall side flag handling and kernel api headers for the
LANDLOCK_ADD_RULE_NO_INHERIT flag.
v2..v3 changes:
* Extended documentation for flag inheritance suppression on
LANDLOCK_ADD_RULE_NO_INHERIT.
* Extended the flag validation rules in the syscall.
* Added mention of no inherit in empty rules in add_rule_path_beneath
as per Tingmao Wang's suggestion.
* Added check for useless no-inherit flag in networking rules.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
include/uapi/linux/landlock.h | 29 +++++++++++++++++++++++++++++
security/landlock/syscalls.c | 14 +++++++++++---
2 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index d4f47d20361a..cf5c8068f513 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -127,10 +127,39 @@ struct landlock_ruleset_attr {
* allowed_access in the passed in rule_attr. When this flag is
* present, the caller is also allowed to pass in an empty
* allowed_access.
+ * %LANDLOCK_ADD_RULE_NO_INHERIT
+ * When set on a rule being added to a ruleset, this flag disables the
+ * inheritance of access rights and flags from parent objects.
+ *
+ * This flag currently applies only to filesystem rules. Adding it to
+ * non-filesystem rules wil return -EINVAL, unless future extensions
+ * of Landlock define other hierarchical object types.
+ *
+ * By default, Landlock filesystem rules inherit allowed accesses from
+ * ancestor directories: if a parent directory grants certain rights,
+ * those rights also apply to its children. A rule marked with
+ * LANDLOCK_ADD_RULE_NO_INHERIT stops this propagation at the directory
+ * covered by the rule. Descendants of that directory continue to inherit
+ * normally unless they also have rules using this flag.
+ *
+ * If a regular file is marked with this flag, it will not inherit any
+ * access rights from its parent directories; only the accesses explicitly
+ * allowed by the rule will apply to that file.
+ *
+ * This flag also enforces parent-directory restrictions: rename, rmdir,
+ * link, and other operations that would change the directory's immediate
+ * parent subtree are denied up to the mount point. This prevents
+ * sandboxed processes from manipulating the filesystem hierarchy to evade
+ * restrictions (e.g., via sandbox-restart attacks).
+ *
+ * In addition, this flag blocks the inheritance of rule-layer flags
+ * (such as the quiet flag) from parent directories to the object covered
+ * by this rule.
*/
/* clang-format off */
#define LANDLOCK_ADD_RULE_QUIET (1U << 0)
+#define LANDLOCK_ADD_RULE_NO_INHERIT (1U << 1)
/* clang-format on */
/**
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 93396bfc1500..1ea9bf95ef61 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -352,7 +352,7 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
/*
* Informs about useless rule: empty allowed_access (i.e. deny rules)
* are ignored in path walks. However, the rule is not useless if it
- * is there to hold a quiet flag
+ * is there to hold a quiet or no inherit flag.
*/
if (!flags && !path_beneath_attr.allowed_access)
return -ENOMSG;
@@ -407,6 +407,10 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
if (flags & LANDLOCK_ADD_RULE_QUIET && !ruleset->quiet_masks.net)
return -EINVAL;
+ /* No inherit is always useless for this scope */
+ if (flags & LANDLOCK_ADD_RULE_NO_INHERIT)
+ return -EINVAL;
+
/* Denies inserting a rule with port greater than 65535. */
if (net_port_attr.port > U16_MAX)
return -EINVAL;
@@ -462,8 +466,12 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
if (!is_initialized())
return -EOPNOTSUPP;
-
- if (flags && flags != LANDLOCK_ADD_RULE_QUIET)
+ /* Checks flag existence */
+ if (flags && flags & ~(LANDLOCK_ADD_RULE_QUIET | LANDLOCK_ADD_RULE_NO_INHERIT))
+ return -EINVAL;
+ /* No inherit may only apply on path_beneath rules. */
+ if ((flags & LANDLOCK_ADD_RULE_NO_INHERIT) &&
+ rule_type != LANDLOCK_RULE_PATH_BENEATH)
return -EINVAL;
/* Gets and checks the ruleset. */
--
2.51.0
^ permalink raw reply related
* [PATCH v3 1/5] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
In-Reply-To: <20251126122039.3832162-1-utilityemal77@gmail.com>
Implements a flag to prevent access grant inheritance within the filesystem hierarchy
for landlock rules.
If a landlock rule on an inode has this flag, any access grants on parent inodes will be
ignored. Moreover, operations that involve altering the direct parent tree of the subject with
LANDLOCK_ADD_RULE_NO_INHERIT will be denied up to the mountpoint.
Additionally (new in v3) parent flag inheritance is blocked by this flag, allowing fine
grained access control over LANDLOCK_ADD_RULE_QUIET.
For example, if /a/b/c/ = read only + LANDLOCK_ADD_RULE_NO_INHERIT and / = read write, writes to
files in /a/b/c will be denied. Moreover, moving /a to /bad, removing /a/b/c, or creating links to
/a will be prohibited.
And if / has LANDLOCK_ADD_RULE_QUIET, /a/b/c will still audit (handled)
accesses. This is because LANDLOCK_ADD_RULE_NO_INHERIT also
suppresses flag inheritance from parent objects.
The parent directory restrictions mitigate sandbox-restart attacks. For example, if a sandboxed program
is able to move a LANDLOCK_ADD_RULE_NO_INHERIT restricted directory, upon sandbox restart, the policy
applied naively on the same filenames would be invalid. Preventing these operations mitigates these attacks.
v2..v3 changes:
* Parent directory topology protections now work by lazily
inserting blank rules on parent inodes if they do not
exist. This replaces the previous xarray implementation
with simplified logic.
* Added an optimization to skip further processing if all layers collected
no inherit.
* Added support to block flag inheritance.
Signed-off-by: Justin Suess <utilityemal77@gmail.com>
---
security/landlock/audit.c | 4 +-
security/landlock/domain.c | 4 +-
security/landlock/fs.c | 592 +++++++++++++++++++++++++++++++++++-
security/landlock/ruleset.c | 27 +-
security/landlock/ruleset.h | 36 ++-
5 files changed, 645 insertions(+), 18 deletions(-)
diff --git a/security/landlock/audit.c b/security/landlock/audit.c
index d51563712325..4da97dd6985c 100644
--- a/security/landlock/audit.c
+++ b/security/landlock/audit.c
@@ -588,7 +588,9 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
subject->domain, &missing, request->layer_masks,
request->layer_masks_size);
object_quiet_flag = !!(request->rule_flags.quiet_masks &
- BIT(youngest_layer));
+ BIT(youngest_layer)) &&
+ !(request->rule_flags.blocked_flag_masks &
+ BIT(youngest_layer));
} else {
youngest_layer = get_layer_from_deny_masks(
&missing, request->all_existing_optional_access,
diff --git a/security/landlock/domain.c b/security/landlock/domain.c
index 8caf07250328..5bd83865c87d 100644
--- a/security/landlock/domain.c
+++ b/security/landlock/domain.c
@@ -236,7 +236,9 @@ optional_access_t landlock_get_quiet_optional_accesses(
BITS_PER_TYPE(access_mask_t)) {
const u8 layer = (deny_masks >> (access_index * 4)) &
(LANDLOCK_MAX_NUM_LAYERS - 1);
- const bool is_quiet = !!(rule_flags.quiet_masks & BIT(layer));
+ const layer_mask_t layer_bit = BIT(layer);
+ const bool is_quiet = !!(rule_flags.quiet_masks & layer_bit) &&
+ !(rule_flags.blocked_flag_masks & layer_bit);
if (is_quiet)
quiet_optional_accesses |= BIT(access_index);
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 29f10da32141..0a5c73f18f26 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -317,6 +317,206 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
LANDLOCK_ACCESS_FS_IOCTL_DEV)
/* clang-format on */
+static const struct landlock_rule *find_rule(const struct landlock_ruleset *const domain,
+ const struct dentry *const dentry);
+
+/**
+ * landlock_domain_layers_mask - Build a mask covering all layers of a domain
+ * @domain: The ruleset (domain) to inspect.
+ *
+ * Return a layer mask with a 1 bit for each existing layer of @domain.
+ * If @domain has no layers 0 is returned. If the number of layers is
+ * greater than or equal to the number of bits in layer_mask_t, all bits
+ * are set.
+ */
+static layer_mask_t landlock_domain_layers_mask(const struct landlock_ruleset
+ *const domain)
+{
+ if (!domain || !domain->num_layers)
+ return 0;
+
+ if (domain->num_layers >= sizeof(layer_mask_t) * BITS_PER_BYTE)
+ return (layer_mask_t)~0ULL;
+
+ return GENMASK_ULL(domain->num_layers - 1, 0);
+}
+
+/**
+ * rule_blocks_all_layers_no_inherit - check whether a rule disables inheritance
+ * @domain_layers_mask: Mask describing the domain's active layers.
+ * @rule: Rule to inspect.
+ *
+ * Return true if every layer present in @rule has its no_inherit flag set
+ * and the set of layers covered by the rule equals @domain_layers_mask.
+ * This indicates that the rule prevents inheritance on all layers of the
+ * domain and thus further walking for inheritance checks can stop.
+ */
+static bool rule_blocks_all_layers_no_inherit(const layer_mask_t domain_layers_mask,
+ const struct landlock_rule *const rule)
+{
+ layer_mask_t rule_layers = 0;
+ u32 layer_index;
+
+ if (!domain_layers_mask || !rule)
+ return false;
+
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *const layer =
+ &rule->layers[layer_index];
+ const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
+
+ if (!layer->flags.no_inherit)
+ return false;
+
+ rule_layers |= layer_bit;
+ }
+
+ return rule_layers && rule_layers == domain_layers_mask;
+}
+
+/**
+ * landlock_collect_no_inherit_layers - Collects layers with no_inherit flags
+ */
+/**
+ * landlock_collect_no_inherit_layers - collect effective no_inherit layers
+ * @ruleset: Ruleset to consult.
+ * @dentry: Dentry used as a starting point for the upward walk.
+ *
+ * Walk upwards from @dentry and return a layer mask containing the layers
+ * for which either a rule on the visited dentry has the no_inherit flag set
+ * or where an ancestor was previously marked as having a descendant with
+ * a no_inherit rule. The search prefers the closest matching dentry and
+ * stops once any relevant layer bits are found or the root is reached.
+ *
+ * Returns a layer_mask_t where each set bit corresponds to a layer with an
+ * effective no_inherit influence for @dentry. Returns 0 if none apply or if
+ * inputs are invalid.
+ */
+static layer_mask_t landlock_collect_no_inherit_layers(const struct landlock_ruleset
+ *const ruleset,
+ struct dentry *const dentry)
+{
+ struct dentry *cursor, *parent;
+ layer_mask_t layers = 0;
+ bool include_descendants = true;
+
+ if (!ruleset || !dentry || d_is_negative(dentry))
+ return 0;
+
+ cursor = dget(dentry);
+ while (true) {
+ const struct landlock_rule *rule;
+ u32 layer_index;
+
+ rule = find_rule(ruleset, cursor);
+ if (rule) {
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *layer = &rule->layers[layer_index];
+
+ if (layer->flags.no_inherit ||
+ (include_descendants &&
+ layer->flags.has_no_inherit_descendant))
+ layers |= BIT_ULL((layer->level ?
+ layer->level : layer_index + 1) - 1);
+ }
+ }
+
+ if (layers) {
+ dput(cursor);
+ return layers;
+ }
+
+ if (IS_ROOT(cursor)) {
+ dput(cursor);
+ break;
+ }
+
+ parent = dget_parent(cursor);
+ dput(cursor);
+ if (!parent)
+ break;
+
+ cursor = parent;
+ include_descendants = false;
+ }
+ return 0;
+}
+
+static int mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
+ struct dentry *dentry,
+ layer_mask_t descendant_layers);
+
+/**
+ * mask_no_inherit_descendant_layers - apply descendant no_inherit masking
+ * @domain: The ruleset (domain) to consult.
+ * @dentry: The dentry whose descendants are considered.
+ * @child_layers: Layers present on the child that may be subject to masking.
+ * @access_request: Accesses being requested (bitmask).
+ * @layer_masks: Per-access layer masks to be modified in-place.
+ * @rule_flags: Collected flags which will be updated accordingly.
+ *
+ * If descendant dentries have no_inherit, clear that
+ * layer's bit from @layer_masks. Also updates @rule_flags to reflect
+ * which layers were blocked. Returns true if any of the @layer_masks were
+ * modified, false otherwise.
+ */
+static bool mask_no_inherit_descendant_layers(const struct landlock_ruleset
+ *const domain,
+ struct dentry *const dentry,
+ layer_mask_t child_layers,
+ const access_mask_t access_request,
+ layer_mask_t
+ (*const layer_masks)
+ [LANDLOCK_NUM_ACCESS_FS],
+ struct collected_rule_flags
+ *const rule_flags)
+{
+ layer_mask_t descendant_layers;
+ const unsigned long access_req = access_request;
+ unsigned long access_bit;
+ bool changed = false;
+
+ if (!access_request || !layer_masks || !rule_flags || !dentry)
+ return false;
+ if (d_is_negative(dentry))
+ return false;
+
+ descendant_layers = landlock_collect_no_inherit_layers(domain, dentry);
+ {
+ layer_mask_t shared_layers = descendant_layers & child_layers;
+
+ if (shared_layers) {
+ rule_flags->no_inherit_masks |= shared_layers;
+ rule_flags->no_inherit_desc_masks |= shared_layers;
+ rule_flags->blocked_flag_masks |= shared_layers;
+ }
+ }
+ descendant_layers &= ~child_layers;
+ descendant_layers &= ~rule_flags->no_inherit_masks;
+ if (!descendant_layers)
+ return false;
+
+ rule_flags->blocked_flag_masks |= descendant_layers;
+
+ for_each_set_bit(access_bit, &access_req,
+ ARRAY_SIZE(*layer_masks)) {
+ layer_mask_t *const layer_mask = &(*layer_masks)[access_bit];
+
+ if (*layer_mask & descendant_layers) {
+ *layer_mask &= ~descendant_layers;
+ changed = true;
+ }
+ }
+
+ if (!changed)
+ return false;
+
+ rule_flags->no_inherit_masks |= descendant_layers;
+ rule_flags->no_inherit_desc_masks |= descendant_layers;
+
+ return true;
+}
+
/*
* @path: Should have been checked by get_path_from_fd().
*/
@@ -325,12 +525,13 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
access_mask_t access_rights, const int flags)
{
int err;
+ const bool is_dir = d_is_dir(path->dentry);
struct landlock_id id = {
.type = LANDLOCK_KEY_INODE,
};
/* Files only get access rights that make sense. */
- if (!d_is_dir(path->dentry) &&
+ if (!is_dir &&
(access_rights | ACCESS_FILE) != ACCESS_FILE)
return -EINVAL;
if (WARN_ON_ONCE(ruleset->num_layers != 1))
@@ -344,13 +545,43 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
return PTR_ERR(id.key.object);
mutex_lock(&ruleset->lock);
err = landlock_insert_rule(ruleset, id, access_rights, flags);
+ if (!err && (flags & LANDLOCK_ADD_RULE_NO_INHERIT)) {
+ const struct landlock_rule *rule;
+ layer_mask_t descendant_layers = 0;
+ u32 layer_index;
+
+ rule = find_rule(ruleset, path->dentry);
+ if (rule) {
+ for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
+ const struct landlock_layer *layer =
+ &rule->layers[layer_index];
+
+ if (layer->flags.no_inherit ||
+ layer->flags.has_no_inherit_descendant)
+ descendant_layers |=
+ BIT_ULL((layer->level ?
+ layer->level : layer_index + 1) - 1);
+ }
+ if (descendant_layers) {
+ err = mark_no_inherit_ancestors(ruleset, path->dentry,
+ descendant_layers);
+ if (err)
+ goto out_unlock;
+ }
+ }
+ }
mutex_unlock(&ruleset->lock);
+out:
/*
* No need to check for an error because landlock_insert_rule()
* increments the refcount for the new object if needed.
*/
landlock_put_object(id.key.object);
return err;
+
+out_unlock:
+ mutex_unlock(&ruleset->lock);
+ goto out;
}
/* Access-control management */
@@ -382,6 +613,134 @@ find_rule(const struct landlock_ruleset *const domain,
return rule;
}
+/**
+ * ensure_rule_for_dentry - ensure a ruleset contains a rule entry for dentry,
+ * inserting a blank rule if needed.
+ * @ruleset: Ruleset to modify/inspect. Caller must hold @ruleset->lock.
+ * @dentry: Dentry to ensure a rule exists for.
+ *
+ * If no rule is currently associated with @dentry, insert an empty rule
+ * (with zero access) tied to the backing inode. Returns a pointer to the
+ * rule associated with @dentry on success, NULL when @dentry is negative, or
+ * an ERR_PTR()-encoded error if the rule cannot be created.
+ *
+ * This is useful for LANDLOCK_ADD_RULE_NO_INHERIT processing, where a rule
+ * may need to be created for an ancestor dentry that does not yet have one
+ * to properly track no_inherit flags.
+ *
+ * The flags are set to zero if a rule is newly created, and the caller
+ * is responsible for setting them appropriately.
+ *
+ * The returned rule pointer's lifetime is tied to @ruleset.
+ */
+static const struct landlock_rule *
+ensure_rule_for_dentry(struct landlock_ruleset *const ruleset,
+ struct dentry *const dentry)
+{
+ struct landlock_id id = {
+ .type = LANDLOCK_KEY_INODE,
+ };
+ const struct landlock_rule *rule;
+ int err;
+
+ if (!ruleset || !dentry || d_is_negative(dentry))
+ return NULL;
+
+ lockdep_assert_held(&ruleset->lock);
+
+ rule = find_rule(ruleset, dentry);
+ if (rule)
+ return rule;
+
+ id.key.object = get_inode_object(d_backing_inode(dentry));
+ if (IS_ERR(id.key.object))
+ return ERR_CAST(id.key.object);
+
+ err = landlock_insert_rule(ruleset, id, 0, 0);
+ landlock_put_object(id.key.object);
+ if (err)
+ return ERR_PTR(err);
+
+ rule = find_rule(ruleset, dentry);
+ return rule ? rule : ERR_PTR(-ENOENT);
+}
+
+/**
+ * mark_no_inherit_ancestors - mark ancestors as having no_inherit descendants
+ * @ruleset: Ruleset to modify. Caller must hold @ruleset->lock.
+ * @dentry: Dentry representing the descendant that carries no_inherit bits.
+ * @descendant_layers: Mask of layers from the descendant that should be
+ * advertised to ancestors via has_no_inherit_descendant.
+ *
+ * Walks upward from @dentry and ensures that any ancestor rule contains the
+ * has_no_inherit_descendant marker for the specified @descendant_layers so
+ * parent lookups can quickly detect descendant no_inherit influence.
+ *
+ * Returns 0 on success or a negative errno if ancestor bookkeeping fails.
+ */
+static int mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
+ struct dentry *dentry,
+ layer_mask_t descendant_layers)
+{
+ struct dentry *cursor;
+ u32 layer_index;
+ int err = 0;
+
+ if (!ruleset || !dentry || !descendant_layers)
+ return -EINVAL;
+
+ lockdep_assert_held(&ruleset->lock);
+
+ cursor = dget(dentry);
+ while (cursor) {
+ struct dentry *parent;
+
+ if (IS_ROOT(cursor)) {
+ dput(cursor);
+ break;
+ }
+
+ parent = dget_parent(cursor);
+ dput(cursor);
+ if (!parent)
+ break;
+
+ if (!d_is_negative(parent)) {
+ const struct landlock_rule *rule;
+ /* Ensures a rule exists for the parent dentry,
+ * inserting a blank one if needed
+ */
+ rule = ensure_rule_for_dentry(ruleset, parent);
+ if (IS_ERR(rule)) {
+ err = PTR_ERR(rule);
+ dput(parent);
+ cursor = NULL;
+ break;
+ }
+ if (rule) {
+ struct landlock_rule *mutable_rule =
+ (struct landlock_rule *)rule;
+
+ for (layer_index = 0;
+ layer_index < mutable_rule->num_layers;
+ layer_index++) {
+ struct landlock_layer *layer =
+ &mutable_rule->layers[layer_index];
+ layer_mask_t layer_bit =
+ BIT_ULL((layer->level ?
+ layer->level : layer_index + 1) - 1);
+
+ if (descendant_layers & layer_bit)
+ layer->flags.has_no_inherit_descendant = true;
+ }
+ }
+ }
+
+ cursor = parent;
+ }
+ return err;
+}
+
/*
* Allows access to pseudo filesystems that will never be mountable (e.g.
* sockfs, pipefs), but can still be reachable through
@@ -764,6 +1123,8 @@ static bool is_access_to_paths_allowed(
struct landlock_request *const log_request_parent2,
struct dentry *const dentry_child2)
{
+ const layer_mask_t domain_layers_mask =
+ landlock_domain_layers_mask(domain);
bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check,
is_dom_check_bkp, child1_is_directory = true,
child2_is_directory = true;
@@ -778,6 +1139,13 @@ static bool is_access_to_paths_allowed(
struct collected_rule_flags *rule_flags_parent1 = &log_request_parent1->rule_flags;
struct collected_rule_flags *rule_flags_parent2 = &log_request_parent2->rule_flags;
struct collected_rule_flags _rule_flag_parent1_bkp, _rule_flag_parent2_bkp;
+ layer_mask_t child1_layers = 0;
+ layer_mask_t child2_layers = 0;
+
+ if (dentry_child1)
+ child1_layers = landlock_collect_no_inherit_layers(domain, dentry_child1);
+ if (dentry_child2)
+ child2_layers = landlock_collect_no_inherit_layers(domain, dentry_child2);
if (!access_request_parent1 && !access_request_parent2)
return true;
@@ -931,6 +1299,10 @@ static bool is_access_to_paths_allowed(
ARRAY_SIZE(*layer_masks_parent2),
rule_flags_parent2);
+ if (rule &&
+ rule_blocks_all_layers_no_inherit(domain_layers_mask, rule))
+ break;
+
/* Stops when a rule from each layer grants access. */
if (allowed_parent1 && allowed_parent2) {
/*
@@ -976,8 +1348,13 @@ static bool is_access_to_paths_allowed(
memcpy(&_rule_flag_parent2_bkp,
rule_flags_parent2,
sizeof(_rule_flag_parent2_bkp));
- is_dom_check_bkp = is_dom_check;
}
+ is_dom_check_bkp = is_dom_check;
+ child1_layers = landlock_collect_no_inherit_layers(domain,
+ walker_path
+ .dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
/* Ignores hidden mount points. */
goto jump_up;
@@ -1001,15 +1378,50 @@ static bool is_access_to_paths_allowed(
break;
}
- /*
- * We reached a disconnected root directory from a bind mount, and
- * we need to reset the walk to the current mount root.
- */
- goto reset_to_mount_root;
- }
- parent_dentry = dget_parent(walker_path.dentry);
- dput(walker_path.dentry);
- walker_path.dentry = parent_dentry;
+ /*
+ * We reached a disconnected root directory from a bind mount, and
+ * we need to reset the walk to the current mount root.
+ */
+ goto reset_to_mount_root;
+ }
+ if (likely(!d_is_negative(walker_path.dentry))) {
+ child1_layers = landlock_collect_no_inherit_layers(domain,
+ walker_path.dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
+ } else {
+ child1_layers = 0;
+ if (layer_masks_parent2)
+ child2_layers = 0;
+ }
+ parent_dentry = dget_parent(walker_path.dentry);
+ dput(walker_path.dentry);
+ walker_path.dentry = parent_dentry;
+ /*
+ * Apply descendant no-inherit masking now that we've moved to the
+ * parent. This ensures the parent respects any no-inherit rules from
+ * the child we just left. Only applies to refer operations (rename/link).
+ */
+ if (unlikely(layer_masks_parent2)) {
+ if (mask_no_inherit_descendant_layers(domain, walker_path.dentry,
+ child1_layers,
+ access_masked_parent1,
+ layer_masks_parent1,
+ rule_flags_parent1))
+ allowed_parent1 =
+ allowed_parent1 ||
+ is_layer_masks_allowed(layer_masks_parent1);
+
+ if (rule_flags_parent2 &&
+ mask_no_inherit_descendant_layers(domain, walker_path.dentry,
+ child2_layers,
+ access_masked_parent2,
+ layer_masks_parent2,
+ rule_flags_parent2))
+ allowed_parent2 =
+ allowed_parent2 ||
+ is_layer_masks_allowed(layer_masks_parent2);
+ }
continue;
reset_to_mount_root:
@@ -1057,6 +1469,10 @@ static bool is_access_to_paths_allowed(
dput(walker_path.dentry);
walker_path.dentry = walker_path.mnt->mnt_root;
dget(walker_path.dentry);
+ child1_layers = landlock_collect_no_inherit_layers(domain,
+ walker_path.dentry);
+ if (layer_masks_parent2)
+ child2_layers = child1_layers;
}
path_put(&walker_path);
@@ -1172,6 +1588,8 @@ static bool collect_domain_accesses(
struct collected_rule_flags *const rule_flags)
{
access_mask_t access_dom;
+ const layer_mask_t domain_layers_mask =
+ landlock_domain_layers_mask(domain);
bool ret = false;
if (WARN_ON_ONCE(!domain || !mnt_dir || !dir || !layer_masks_dom))
@@ -1187,9 +1605,11 @@ static bool collect_domain_accesses(
while (true) {
struct dentry *parent_dentry;
+ const struct landlock_rule *rule = find_rule(domain, dir);
+
/* Gets all layers allowing all domain accesses. */
if (landlock_unmask_layers(
- find_rule(domain, dir), access_dom, layer_masks_dom,
+ rule, access_dom, layer_masks_dom,
ARRAY_SIZE(*layer_masks_dom), rule_flags)) {
/*
* Before allowing this side of the access request, checks that the
@@ -1206,6 +1626,10 @@ static bool collect_domain_accesses(
break;
}
+ if (rule &&
+ rule_blocks_all_layers_no_inherit(domain_layers_mask, rule))
+ break;
+
/* Stops at the mount point. */
if (dir == mnt_dir->dentry)
break;
@@ -1232,6 +1656,121 @@ static bool collect_domain_accesses(
return ret;
}
+/**
+ * collect_topology_sealed_layers - collect layers sealed against topology changes
+ * @domain: Ruleset to consult.
+ * @dentry: Starting dentry for the upward walk.
+ * @override_layers: Optional out parameter filled with layers that are
+ * present on ancestors but considered overrides (not
+ * sealing the topology for descendants).
+ *
+ * Walk upwards from @dentry and return a mask of layers where either the
+ * visited dentry contains a no_inherit rule or ancestors were previously
+ * marked as having a descendant with no_inherit. @override_layers, if not
+ * NULL, is filled with layers that would normally be overridden by more
+ * specific descendant rules.
+ *
+ * Returns a layer mask where set bits indicate layers that are "sealed"
+ * (topology changes like rename/rmdir are denied) for the subtree rooted at
+ * @dentry.
+ *
+ * Useful for LANDLOCK_ADD_RULE_NO_INHERIT parent directory enforcement to ensure
+ * that topology changes do not violate the no_inherit constraints.
+ */
+static layer_mask_t
+collect_topology_sealed_layers(const struct landlock_ruleset *const domain,
+ struct dentry *dentry,
+ layer_mask_t *const override_layers)
+{
+ struct dentry *cursor, *parent;
+ bool include_descendants = true;
+ layer_mask_t sealed_layers = 0;
+
+ if (override_layers)
+ *override_layers = 0;
+
+ if (!domain || !dentry || d_is_negative(dentry))
+ return 0;
+
+ cursor = dget(dentry);
+ while (cursor) {
+ const struct landlock_rule *rule;
+ u32 layer_index;
+
+ rule = find_rule(domain, cursor);
+ if (rule) {
+ for (layer_index = 0; layer_index < rule->num_layers;
+ layer_index++) {
+ const struct landlock_layer *layer =
+ &rule->layers[layer_index];
+ const int level = layer->level ? layer->level :
+ layer_index + 1;
+ layer_mask_t layer_bit = BIT_ULL(level - 1);
+
+ if (include_descendants &&
+ (layer->flags.no_inherit ||
+ layer->flags.has_no_inherit_descendant)) {
+ sealed_layers |= layer_bit;
+ } else if (override_layers) {
+ *override_layers |= layer_bit;
+ }
+ }
+ }
+
+ if (sealed_layers || IS_ROOT(cursor))
+ break;
+
+ parent = dget_parent(cursor);
+ dput(cursor);
+ if (!parent)
+ return sealed_layers;
+
+ cursor = parent;
+ include_descendants = false;
+ }
+ dput(cursor);
+ return sealed_layers;
+}
+
+/**
+ * deny_no_inherit_topology_change - deny topology changes on sealed layers
+ * @subject: Subject performing the operation (contains the domain).
+ * @dentry: Dentry that is the target of the topology modification.
+ *
+ * Checks whether any domain layers are sealed against topology changes at
+ * @dentry (via collect_topology_sealed_layers). If so, emit an audit record
+ * and return -EACCES. Otherwise return 0.
+ */
+static int deny_no_inherit_topology_change(const struct landlock_cred_security
+ *subject,
+ struct dentry *dentry)
+{
+ layer_mask_t sealed_layers;
+ layer_mask_t override_layers;
+ unsigned long layer_index;
+
+ if (!subject || !dentry || d_is_negative(dentry))
+ return 0;
+ sealed_layers = collect_topology_sealed_layers(subject->domain,
+ dentry, &override_layers);
+ sealed_layers &= ~override_layers;
+
+ if (!sealed_layers)
+ return 0;
+
+ layer_index = __ffs((unsigned long)sealed_layers);
+ landlock_log_denial(subject, &(struct landlock_request) {
+ .type = LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY,
+ .audit = {
+ .type = LSM_AUDIT_DATA_DENTRY,
+ .u.dentry = dentry,
+ },
+ .layer_plus_one = layer_index + 1,
+ });
+
+ return -EACCES;
+}
+
/**
* current_check_refer_path - Check if a rename or link action is allowed
*
@@ -1316,6 +1855,16 @@ static int current_check_refer_path(struct dentry *const old_dentry,
access_request_parent2 =
get_mode_access(d_backing_inode(old_dentry)->i_mode);
if (removable) {
+ int err;
+
+ err = deny_no_inherit_topology_change(subject, old_dentry);
+ if (err)
+ return err;
+ if (exchange) {
+ err = deny_no_inherit_topology_change(subject, new_dentry);
+ if (err)
+ return err;
+ }
access_request_parent1 |= maybe_remove(old_dentry);
access_request_parent2 |= maybe_remove(new_dentry);
}
@@ -1707,12 +2256,31 @@ static int hook_path_symlink(const struct path *const dir,
static int hook_path_unlink(const struct path *const dir,
struct dentry *const dentry)
{
+ const struct landlock_cred_security *const subject =
+ landlock_get_applicable_subject(current_cred(), any_fs, NULL);
+ int err;
+
+ if (subject) {
+ err = deny_no_inherit_topology_change(subject, dentry);
+ if (err)
+ return err;
+ }
return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_FILE);
}
static int hook_path_rmdir(const struct path *const dir,
struct dentry *const dentry)
{
+ const struct landlock_cred_security *const subject =
+ landlock_get_applicable_subject(current_cred(), any_fs, NULL);
+ int err;
+
+ if (subject) {
+ err = deny_no_inherit_topology_change(subject, dentry);
+ if (err)
+ return err;
+ }
+
return current_check_access_path(dir, LANDLOCK_ACCESS_FS_REMOVE_DIR);
}
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index 750a444e1983..f7b6a48bbf39 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -255,8 +255,13 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
return -EINVAL;
if (WARN_ON_ONCE(this->layers[0].level != 0))
return -EINVAL;
+ /* Merge the flags into the rules */
this->layers[0].access |= (*layers)[0].access;
this->layers[0].flags.quiet |= (*layers)[0].flags.quiet;
+ this->layers[0].flags.no_inherit |=
+ (*layers)[0].flags.no_inherit;
+ this->layers[0].flags.has_no_inherit_descendant |=
+ (*layers)[0].flags.has_no_inherit_descendant;
return 0;
}
@@ -315,7 +320,10 @@ int landlock_insert_rule(struct landlock_ruleset *const ruleset,
.level = 0,
.flags = {
.quiet = !!(flags & LANDLOCK_ADD_RULE_QUIET),
- },
+ .no_inherit = !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
+ .has_no_inherit_descendant =
+ !!(flags & LANDLOCK_ADD_RULE_NO_INHERIT),
+ }
} };
build_check_layer();
@@ -662,9 +670,22 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
unsigned long access_bit;
bool is_empty;
- /* Collect rule flags for each layer. */
- if (rule_flags && layer->flags.quiet)
+ /* Skip layers that already have no inherit flags. */
+ if (rule_flags &&
+ (rule_flags->no_inherit_masks & layer_bit))
+ continue;
+
+ /* Collect rule flags for each layer.
+ * We block flag inheritance if needed
+ * because of a no_inherit rule.
+ */
+ if (rule_flags && layer->flags.quiet &&
+ !(rule_flags->blocked_flag_masks & layer_bit))
rule_flags->quiet_masks |= layer_bit;
+ if (rule_flags && layer->flags.no_inherit)
+ rule_flags->no_inherit_masks |= layer_bit;
+ if (rule_flags && layer->flags.has_no_inherit_descendant)
+ rule_flags->no_inherit_desc_masks |= layer_bit;
/*
* Records in @layer_masks which layer grants access to each requested
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index eb60db646422..8b46ab14e995 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -40,6 +40,21 @@ struct landlock_layer {
* down the file hierarchy.
*/
bool quiet:1;
+ /**
+ * @no_inherit: Prevents this rule from being inherited by
+ * descendant directories in the filesystem layer. Only used
+ * for filesystem rules.
+ */
+ bool no_inherit:1;
+ /**
+ * @has_no_inherit_descendant: Marker to indicate that this layer
+ * has at least one descendant directory with a rule having the
+ * no_inherit flag. Only used for filesystem rules.
+ * This "flag" is not set by the user, but by Landlock on
+ * parent directories of rules when the child rule has
+ * a rule with the no_inherit flag.
+ */
+ bool has_no_inherit_descendant:1;
} flags;
/**
* @access: Bitfield of allowed actions on the kernel object. They are
@@ -49,13 +64,32 @@ struct landlock_layer {
};
/**
- * struct collected_rule_flags - Hold accumulated flags for each layer.
+ * struct collected_rule_flags - Hold accumulated flags and their markers for each layer.
*/
struct collected_rule_flags {
/**
* @quiet_masks: Layers for which the quiet flag is effective.
*/
layer_mask_t quiet_masks;
+ /**
+ * @no_inherit_masks: Layers for which the no_inherit flag is effective.
+ */
+ layer_mask_t no_inherit_masks;
+ /**
+ * @no_inherit_desc_masks: Layers for which the
+ * has_no_inherit_descendant tag is effective.
+ * This is not a flag itself, but a marker set on ancestors
+ * of rules with the no_inherit flag to deny topology changes
+ * in the direct parent path.
+ */
+ layer_mask_t no_inherit_desc_masks;
+ /**
+ * @blocked_flag_masks: Layers where flag inheritance must be blocked
+ * because of a no_inherit rule. This is not a flag itself, but a marker
+ * for layers that have their flags blocked due to no_inherit rule
+ * propagation.
+ */
+ layer_mask_t blocked_flag_masks;
};
/**
--
2.51.0
^ permalink raw reply related
* [PATCH v3 0/5] Implement LANDLOCK_ADD_RULE_NO_INHERIT
From: Justin Suess @ 2025-11-26 12:20 UTC (permalink / raw)
To: linux-security-module
Cc: Tingmao Wang, Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, Justin Suess
Hi,
This is version 3 of the LANDLOCK_ADD_RULE_NO_INHERIT series, which
implements a new flag to suppress inheritance of access rights and
flags from parent objects.
This series is rebased on v5 of Tingmao Wang's "quiet flag" series.
The new flag enables policies where a parent directory needs broader
access than its children. For example, a sandbox may permit read-write
access to /home/user but still prohibit writes to ~/.bashrc or
~/.ssh, even though they are nested beneath the parent. Today this is
not possible because access rights always propagate from parent to
child within a layer.
When a rule is added with LANDLOCK_ADD_RULE_NO_INHERIT:
* access rights on parent inodes are ignored for that inode and its
descendants; and
* operations that change the direct parent subtree of such objects
(rename, rmdir, link) are denied up to the mountpoint; and
* parent flags do not propagate below a NO_INHERIT rule (new in v3).
These parent-directory restrictions help mitigate sandbox-restart
attacks: a sandboxed process could otherwise move a protected
directory before exit, causing the next sandbox instance to apply its
policy to the wrong path.
Changes since v2:
1. Add six new selftests for the new flag.
2. Add an optimization to stop permission harvesting when all
relevant layers are tagged with NO_INHERIT.
3. Suppress inheritance of parent flags.
4. Rebase onto v5 of the quiet-flag series.
5. Remove the xarray structure used for flag tracking in favor of
blank rule insertion, simplifying the implementation.
6. Fix edge cases involving flag inheritance across multiple
NO_INHERIT layers.
7. Add documenting comments to new functions.
Links:
v1:
https://lore.kernel.org/linux-security-module/20251105180019.1432367-1-utilityemal77@gmail.com/T/#t
v2:
https://lore.kernel.org/linux-security-module/20251120222346.1157004-1-utilityemal77@gmail.com/T/#t
quiet-flag v5:
https://lore.kernel.org/linux-security-module/cover.1763931318.git.m@maowtm.org/T/#t
Example usage:
# LL_FS_RO="" LL_FS_RW="/" LL_FS_RO_NO_INHERIT="/a/b/c" landlock-sandboxer sh
# touch /a/b/c/fi # denied; / RW does not inherit
# rmdir /a/b/c # denied due to parent-directory protections
# mv /a /bad # denied
# mkdir /a/good; touch /a/good/fi # allowed; unrelated path
If preferred, I'm happy to split the selftests into multiple commits.
I am particularly interested in feedback on:
* The soundness of inserting blank rules in ensure_rule_for_dentry.
A zero-access rule is lazily inserted into parent directories on
first access to enforce topology-change protections. This replaces
the prior xarray tracking, and should reduce complexity and improve
performance.
* Additional edge cases that should be covered by new tests.
* Performance implications of the current design.
All existing Landlock selftests and KUnit tests, as well as the new
tests added in this series, are passing.
Thank you for your time and review.
Regards,
Justin Suess
Justin Suess (5):
landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT
landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT userspace api
samples/landlock: Add LANDLOCK_ADD_RULE_NO_INHERIT to
landlock-sandboxer
selftests/landlock: Implement selftests for
LANDLOCK_ADD_RULE_NO_INHERIT
landlock: Implement KUnit test for LANDLOCK_ADD_RULE_NO_INHERIT
include/uapi/linux/landlock.h | 29 +
samples/landlock/sandboxer.c | 37 +-
security/landlock/audit.c | 4 +-
security/landlock/domain.c | 4 +-
security/landlock/fs.c | 592 ++++++++++++++++++++-
security/landlock/ruleset.c | 116 +++-
security/landlock/ruleset.h | 36 +-
security/landlock/syscalls.c | 14 +-
tools/testing/selftests/landlock/fs_test.c | 459 +++++++++++++++-
9 files changed, 1249 insertions(+), 42 deletions(-)
base-commit: 91d200c5385c926c8d1f2df33a8a4160924fa977
--
2.51.0
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-26 11:31 UTC (permalink / raw)
To: John Johansen, david laight
Cc: John Paul Adrian Glaubitz, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <4034ad19-8e09-440c-a042-a66a488c048b@gmx.de>
* Helge Deller <deller@gmx.de>:
> On 11/26/25 11:44, david laight wrote:
> > On Wed, 26 Nov 2025 01:11:45 -0800
> > John Johansen <john.johansen@canonical.com> wrote:
> >
> > > On 11/25/25 13:13, Helge Deller wrote:
> > > > On 11/25/25 20:20, John Johansen wrote:
> > > > > On 11/25/25 07:11, Helge Deller wrote:
> > > > > > * John Johansen <john.johansen@canonical.com>:
> > > > > > > On 11/18/25 04:49, Helge Deller wrote:
> > > > > > > > Hi Adrian,
> > > > > > > >
> > > > > > > > On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
> > > > > > > > > On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
> > > > > > > > > > My patch fixed two call sites, but I suspect you see another call site which
> > > > > > > > > > hasn't been fixed yet.
> > > > > > > > > >
> > > > > > > > > > Can you try attached patch? It might indicate the caller of the function and
> > > > > > > > > > maybe prints the struct name/address which isn't aligned.
> > > > > > > > > >
> > > > > > > > > > Helge
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > > > > > > > > > index c5a91600842a..b477430c07eb 100644
> > > > > > > > > > --- a/security/apparmor/match.c
> > > > > > > > > > +++ b/security/apparmor/match.c
> > > > > > > > > > @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
> > > > > > > > > > if (size < sizeof(struct table_set_header))
> > > > > > > > > > goto fail;
> > > > > > > > > > + if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
> > > > > > > > > > + pr_warn("dfa blob stream %pS not aligned.\n", data);
> > > > > > > > > > +
> > > > > > > > > > if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
> > > > > > > > > > goto fail;
> > > > > > > > >
> > > > > > > > > Here is the relevant output with the patch applied:
> > > > > > > > >
> > > > > > > > > [ 73.840639] ------------[ cut here ]------------
> > > > > > > > > [ 73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
> > > > > > > > > [ 74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
> > > > > > > > > sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
> > > > > > > > > [ 74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
> > > > > > > > > [ 74.536543] Call Trace:
> > > > > > > > > [ 74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
> > > > > > > > > [ 74.633757] [<0000000000476438>] __warn+0xd8/0x100
> > > > > > > > > [ 74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
> > > > > > > > > [ 74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
> > > > > > > > > [ 74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
> > > > > > > > > [ 74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
> > > > > > > > > [ 74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
> > > > > > > > > [ 75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
> > > > > > > > > [ 75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
> > > > > > > > > [ 75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
> > > > > > > > > [ 75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
> > > > > > > > > [ 75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
> > > > > > > > > [ 75.406932] [<0000000000767174>] sys_write+0x14/0x40
> > > > > > > > > [ 75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
> > > > > > > > > [ 75.548802] ---[ end trace 0000000000000000 ]---
> > > > > > > > > [ 75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
> > > > > > > > > [ 75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
> > > > > > > >
> > > > > > > > The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
> > > > > > > > (via the write syscall).
> > > > > > > > Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
> > > > > > > > a source address which is not correctly aligned.
> > > > > > >
> > > > > > > the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
> > > > > > > is but the userspace tooling hasn't been treating it as aligned. With that said,
> > > > > > > the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
> > > > > > > and there really should be some validation to catch it.
> > > > > > > > You should be able to debug/find the problematic code with strace from userspace.
> > > > > > > > Maybe someone with apparmor knowledge here on the list has an idea?
> > > > > > > This is likely an unaligned 2nd profile, being split out and loaded separately
> > > > > > > from the rest of the container. Basically the loader for some reason (there
> > > > > > > are a few different possible reasons) is poking into the container format and
> > > > > > > pulling out the profile at some offset, this gets loaded to the kernel but
> > > > > > > it would seem that its causing an issue with the dfa alignment within the container,
> > > > > > > which should be aligned to the original container.
> > > > > >
> > > > > >
> > > > > > Regarding this:
> > > > > > > Kernel side, we are going to need to add some extra verification checks, it should
> > > > > > > be catching this, as unaligned as part of the unpack. Userspace side, we will have
> > > > > > > to verify my guess and fix the loader.
> > > > > >
> > > > > > I wonder if loading those tables are really time critical?
> > > > >
> > > > > no, most policy is loaded once on boot and then at package upgrades. There are some
> > > > > bits that may be loaded at application startup like, snap, libvirt, lxd, basically
> > > > > container managers might do some thing custom per container.
> > > > >
> > > > > Its the run time we want to minimize, the cost of.
> > > > >
> > > > > Policy already can be unaligned (the container format rework to fix this is low
> > > > > priority), and is treated as untrusted. It goes through an unpack, and translation to
> > > > > machine native, with as many bounds checks, necessary transforms etc done at unpack
> > > > > time as possible, so that the run time costs can be minimized.
> > > > > > If not, maybe just making the kernel aware that the tables might be unaligned
> > > > > > can help, e.g. with the following (untested) patch.
> > > > > > Adrian, maybe you want to test?
> > > > > > ------------------------
> > > > > >
> > > > > > [PATCH] Allow apparmor to handle unaligned dfa tables
> > > > > >
> > > > > > The dfa tables can originate from kernel or userspace and 8-byte alignment
> > > > > > isn't always guaranteed and as such may trigger unaligned memory accesses
> > > > > > on various architectures.
> > > > > > Work around it by using the get_unaligned_xx() helpers.
> > > > > >
> > > > > > Signed-off-by: Helge Deller <deller@gmx.de>
> > > > > lgtm,
> > > > >
> > > > > Acked-by: John Johansen <john.johansen@canonical.com>
> > > > >
> > > > > I'll pull this into my tree regardless of whether it fixes the issue
> > > > > for Adrian, as it definitely fixes an issue.
> > > > >
> > > > > We can added additional patches on top s needed.
> > > >
> > > > My patch does not modify the UNPACK_ARRAY() macro, which we
> > > > possibly should adjust as well.
> > >
> > > Indeed. See the patch below. I am not surprised testing hasn't triggered this
> > > case, but a malicious userspace could certainly construct a policy that would
> > > trigger it. Yes it would have to be root, but I still would like to prevent
> > > root from being able to trigger this.
> > >
> > > > Adrian's testing seems to trigger only a few unaligned accesses,
> > > > so maybe it's not a issue currently.
> > > I don't think the userspace compiler is generating one that is bad, but it
> > > possible to construct one and get it to the point where it can trip in
> > > UNPACK_ARRAY
> > >
> > > commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
> > > Author: John Johansen <john.johansen@canonical.com>
> > > Date: Tue Nov 25 13:59:32 2025 -0800
> > >
> > > apparmor: fix unaligned memory access of UNPACK_ARRAY
> > > The UNPACK_ARRAY macro has the potential to have unaligned memory
> > > access when the unpacking an unaligned profile, which is caused by
> > > userspace splitting up a profile container before sending it to the
> > > kernel.
> > > While this is corner case, policy loaded from userspace should be
> > > treated as untrusted so ensure that userspace can not trigger an
> > > unaligned access.
> > > Signed-off-by: John Johansen <john.johansen@canonical.com>
> > >
> > > diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> > > index 1fbe82f5021b1..203f7c07529f5 100644
> > > --- a/security/apparmor/include/match.h
> > > +++ b/security/apparmor/include/match.h
> > > @@ -104,7 +104,7 @@ struct aa_dfa {
> > > struct table_header *tables[YYTD_ID_TSIZE];
> > > };
> > > -#define byte_to_byte(X) (X)
> > > +#define byte_to_byte(X) *(X)
> >
> > Even though is is only used once that ought to be (*(X))
> >
> > > #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
> > > do { \
> > > @@ -112,7 +112,7 @@ struct aa_dfa {
> > > TTYPE *__t = (TTYPE *) TABLE; \
> > > BTYPE *__b = (BTYPE *) BLOB; \
> > > for (__i = 0; __i < LEN; __i++) { \
> > > - __t[__i] = NTOHX(__b[__i]); \
> > > + __t[__i] = NTOHX(&__b[__i]); \
> > > } \
> > > } while (0)
> > > diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> > > index 26e82ba879d44..3dcc342337aca 100644
> > > --- a/security/apparmor/match.c
> > > +++ b/security/apparmor/match.c
> > > @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> > > u8, u8, byte_to_byte);
> >
> > Is that that just memcpy() ?
>
> No, it's memcpy() only on big-endian machines.
> On little-endian machines it converts from big-endian
> 16/32-bit ints to little-endian 16/32-bit ints.
>
> But I see some potential for optimization here:
> a) on big-endian machines just use memcpy()
> b) on little-endian machines use memcpy() to copy from possibly-unaligned
> memory to then known-to-be-aligned destination. Then use a loop with
> be32_to_cpu() instead of get_unaligned_xx() as it's faster.
>
> Thoughts?
Like this (untested!) patch:
[PATCH] apparmor: Optimize table creation from possibly unaligned memory
Source blob may come from userspace and might be unaligned.
Try to optize the copying process by avoiding unaligned memory accesses.
Signed-off-by: Helge Deller <deller@gmx.de>
diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
index 1fbe82f5021b..225df6495c84 100644
--- a/security/apparmor/include/match.h
+++ b/security/apparmor/include/match.h
@@ -111,9 +111,14 @@ struct aa_dfa {
typeof(LEN) __i; \
TTYPE *__t = (TTYPE *) TABLE; \
BTYPE *__b = (BTYPE *) BLOB; \
- for (__i = 0; __i < LEN; __i++) { \
- __t[__i] = NTOHX(__b[__i]); \
- } \
+ BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \
+ /* copy to naturally aligned table address */ \
+ memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \
+ /* convert from big-endian if necessary */ \
+ if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \
+ for (__i = 0; __i < LEN; __i++, __t++) { \
+ *__t = NTOHX(*__t); \
+ } \
} while (0)
static inline size_t table_size(size_t len, size_t el_size)
^ permalink raw reply related
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: Helge Deller @ 2025-11-26 11:03 UTC (permalink / raw)
To: david laight, John Johansen
Cc: Helge Deller, John Paul Adrian Glaubitz, linux-kernel, apparmor,
linux-security-module, linux-parisc
In-Reply-To: <20251126104444.29002552@pumpkin>
On 11/26/25 11:44, david laight wrote:
> On Wed, 26 Nov 2025 01:11:45 -0800
> John Johansen <john.johansen@canonical.com> wrote:
>
>> On 11/25/25 13:13, Helge Deller wrote:
>>> On 11/25/25 20:20, John Johansen wrote:
>>>> On 11/25/25 07:11, Helge Deller wrote:
>>>>> * John Johansen <john.johansen@canonical.com>:
>>>>>> On 11/18/25 04:49, Helge Deller wrote:
>>>>>>> Hi Adrian,
>>>>>>>
>>>>>>> On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
>>>>>>>> On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
>>>>>>>>> My patch fixed two call sites, but I suspect you see another call site which
>>>>>>>>> hasn't been fixed yet.
>>>>>>>>>
>>>>>>>>> Can you try attached patch? It might indicate the caller of the function and
>>>>>>>>> maybe prints the struct name/address which isn't aligned.
>>>>>>>>>
>>>>>>>>> Helge
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>>>>>>>>> index c5a91600842a..b477430c07eb 100644
>>>>>>>>> --- a/security/apparmor/match.c
>>>>>>>>> +++ b/security/apparmor/match.c
>>>>>>>>> @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
>>>>>>>>> if (size < sizeof(struct table_set_header))
>>>>>>>>> goto fail;
>>>>>>>>> + if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
>>>>>>>>> + pr_warn("dfa blob stream %pS not aligned.\n", data);
>>>>>>>>> +
>>>>>>>>> if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
>>>>>>>>> goto fail;
>>>>>>>>
>>>>>>>> Here is the relevant output with the patch applied:
>>>>>>>>
>>>>>>>> [ 73.840639] ------------[ cut here ]------------
>>>>>>>> [ 73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
>>>>>>>> sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
>>>>>>>> [ 74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
>>>>>>>> [ 74.536543] Call Trace:
>>>>>>>> [ 74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
>>>>>>>> [ 74.633757] [<0000000000476438>] __warn+0xd8/0x100
>>>>>>>> [ 74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
>>>>>>>> [ 74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
>>>>>>>> [ 74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
>>>>>>>> [ 74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
>>>>>>>> [ 74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
>>>>>>>> [ 75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
>>>>>>>> [ 75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
>>>>>>>> [ 75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
>>>>>>>> [ 75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
>>>>>>>> [ 75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
>>>>>>>> [ 75.406932] [<0000000000767174>] sys_write+0x14/0x40
>>>>>>>> [ 75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
>>>>>>>> [ 75.548802] ---[ end trace 0000000000000000 ]---
>>>>>>>> [ 75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
>>>>>>>> [ 75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
>>>>>>>
>>>>>>> The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
>>>>>>> (via the write syscall).
>>>>>>> Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
>>>>>>> a source address which is not correctly aligned.
>>>>>>
>>>>>> the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
>>>>>> is but the userspace tooling hasn't been treating it as aligned. With that said,
>>>>>> the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
>>>>>> and there really should be some validation to catch it.
>>>>>>
>>>>>>> You should be able to debug/find the problematic code with strace from userspace.
>>>>>>> Maybe someone with apparmor knowledge here on the list has an idea?
>>>>>>>
>>>>>> This is likely an unaligned 2nd profile, being split out and loaded separately
>>>>>> from the rest of the container. Basically the loader for some reason (there
>>>>>> are a few different possible reasons) is poking into the container format and
>>>>>> pulling out the profile at some offset, this gets loaded to the kernel but
>>>>>> it would seem that its causing an issue with the dfa alignment within the container,
>>>>>> which should be aligned to the original container.
>>>>>
>>>>>
>>>>> Regarding this:
>>>>>
>>>>>> Kernel side, we are going to need to add some extra verification checks, it should
>>>>>> be catching this, as unaligned as part of the unpack. Userspace side, we will have
>>>>>> to verify my guess and fix the loader.
>>>>>
>>>>> I wonder if loading those tables are really time critical?
>>>>
>>>> no, most policy is loaded once on boot and then at package upgrades. There are some
>>>> bits that may be loaded at application startup like, snap, libvirt, lxd, basically
>>>> container managers might do some thing custom per container.
>>>>
>>>> Its the run time we want to minimize, the cost of.
>>>>
>>>> Policy already can be unaligned (the container format rework to fix this is low
>>>> priority), and is treated as untrusted. It goes through an unpack, and translation to
>>>> machine native, with as many bounds checks, necessary transforms etc done at unpack
>>>> time as possible, so that the run time costs can be minimized.
>>>>
>>>>> If not, maybe just making the kernel aware that the tables might be unaligned
>>>>> can help, e.g. with the following (untested) patch.
>>>>> Adrian, maybe you want to test?
>>>>>
>>>>
>>>>> ------------------------
>>>>>
>>>>> [PATCH] Allow apparmor to handle unaligned dfa tables
>>>>>
>>>>> The dfa tables can originate from kernel or userspace and 8-byte alignment
>>>>> isn't always guaranteed and as such may trigger unaligned memory accesses
>>>>> on various architectures.
>>>>> Work around it by using the get_unaligned_xx() helpers.
>>>>>
>>>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>>>>
>>>> lgtm,
>>>>
>>>> Acked-by: John Johansen <john.johansen@canonical.com>
>>>>
>>>> I'll pull this into my tree regardless of whether it fixes the issue
>>>> for Adrian, as it definitely fixes an issue.
>>>>
>>>> We can added additional patches on top s needed.
>>>
>>> My patch does not modify the UNPACK_ARRAY() macro, which we
>>> possibly should adjust as well.
>>
>> Indeed. See the patch below. I am not surprised testing hasn't triggered this
>> case, but a malicious userspace could certainly construct a policy that would
>> trigger it. Yes it would have to be root, but I still would like to prevent
>> root from being able to trigger this.
>>
>>> Adrian's testing seems to trigger only a few unaligned accesses,
>>> so maybe it's not a issue currently.
>>>
>> I don't think the userspace compiler is generating one that is bad, but it
>> possible to construct one and get it to the point where it can trip in
>> UNPACK_ARRAY
>>
>> commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
>> Author: John Johansen <john.johansen@canonical.com>
>> Date: Tue Nov 25 13:59:32 2025 -0800
>>
>> apparmor: fix unaligned memory access of UNPACK_ARRAY
>>
>> The UNPACK_ARRAY macro has the potential to have unaligned memory
>> access when the unpacking an unaligned profile, which is caused by
>> userspace splitting up a profile container before sending it to the
>> kernel.
>>
>> While this is corner case, policy loaded from userspace should be
>> treated as untrusted so ensure that userspace can not trigger an
>> unaligned access.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>>
>> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
>> index 1fbe82f5021b1..203f7c07529f5 100644
>> --- a/security/apparmor/include/match.h
>> +++ b/security/apparmor/include/match.h
>> @@ -104,7 +104,7 @@ struct aa_dfa {
>> struct table_header *tables[YYTD_ID_TSIZE];
>> };
>>
>> -#define byte_to_byte(X) (X)
>> +#define byte_to_byte(X) *(X)
>
> Even though is is only used once that ought to be (*(X))
>
>>
>> #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
>> do { \
>> @@ -112,7 +112,7 @@ struct aa_dfa {
>> TTYPE *__t = (TTYPE *) TABLE; \
>> BTYPE *__b = (BTYPE *) BLOB; \
>> for (__i = 0; __i < LEN; __i++) { \
>> - __t[__i] = NTOHX(__b[__i]); \
>> + __t[__i] = NTOHX(&__b[__i]); \
>> } \
>> } while (0)
>>
>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
>> index 26e82ba879d44..3dcc342337aca 100644
>> --- a/security/apparmor/match.c
>> +++ b/security/apparmor/match.c
>> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>> u8, u8, byte_to_byte);
>
> Is that that just memcpy() ?
No, it's memcpy() only on big-endian machines.
On little-endian machines it converts from big-endian
16/32-bit ints to little-endian 16/32-bit ints.
But I see some potential for optimization here:
a) on big-endian machines just use memcpy()
b) on little-endian machines use memcpy() to copy from possibly-unaligned
memory to then known-to-be-aligned destination. Then use a loop with
be32_to_cpu() instead of get_unaligned_xx() as it's faster.
Thoughts?
Helge
> David
>
>> else if (th.td_flags == YYTD_DATA16)
>> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
>> - u16, __be16, be16_to_cpu);
>> + u16, __be16, get_unaligned_be16);
>> else if (th.td_flags == YYTD_DATA32)
>> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
>> - u32, __be32, be32_to_cpu);
>> + u32, __be32, get_unaligned_be32);
>> else
>> goto fail;
>> /* if table was vmalloced make sure the page tables are synced
>>
>>
>>
>
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: david laight @ 2025-11-26 10:44 UTC (permalink / raw)
To: John Johansen
Cc: Helge Deller, Helge Deller, John Paul Adrian Glaubitz,
linux-kernel, apparmor, linux-security-module, linux-parisc
In-Reply-To: <d35010b3-7d07-488c-b5a4-a13380d0ef7c@canonical.com>
On Wed, 26 Nov 2025 01:11:45 -0800
John Johansen <john.johansen@canonical.com> wrote:
> On 11/25/25 13:13, Helge Deller wrote:
> > On 11/25/25 20:20, John Johansen wrote:
> >> On 11/25/25 07:11, Helge Deller wrote:
> >>> * John Johansen <john.johansen@canonical.com>:
> >>>> On 11/18/25 04:49, Helge Deller wrote:
> >>>>> Hi Adrian,
> >>>>>
> >>>>> On 11/18/25 12:43, John Paul Adrian Glaubitz wrote:
> >>>>>> On Tue, 2025-11-18 at 12:09 +0100, Helge Deller wrote:
> >>>>>>> My patch fixed two call sites, but I suspect you see another call site which
> >>>>>>> hasn't been fixed yet.
> >>>>>>>
> >>>>>>> Can you try attached patch? It might indicate the caller of the function and
> >>>>>>> maybe prints the struct name/address which isn't aligned.
> >>>>>>>
> >>>>>>> Helge
> >>>>>>>
> >>>>>>>
> >>>>>>> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> >>>>>>> index c5a91600842a..b477430c07eb 100644
> >>>>>>> --- a/security/apparmor/match.c
> >>>>>>> +++ b/security/apparmor/match.c
> >>>>>>> @@ -313,6 +313,9 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags)
> >>>>>>> if (size < sizeof(struct table_set_header))
> >>>>>>> goto fail;
> >>>>>>> + if (WARN_ON(((unsigned long)data) & (BITS_PER_LONG/8 - 1)))
> >>>>>>> + pr_warn("dfa blob stream %pS not aligned.\n", data);
> >>>>>>> +
> >>>>>>> if (ntohl(*(__be32 *) data) != YYTH_MAGIC)
> >>>>>>> goto fail;
> >>>>>>
> >>>>>> Here is the relevant output with the patch applied:
> >>>>>>
> >>>>>> [ 73.840639] ------------[ cut here ]------------
> >>>>>> [ 73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720
> >>>>>> [ 74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom
> >>>>>> sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common
> >>>>>> [ 74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE
> >>>>>> [ 74.536543] Call Trace:
> >>>>>> [ 74.568561] [<0000000000434c24>] dump_stack+0x8/0x18
> >>>>>> [ 74.633757] [<0000000000476438>] __warn+0xd8/0x100
> >>>>>> [ 74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74
> >>>>>> [ 74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720
> >>>>>> [ 74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0
> >>>>>> [ 74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300
> >>>>>> [ 74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0
> >>>>>> [ 75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160
> >>>>>> [ 75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280
> >>>>>> [ 75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100
> >>>>>> [ 75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420
> >>>>>> [ 75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0
> >>>>>> [ 75.406932] [<0000000000767174>] sys_write+0x14/0x40
> >>>>>> [ 75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44
> >>>>>> [ 75.548802] ---[ end trace 0000000000000000 ]---
> >>>>>> [ 75.609503] dfa blob stream 0xfff0000008926b96 not aligned.
> >>>>>> [ 75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720
> >>>>>
> >>>>> The non-8-byte-aligned address (0xfff0000008926b96) is coming from userspace
> >>>>> (via the write syscall).
> >>>>> Some apparmor userspace tool writes into the apparmor ".replace" virtual file with
> >>>>> a source address which is not correctly aligned.
> >>>>
> >>>> the userpace buffer passed to write(2) has to be aligned? Its certainly nice if it
> >>>> is but the userspace tooling hasn't been treating it as aligned. With that said,
> >>>> the dfa should be padded to be aligned. So this tripping in the dfa is a bug,
> >>>> and there really should be some validation to catch it.
> >>>>
> >>>>> You should be able to debug/find the problematic code with strace from userspace.
> >>>>> Maybe someone with apparmor knowledge here on the list has an idea?
> >>>>>
> >>>> This is likely an unaligned 2nd profile, being split out and loaded separately
> >>>> from the rest of the container. Basically the loader for some reason (there
> >>>> are a few different possible reasons) is poking into the container format and
> >>>> pulling out the profile at some offset, this gets loaded to the kernel but
> >>>> it would seem that its causing an issue with the dfa alignment within the container,
> >>>> which should be aligned to the original container.
> >>>
> >>>
> >>> Regarding this:
> >>>
> >>>> Kernel side, we are going to need to add some extra verification checks, it should
> >>>> be catching this, as unaligned as part of the unpack. Userspace side, we will have
> >>>> to verify my guess and fix the loader.
> >>>
> >>> I wonder if loading those tables are really time critical?
> >>
> >> no, most policy is loaded once on boot and then at package upgrades. There are some
> >> bits that may be loaded at application startup like, snap, libvirt, lxd, basically
> >> container managers might do some thing custom per container.
> >>
> >> Its the run time we want to minimize, the cost of.
> >>
> >> Policy already can be unaligned (the container format rework to fix this is low
> >> priority), and is treated as untrusted. It goes through an unpack, and translation to
> >> machine native, with as many bounds checks, necessary transforms etc done at unpack
> >> time as possible, so that the run time costs can be minimized.
> >>
> >>> If not, maybe just making the kernel aware that the tables might be unaligned
> >>> can help, e.g. with the following (untested) patch.
> >>> Adrian, maybe you want to test?
> >>>
> >>
> >>> ------------------------
> >>>
> >>> [PATCH] Allow apparmor to handle unaligned dfa tables
> >>>
> >>> The dfa tables can originate from kernel or userspace and 8-byte alignment
> >>> isn't always guaranteed and as such may trigger unaligned memory accesses
> >>> on various architectures.
> >>> Work around it by using the get_unaligned_xx() helpers.
> >>>
> >>> Signed-off-by: Helge Deller <deller@gmx.de>
> >>>
> >> lgtm,
> >>
> >> Acked-by: John Johansen <john.johansen@canonical.com>
> >>
> >> I'll pull this into my tree regardless of whether it fixes the issue
> >> for Adrian, as it definitely fixes an issue.
> >>
> >> We can added additional patches on top s needed.
> >
> > My patch does not modify the UNPACK_ARRAY() macro, which we
> > possibly should adjust as well.
>
> Indeed. See the patch below. I am not surprised testing hasn't triggered this
> case, but a malicious userspace could certainly construct a policy that would
> trigger it. Yes it would have to be root, but I still would like to prevent
> root from being able to trigger this.
>
> > Adrian's testing seems to trigger only a few unaligned accesses,
> > so maybe it's not a issue currently.
> >
> I don't think the userspace compiler is generating one that is bad, but it
> possible to construct one and get it to the point where it can trip in
> UNPACK_ARRAY
>
> commit 2c87528c1e7be3976b61ac797c6c8700364c4c63
> Author: John Johansen <john.johansen@canonical.com>
> Date: Tue Nov 25 13:59:32 2025 -0800
>
> apparmor: fix unaligned memory access of UNPACK_ARRAY
>
> The UNPACK_ARRAY macro has the potential to have unaligned memory
> access when the unpacking an unaligned profile, which is caused by
> userspace splitting up a profile container before sending it to the
> kernel.
>
> While this is corner case, policy loaded from userspace should be
> treated as untrusted so ensure that userspace can not trigger an
> unaligned access.
>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
>
> diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h
> index 1fbe82f5021b1..203f7c07529f5 100644
> --- a/security/apparmor/include/match.h
> +++ b/security/apparmor/include/match.h
> @@ -104,7 +104,7 @@ struct aa_dfa {
> struct table_header *tables[YYTD_ID_TSIZE];
> };
>
> -#define byte_to_byte(X) (X)
> +#define byte_to_byte(X) *(X)
Even though is is only used once that ought to be (*(X))
>
> #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \
> do { \
> @@ -112,7 +112,7 @@ struct aa_dfa {
> TTYPE *__t = (TTYPE *) TABLE; \
> BTYPE *__b = (BTYPE *) BLOB; \
> for (__i = 0; __i < LEN; __i++) { \
> - __t[__i] = NTOHX(__b[__i]); \
> + __t[__i] = NTOHX(&__b[__i]); \
> } \
> } while (0)
>
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 26e82ba879d44..3dcc342337aca 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -71,10 +71,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
> u8, u8, byte_to_byte);
Is that that just memcpy() ?
David
> else if (th.td_flags == YYTD_DATA16)
> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> - u16, __be16, be16_to_cpu);
> + u16, __be16, get_unaligned_be16);
> else if (th.td_flags == YYTD_DATA32)
> UNPACK_ARRAY(table->td_data, blob, th.td_lolen,
> - u32, __be32, be32_to_cpu);
> + u32, __be32, get_unaligned_be32);
> else
> goto fail;
> /* if table was vmalloced make sure the page tables are synced
>
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox