* Re: [PATCH v4 02/10] landlock: Add API support and docs for the quiet flags
From: Mickaël Salaün @ 2025-11-21 15:27 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <cd79fcf71e6d38ab4280c0de2500fa5f6b06cd9d.1763330228.git.m@maowtm.org>
On Sun, Nov 16, 2025 at 09:59:32PM +0000, Tingmao Wang wrote:
> Adds the UAPI for the quiet flags feature (but not the implementation
> yet).
>
> According to pahole, even after adding the struct access_masks quiet_masks
> in struct landlock_hierarchy, the u32 log_* bitfield still only has a size
> of 2 bytes, so there's minimal wasted space.
>
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>
> Changes since v3:
> - Minor update to this commit message.
> - Fix minor formatting
>
> Changes since v2:
> - Updated docs from Mickaël's suggestions.
>
> Changes since v1:
> - Per suggestion, added support for quieting only certain access bits,
> controlled by extra quiet_access_* fields in the ruleset_attr.
> - Added docs for the extra fields and made updates to doc changes in v1.
> In particular, call out that the effect of LANDLOCK_ADD_RULE_QUIET is
> independent from the access bits passed in rule_attr
> - landlock_add_rule will return -EINVAL when LANDLOCK_ADD_RULE_QUIET is
> used but the ruleset does not have any quiet access bits set for the
> given rule type.
> - ABI version bump to v8
> - Syntactic and comment changes per suggestion.
>
> include/uapi/linux/landlock.h | 64 +++++++++++++++++
> security/landlock/domain.h | 5 ++
> security/landlock/fs.c | 4 +-
> security/landlock/fs.h | 2 +-
> security/landlock/net.c | 5 +-
> security/landlock/net.h | 3 +-
> security/landlock/ruleset.c | 10 ++-
> security/landlock/ruleset.h | 8 ++-
> security/landlock/syscalls.c | 72 +++++++++++++++-----
> tools/testing/selftests/landlock/base_test.c | 4 +-
> 10 files changed, 150 insertions(+), 27 deletions(-)
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f030adc462ee..50f0806b7e33 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -32,6 +32,19 @@
> * *handle* a wide range or all access rights that they know about at build time
> * (and that they have tested with a kernel that supported them all).
> *
> + * @quiet_access_fs and @quiet_access_net are bitmasks of actions for
> + * which a denial by this layer will not trigger an audit log if the
> + * corresponding object (or its children, for filesystem rules) is marked
> + * with the "quiet" bit via %LANDLOCK_ADD_RULE_QUIET, even if logging
> + * would normally take place per landlock_restrict_self() flags.
> + * quiet_scoped is similar, except that it does not require marking any
> + * objects as quiet - if the ruleset is created with any bits set in
> + * quiet_scoped, then denial of such scoped resources will not trigger any
> + * log. These 3 fields are available since Landlock ABI version 8.
> + *
> + * @quiet_access_fs, @quiet_access_net and @quiet_scoped must be a subset
> + * of @handled_access_fs, @handled_access_net and @scoped respectively.
> + *
> * This structure can grow in future Landlock versions.
> */
> struct landlock_ruleset_attr {
> @@ -51,6 +64,24 @@ struct landlock_ruleset_attr {
> * resources (e.g. IPCs).
> */
> __u64 scoped;
> +
> + /* Since ABI 8: */
> +
> + /**
> + * @quiet_access_fs: Bitmask of filesystem actions which should not be
> + * audit logged if per-object quiet flag is set.
> + */
> + __u64 quiet_access_fs;
> + /**
> + * @quiet_access_net: Bitmask of network actions which should not be
> + * audit logged if per-object quiet flag is set.
> + */
> + __u64 quiet_access_net;
> + /**
> + * @quiet_scoped: Bitmask of scoped actions which should not be audit
> + * logged.
> + */
> + __u64 quiet_scoped;
> };
>
> /**
> @@ -69,6 +100,39 @@ struct landlock_ruleset_attr {
> #define LANDLOCK_CREATE_RULESET_ERRATA (1U << 1)
> /* clang-format on */
>
> +/**
> + * DOC: landlock_add_rule_flags
> + *
> + * **Flags**
> + *
> + * %LANDLOCK_ADD_RULE_QUIET
> + * Together with the quiet_* fields in struct landlock_ruleset_attr,
> + * this flag controls whether Landlock will log audit messages when
> + * access to the objects covered by this rule is denied by this layer.
> + *
> + * If audit logging is enabled, when Landlock denies an access, it will
> + * suppress the audit log if all of the following are true:
> + *
> + * - This layer is the innermost layer that denied the access;
Because these items follows ":" they should not start with a capital
letter (e.g. "- this layer ...").
> + * - All requested accesses are part of the quiet_* fields in the
> + * related struct landlock_ruleset_attr;
This should be updated to reflect my comment in the next patch about
landlock_log_denial().
> + * - The object (or one of its parents, for filesystem rules) is
> + * marked as "quiet" via %LANDLOCK_ADD_RULE_QUIET.
> + *
> + * Because logging is only suppressed by a layer if the layer denies
> + * access, a sandboxed program cannot use this flag to "hide" access
> + * denials, without denying itself the access in the first place.
> + *
> + * The effect of this flag does not depend on the value of
> + * 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.
> + */
> +
> +/* clang-format off */
> +#define LANDLOCK_ADD_RULE_QUIET (1U << 0)
> +/* clang-format on */
> +
> /**
> * DOC: landlock_restrict_self_flags
> *
> diff --git a/security/landlock/domain.h b/security/landlock/domain.h
> index 7fb70b25f85a..aadbf53505c0 100644
> --- a/security/landlock/domain.h
> +++ b/security/landlock/domain.h
> @@ -114,6 +114,11 @@ struct landlock_hierarchy {
> * %LANDLOCK_RESTRICT_SELF_LOG_NEW_EXEC_ON. Set to false by default.
> */
> log_new_exec : 1;
> + /**
> + * @quiet_masks: Bitmasks of access that should be quieted (i.e. not
> + * logged) if the related object is marked as quiet.
> + */
> + struct access_masks quiet_masks;
Please update the above @work_free doc.
> #endif /* CONFIG_AUDIT */
> };
>
^ permalink raw reply
* Re: [PATCH -next] ima: Handle error code returned by ima_filter_rule_match()
From: Mimi Zohar @ 2025-11-21 15:38 UTC (permalink / raw)
To: Zhao Yipeng, roberto.sassu, dmitry.kasatkin, eric.snowberg
Cc: lujialin4, linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20251120071805.1604864-1-zhaoyipeng5@huawei.com>
On Thu, 2025-11-20 at 15:18 +0800, Zhao Yipeng wrote:
> In ima_match_rules(), if ima_filter_rule_match() returns -ENOENT due to
> the rule being NULL, the function incorrectly skips the 'if (!rc)' check
> and sets 'result = true'. The LSM rule is considered a match, causing
> extra files to be measured by IMA.
>
> This issue can be reproduced in the following scenario:
> After unloading the SELinux policy module via 'semodule -d', if an IMA
> measurement is triggered before ima_lsm_rules is updated,
> in ima_match_rules(), the first call to ima_filter_rule_match() returns
> -ESTALE. This causes the code to enter the 'if (rc == -ESTALE &&
> !rule_reinitialized)' block, perform ima_lsm_copy_rule() and retry. In
> ima_lsm_copy_rule(), since the SELinux module has been removed, the rule
> becomes NULL, and the second call to ima_filter_rule_match() returns
> -ENOENT. This bypasses the 'if (!rc)' check and results in a false match.
>
> Call trace:
> selinux_audit_rule_match+0x310/0x3b8
> security_audit_rule_match+0x60/0xa0
> ima_match_rules+0x2e4/0x4a0
> ima_match_policy+0x9c/0x1e8
> ima_get_action+0x48/0x60
> process_measurement+0xf8/0xa98
> ima_bprm_check+0x98/0xd8
> security_bprm_check+0x5c/0x78
> search_binary_handler+0x6c/0x318
> exec_binprm+0x58/0x1b8
> bprm_execve+0xb8/0x130
> do_execveat_common.isra.0+0x1a8/0x258
> __arm64_sys_execve+0x48/0x68
> invoke_syscall+0x50/0x128
> el0_svc_common.constprop.0+0xc8/0xf0
> do_el0_svc+0x24/0x38
> el0_svc+0x44/0x200
> el0t_64_sync_handler+0x100/0x130
> el0t_64_sync+0x3c8/0x3d0
>
> Fix this by changing 'if (!rc)' to 'if (rc <= 0)' to ensure that error
> codes like -ENOENT do not bypass the check and accidentally result in a
> successful match.
>
> Fixes: 4af4662fa4a9d ("integrity: IMA policy")
> Signed-off-by: Zhao Yipeng <zhaoyipeng5@huawei.com>
Thank you. The patch is now queued in next-integrity.
Mimi
^ permalink raw reply
* Re: [PATCH v4 03/10] landlock: Suppress logging when quiet flag is present
From: Mickaël Salaün @ 2025-11-21 15:27 UTC (permalink / raw)
To: Tingmao Wang
Cc: Günther Noack, Justin Suess, Jan Kara, Abhinav Saxena,
linux-security-module
In-Reply-To: <5c0de8ee7e00aff1aceb3a80f5af162eeaaa06db.1763330228.git.m@maowtm.org>
On Sun, Nov 16, 2025 at 09:59:33PM +0000, Tingmao Wang wrote:
> The quietness behaviour is as documented in the previous patch.
>
> For optional accesses, since the existing deny_masks can only store 2x4bit
> of layer index, with no way to represent "no layer", we need to either
> expand it or have another field to correctly handle quieting of those.
> This commit uses the latter approach - we add another field to store which
> optional access (of the 2) are covered by quiet rules in their respective
> layers as stored in deny_masks.
>
> We can avoid making struct landlock_file_security larger by converting the
> existing fown_layer to a 4bit field. This commit does that, and adds test
> to ensure that it is large enough for LANDLOCK_MAX_NUM_LAYERS-1.
>
> Signed-off-by: Tingmao Wang <m@maowtm.org>
> ---
>
> Changes since v2:
> - Renamed patch title from "Check for quiet flag in landlock_log_denial"
> to this given the growth.
> - Moved quiet bit check after domain_exec check
> - Rename, style and comment fixes suggested by Mickaël.
> - Squashed patch 6/6 from v2 "Implement quiet for optional accesses" into
> this one. Changes to that below:
> - Refactor the quiet flag setting in get_layer_from_deny_masks() to be
> more clear.
> - Add KUnit tests
> - Fix comments, add WARN_ON_ONCE, use __const_hweight64() as suggested by
> review
> - Move build_check_file_security to fs.c
> - Use a typedef for quiet_optional_accesses, add static_assert, and
> improve docs on landlock_get_quiet_optional_accesses.
>
> Changes since v1:
> - Supports the new quiet access masks.
> - Support quieting scope requests (but not ptrace and attempted mounting
> for now)
>
> security/landlock/access.h | 5 +
> security/landlock/audit.c | 255 +++++++++++++++++++++++++++++++++---
> security/landlock/audit.h | 4 +-
> security/landlock/domain.c | 33 +++++
> security/landlock/domain.h | 5 +
> security/landlock/fs.c | 45 +++++--
> security/landlock/fs.h | 17 ++-
> security/landlock/net.c | 3 +-
> security/landlock/ruleset.h | 5 +
> security/landlock/task.c | 12 +-
> 10 files changed, 344 insertions(+), 40 deletions(-)
>
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d..db47edc88afa 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -97,4 +97,9 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)
> return access_masks;
> }
>
> +/* A bitmask that is large enough to hold set of optional accesses. */
> +typedef u8 optional_access_t;
> +static_assert(BITS_PER_TYPE(optional_access_t) >=
> + HWEIGHT(_LANDLOCK_ACCESS_FS_OPTIONAL));
> +
> #endif /* _SECURITY_LANDLOCK_ACCESS_H */
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index c52d079cdb77..505b13c8f4c2 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -273,7 +273,7 @@ static void test_get_denied_layer(struct kunit *const test)
> static size_t
> get_layer_from_deny_masks(access_mask_t *const access_request,
> const access_mask_t all_existing_optional_access,
> - const deny_masks_t deny_masks)
> + const deny_masks_t deny_masks, u8 quiet_optional_accesses, bool *quiet)
> {
> const unsigned long access_opt = all_existing_optional_access;
> const unsigned long access_req = *access_request;
> @@ -285,18 +285,29 @@ get_layer_from_deny_masks(access_mask_t *const access_request,
> /* This will require change with new object types. */
> WARN_ON_ONCE(access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL);
>
> + *quiet = false;
I'd prefer to have a local variable and just at before returning,
setting the quiet parameter, similar to access_request.
> for_each_set_bit(access_bit, &access_opt,
> BITS_PER_TYPE(access_mask_t)) {
> if (access_req & BIT(access_bit)) {
> const size_t layer =
> (deny_masks >> (access_index * 4)) &
> (LANDLOCK_MAX_NUM_LAYERS - 1);
> + const bool layer_has_quiet =
> + !!(quiet_optional_accesses & BIT(access_index));
>
> if (layer > youngest_layer) {
> youngest_layer = layer;
> + *quiet = layer_has_quiet;
> missing = BIT(access_bit);
> } else if (layer == youngest_layer) {
> missing |= BIT(access_bit);
> + /*
> + * Whether the layer has rules with quiet flag covering
> + * the file accessed does not depend on the access, and so
> + * if this fails, quiet_optional_accesses is corrupted.
> + */
> + WARN_ON_ONCE(*quiet && !layer_has_quiet);
> + *quiet = layer_has_quiet;
In this case, why update *quiet?
> }
> }
> access_index++;
> @@ -312,42 +323,188 @@ static void test_get_layer_from_deny_masks(struct kunit *const test)
> {
> deny_masks_t deny_mask;
> access_mask_t access;
> + u8 quiet_optional_accesses;
> + bool quiet;
>
> /* truncate:0 ioctl_dev:2 */
> deny_mask = 0x20;
> + quiet_optional_accesses = 0;
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE;
> KUNIT_EXPECT_EQ(test, 0,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* layer denying truncate: quiet, ioctl: not quiet */
> + quiet_optional_accesses = 0b01;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* Reverse order - truncate:2 ioctl_dev:0 */
> + deny_mask = 0x02;
> + quiet_optional_accesses = 0;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* layer denying truncate: quiet, ioctl: not quiet */
> + quiet_optional_accesses = 0b01;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> KUNIT_EXPECT_EQ(test, 2,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + /* layer denying truncate: not quiet, ioctl: quiet */
> + quiet_optional_accesses = 0b10;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 0,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 2,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
>
> /* truncate:15 ioctl_dev:15 */
> deny_mask = 0xff;
> + quiet_optional_accesses = 0;
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE;
> + KUNIT_EXPECT_EQ(test, 15,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> + KUNIT_EXPECT_EQ(test, 15,
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> + KUNIT_EXPECT_EQ(test, access,
> + LANDLOCK_ACCESS_FS_TRUNCATE |
> + LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, false);
> +
> + /* Both quiet (same layer so quietness must be the same) */
> + quiet_optional_accesses = 0b11;
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE;
> KUNIT_EXPECT_EQ(test, 15,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_TRUNCATE);
> + KUNIT_EXPECT_EQ(test, quiet, true);
>
> access = LANDLOCK_ACCESS_FS_TRUNCATE | LANDLOCK_ACCESS_FS_IOCTL_DEV;
> KUNIT_EXPECT_EQ(test, 15,
> - get_layer_from_deny_masks(&access,
> - _LANDLOCK_ACCESS_FS_OPTIONAL,
> - deny_mask));
> + get_layer_from_deny_masks(
> + &access, _LANDLOCK_ACCESS_FS_OPTIONAL,
> + deny_mask, quiet_optional_accesses, &quiet));
> KUNIT_EXPECT_EQ(test, access,
> LANDLOCK_ACCESS_FS_TRUNCATE |
> LANDLOCK_ACCESS_FS_IOCTL_DEV);
> + KUNIT_EXPECT_EQ(test, quiet, true);
> }
>
> #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> @@ -381,19 +538,39 @@ static bool is_valid_request(const struct landlock_request *const request)
> return true;
> }
>
> +static access_mask_t
> +pick_access_mask_for_request_type(const enum landlock_request_type type,
> + const struct access_masks access_masks)
> +{
> + switch (type) {
> + case LANDLOCK_REQUEST_FS_ACCESS:
> + return access_masks.fs;
> + case LANDLOCK_REQUEST_NET_ACCESS:
> + return access_masks.net;
> + default:
> + WARN_ONCE(1, "Invalid request type %d passed to %s", type,
> + __func__);
> + return 0;
> + }
> +}
> +
> /**
> * landlock_log_denial - Create audit records related to a denial
> *
> * @subject: The Landlock subject's credential denying an action.
> * @request: Detail of the user space request.
> + * @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if
> + * this is a scope request (no particular object involved).
> */
> void landlock_log_denial(const struct landlock_cred_security *const subject,
> - const struct landlock_request *const request)
> + const struct landlock_request *const request,
> + const struct collected_rule_flags rule_flags)
It would be simpler and limit code change to move rule_flags/quiet_flags
into struct landlock_request, which means we can also remove
no_rule_flags.
> {
> struct audit_buffer *ab;
> struct landlock_hierarchy *youngest_denied;
> size_t youngest_layer;
> - access_mask_t missing;
> + access_mask_t missing, quiet_mask;
> + bool object_quiet_flag = false, quiet_applicable_to_access = false;
>
> if (WARN_ON_ONCE(!subject || !subject->domain ||
> !subject->domain->hierarchy || !request))
> @@ -409,10 +586,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> youngest_layer = get_denied_layer(
> subject->domain, &missing, request->layer_masks,
> request->layer_masks_size);
> + object_quiet_flag = !!(rule_flags.quiet_masks & BIT(youngest_layer));
> } else {
> youngest_layer = get_layer_from_deny_masks(
> &missing, request->all_existing_optional_access,
> - request->deny_masks);
> + request->deny_masks,
> + request->quiet_optional_accesses,
> + &object_quiet_flag);
> }
> youngest_denied =
> get_hierarchy(subject->domain, youngest_layer);
> @@ -447,6 +627,49 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> return;
> }
>
> + /*
> + * Checks if the object is marked quiet by the layer that denied the
> + * request. If it's a different layer that marked it as quiet, but
> + * that layer is not the one that denied the request, we should still
> + * audit log the denial.
> + */
> + if (object_quiet_flag) {
> + /*
> + * We now check if the denied requests are all covered by the
> + * layer's quiet access bits.
> + */
> + quiet_mask = pick_access_mask_for_request_type(
This quiet_mask is only used in this branch, so we can declare it here
and make it const:
const access_mask_t quiet_mask = pick_access_mask_for_request_type(
> + request->type, youngest_denied->quiet_masks);
> + quiet_applicable_to_access = (quiet_mask & missing) == missing;
I think it should be:
quiet_applicable_to_access = (quiet_mask & missing) == (handled_mask & missing);
We should have a test for this case: an access request (e.g. read-write)
is denied, half by one layer (e.g. read) and half by another (e.g.
write). Tests should cover this matrix.
> + } else {
> + /*
> + * Either the object is not quiet, or this is a scope request. We
> + * check request->type to distinguish between the two cases.
> + */
> + quiet_mask = youngest_denied->quiet_masks.scope;
const access_mask_t quiet_mask = youngest_denied->quiet_masks.scope;
> + switch (request->type) {
> + case LANDLOCK_REQUEST_SCOPE_SIGNAL:
> + quiet_applicable_to_access =
> + !!(quiet_mask & LANDLOCK_SCOPE_SIGNAL);
> + break;
> + case LANDLOCK_REQUEST_SCOPE_ABSTRACT_UNIX_SOCKET:
> + quiet_applicable_to_access =
> + !!(quiet_mask &
> + LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET);
> + break;
> + /*
> + * Leave LANDLOCK_REQUEST_PTRACE and
> + * LANDLOCK_REQUEST_FS_CHANGE_TOPOLOGY unhandled for now - they are
> + * never quiet.
> + */
> + default:
> + break;
> + }
> + }
> +
> + if (quiet_applicable_to_access)
> + return;
> +
> /* Uses consistent allocation flags wrt common_lsm_audit(). */
> ab = audit_log_start(audit_context(), GFP_ATOMIC | __GFP_NOWARN,
> AUDIT_LANDLOCK_ACCESS);
^ permalink raw reply
* [PATCH v2] security: Rename functions and add namespace mapping tests
From: Ryan Foster @ 2025-11-21 17:48 UTC (permalink / raw)
To: serge, paul, linux-security-module; +Cc: linux-kernel, Ryan Foster
In-Reply-To: <20251110143748.4144288-1-foster.ryan.r@gmail.com>
Rename rootid_owns_currentns() to uid_owns_currentns() and
rootid_owns_userns() to uid_owns_ns() for clarity, as the function checks
any UID, not just root. Update all call sites accordingly.
Add tests that create actual user namespaces with different UID mappings
to verify namespace traversal logic. The tests create namespaces where
uid 0 maps to different kuids (e.g., kuid 1000, 2000) and verify that
uid_owns_ns() correctly identifies ownership based on the namespace
hierarchy traversal.
This addresses feedback to use clearer function naming and test actual
namespace functionality with real user namespace creation and mappings,
rather than just basic input validation.
---
security/commoncap.c | 26 ++--
security/commoncap_test.c | 286 ++++++++++++++++++++++++++++++++------
2 files changed, 254 insertions(+), 58 deletions(-)
diff --git a/security/commoncap.c b/security/commoncap.c
index 15d8147a34c4..cca291df9551 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -359,16 +359,16 @@ int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry)
}
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
-bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
-bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
+bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
+bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
#else
-static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
+static bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid)
#endif
{
struct user_namespace *iter;
for (iter = ns;; iter = iter->parent) {
- if (from_kuid(iter, kroot) == 0)
+ if (from_kuid(iter, kuid) == 0)
return true;
if (iter == &init_user_ns)
break;
@@ -378,19 +378,19 @@ static bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot)
}
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
-bool rootid_owns_currentns(vfsuid_t rootvfsuid);
-bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+bool uid_owns_currentns(vfsuid_t vfsuid);
+bool uid_owns_currentns(vfsuid_t vfsuid)
#else
-static bool rootid_owns_currentns(vfsuid_t rootvfsuid)
+static bool uid_owns_currentns(vfsuid_t vfsuid)
#endif
{
- kuid_t kroot;
+ kuid_t kuid;
- if (!vfsuid_valid(rootvfsuid))
+ if (!vfsuid_valid(vfsuid))
return false;
- kroot = vfsuid_into_kuid(rootvfsuid);
- return rootid_owns_userns(current_user_ns(), kroot);
+ kuid = vfsuid_into_kuid(vfsuid);
+ return uid_owns_ns(current_user_ns(), kuid);
}
static __u32 sansflags(__u32 m)
@@ -497,7 +497,7 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
goto out_free;
}
- if (!rootid_owns_currentns(vfsroot)) {
+ if (!uid_owns_currentns(vfsroot)) {
size = -EOVERFLOW;
goto out_free;
}
@@ -738,7 +738,7 @@ int get_vfs_caps_from_disk(struct mnt_idmap *idmap,
/* Limit the caps to the mounter of the filesystem
* or the more limited uid specified in the xattr.
*/
- if (!rootid_owns_currentns(rootvfsuid))
+ if (!uid_owns_currentns(rootvfsuid))
return -ENODATA;
cpu_caps->permitted.val = le32_to_cpu(caps->data[0].permitted);
diff --git a/security/commoncap_test.c b/security/commoncap_test.c
index 962aa899455d..7f066dc0df5d 100644
--- a/security/commoncap_test.c
+++ b/security/commoncap_test.c
@@ -10,6 +10,8 @@
#include <linux/user_namespace.h>
#include <linux/uidgid.h>
#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/refcount.h>
/* Forward declare types and functions we need from mnt_idmapping.h
* We avoid including the full header because it contains inline functions
@@ -50,38 +52,38 @@ static inline kuid_t vfsuid_into_kuid(vfsuid_t vfsuid)
#ifdef CONFIG_SECURITY_COMMONCAP_KUNIT_TEST
/* Forward declarations - functions are exported when KUNIT_TEST is enabled */
-extern bool rootid_owns_userns(struct user_namespace *ns, kuid_t kroot);
-extern bool rootid_owns_currentns(vfsuid_t rootvfsuid);
+extern bool uid_owns_ns(struct user_namespace *ns, kuid_t kuid);
+extern bool uid_owns_currentns(vfsuid_t vfsuid);
/**
- * test_rootid_owns_currentns_init_ns - Test rootid_owns_currentns with init ns
+ * test_uid_owns_currentns_init_ns - Test uid_owns_currentns with init ns
*
- * Verifies that a root ID in the init namespace correctly owns the current
+ * Verifies that UID 0 in the init namespace correctly owns the current
* namespace when running in init_user_ns.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_init_ns(struct kunit *test)
+static void test_uid_owns_currentns_init_ns(struct kunit *test)
{
- vfsuid_t root_vfsuid;
- kuid_t root_kuid;
+ vfsuid_t vfsuid;
+ kuid_t kuid;
- /* Create a root UID in init namespace */
- root_kuid = KUIDT_INIT(0);
- root_vfsuid = VFSUIDT_INIT(root_kuid);
+ /* Create UID 0 in init namespace */
+ kuid = KUIDT_INIT(0);
+ vfsuid = VFSUIDT_INIT(kuid);
- /* In init namespace, root should own current namespace */
- KUNIT_EXPECT_TRUE(test, rootid_owns_currentns(root_vfsuid));
+ /* In init namespace, UID 0 should own current namespace */
+ KUNIT_EXPECT_TRUE(test, uid_owns_currentns(vfsuid));
}
/**
- * test_rootid_owns_currentns_invalid - Test rootid_owns_currentns with invalid vfsuid
+ * test_uid_owns_currentns_invalid - Test uid_owns_currentns with invalid vfsuid
*
* Verifies that an invalid vfsuid correctly returns false.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_invalid(struct kunit *test)
+static void test_uid_owns_currentns_invalid(struct kunit *test)
{
vfsuid_t invalid_vfsuid;
@@ -89,74 +91,268 @@ static void test_rootid_owns_currentns_invalid(struct kunit *test)
invalid_vfsuid = INVALID_VFSUID;
/* Invalid vfsuid should return false */
- KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(invalid_vfsuid));
+ KUNIT_EXPECT_FALSE(test, uid_owns_currentns(invalid_vfsuid));
}
/**
- * test_rootid_owns_currentns_nonroot - Test rootid_owns_currentns with non-root UID
+ * test_uid_owns_currentns_nonzero - Test uid_owns_currentns with non-zero UID
*
- * Verifies that a non-root UID correctly returns false.
+ * Verifies that a non-zero UID correctly returns false.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_currentns_nonroot(struct kunit *test)
+static void test_uid_owns_currentns_nonzero(struct kunit *test)
{
- vfsuid_t nonroot_vfsuid;
- kuid_t nonroot_kuid;
+ vfsuid_t vfsuid;
+ kuid_t kuid;
- /* Create a non-root UID */
- nonroot_kuid = KUIDT_INIT(1000);
- nonroot_vfsuid = VFSUIDT_INIT(nonroot_kuid);
+ /* Create a non-zero UID */
+ kuid = KUIDT_INIT(1000);
+ vfsuid = VFSUIDT_INIT(kuid);
- /* Non-root UID should return false */
- KUNIT_EXPECT_FALSE(test, rootid_owns_currentns(nonroot_vfsuid));
+ /* Non-zero UID should return false */
+ KUNIT_EXPECT_FALSE(test, uid_owns_currentns(vfsuid));
}
/**
- * test_rootid_owns_userns_init_ns - Test rootid_owns_userns with init namespace
+ * test_uid_owns_ns_init_ns_uid0 - Test uid_owns_ns with init namespace and UID 0
*
- * Verifies that rootid_owns_userns correctly identifies root UID in init namespace.
- * This tests the core namespace traversal logic.
+ * Verifies that uid_owns_ns correctly identifies UID 0 in init namespace.
+ * This tests the core namespace traversal logic. In init namespace, UID 0
+ * maps to itself, so it should own the namespace.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_userns_init_ns(struct kunit *test)
+static void test_uid_owns_ns_init_ns_uid0(struct kunit *test)
{
- kuid_t root_kuid;
+ kuid_t kuid;
struct user_namespace *init_ns;
- root_kuid = KUIDT_INIT(0);
+ kuid = KUIDT_INIT(0);
init_ns = &init_user_ns;
- /* Root UID should own init namespace */
- KUNIT_EXPECT_TRUE(test, rootid_owns_userns(init_ns, root_kuid));
+ /* UID 0 should own init namespace */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
}
/**
- * test_rootid_owns_userns_nonroot - Test rootid_owns_userns with non-root UID
+ * test_uid_owns_ns_init_ns_nonzero - Test uid_owns_ns with init namespace and non-zero UID
*
- * Verifies that rootid_owns_userns correctly rejects non-root UIDs.
+ * Verifies that uid_owns_ns correctly rejects non-zero UIDs in init namespace.
+ * Only UID 0 should own a namespace.
*
* @test: KUnit test context
*/
-static void test_rootid_owns_userns_nonroot(struct kunit *test)
+static void test_uid_owns_ns_init_ns_nonzero(struct kunit *test)
{
- kuid_t nonroot_kuid;
+ kuid_t kuid;
struct user_namespace *init_ns;
- nonroot_kuid = KUIDT_INIT(1000);
+ kuid = KUIDT_INIT(1000);
init_ns = &init_user_ns;
- /* Non-root UID should not own namespace */
- KUNIT_EXPECT_FALSE(test, rootid_owns_userns(init_ns, nonroot_kuid));
+ /* Non-zero UID should not own namespace */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+}
+
+/**
+ * test_uid_owns_ns_init_ns_various_uids - Test uid_owns_ns with various UIDs
+ *
+ * Verifies that uid_owns_ns correctly identifies only UID 0 as owning
+ * the namespace, regardless of the UID value tested.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_init_ns_various_uids(struct kunit *test)
+{
+ struct user_namespace *init_ns;
+ kuid_t kuid;
+
+ init_ns = &init_user_ns;
+
+ /* UID 0 should own the namespace */
+ kuid = KUIDT_INIT(0);
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(init_ns, kuid));
+
+ /* Other UIDs should not own the namespace */
+ kuid = KUIDT_INIT(1);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+
+ kuid = KUIDT_INIT(1000);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+
+ kuid = KUIDT_INIT(65534);
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(init_ns, kuid));
+}
+
+/**
+ * create_test_user_ns_with_mapping - Create a test user namespace with uid mapping
+ *
+ * Creates a minimal user namespace for testing where uid 0 in the namespace
+ * maps to the specified kuid in the parent namespace.
+ *
+ * The mapping semantics:
+ * - first: uid in this namespace (0)
+ * - lower_first: kuid in parent namespace (mapped_kuid)
+ * - count: range size (1)
+ *
+ * This means: from_kuid(ns, mapped_kuid) will return 0
+ * because map_id_up looks for kuid in [lower_first, lower_first+count)
+ * and returns first + (kuid - lower_first) = 0 + (mapped_kuid - mapped_kuid) = 0
+ *
+ * @test: KUnit test context
+ * @parent_ns: Parent user namespace
+ * @mapped_kuid: The kuid that uid 0 in the new namespace maps to
+ *
+ * Returns: The new user namespace, or NULL on failure
+ */
+static struct user_namespace *create_test_user_ns_with_mapping(struct kunit *test,
+ struct user_namespace *parent_ns,
+ kuid_t mapped_kuid)
+{
+ struct user_namespace *ns;
+ struct uid_gid_extent extent;
+
+ /* Allocate a test namespace - use kzalloc to zero all fields */
+ ns = kunit_kzalloc(test, sizeof(*ns), GFP_KERNEL);
+ if (!ns)
+ return NULL;
+
+ /* Initialize basic namespace structure fields */
+ ns->parent = parent_ns;
+ ns->level = parent_ns ? parent_ns->level + 1 : 0;
+ ns->owner = mapped_kuid;
+ ns->group = KGIDT_INIT(0);
+
+ /* Initialize ns_common structure */
+ refcount_set(&ns->ns.__ns_ref, 1);
+
+ /* Set up uid mapping: uid 0 in this namespace maps to mapped_kuid in parent
+ * Format: first (uid in ns) : lower_first (kuid in parent) : count
+ * So: uid 0 in ns -> kuid mapped_kuid in parent
+ * This means from_kuid(ns, mapped_kuid) returns 0
+ */
+ extent.first = 0; /* uid 0 in this namespace */
+ extent.lower_first = __kuid_val(mapped_kuid); /* maps to this kuid in parent */
+ extent.count = 1;
+
+ ns->uid_map.extent[0] = extent;
+ ns->uid_map.nr_extents = 1;
+
+ /* Set up gid mapping: gid 0 maps to gid 0 in parent (simplified) */
+ extent.first = 0;
+ extent.lower_first = 0;
+ extent.count = 1;
+
+ ns->gid_map.extent[0] = extent;
+ ns->gid_map.nr_extents = 1;
+
+ return ns;
+}
+
+/**
+ * test_uid_owns_ns_with_mapping - Test uid_owns_ns with namespace where uid 0
+ * maps to different kuid
+ *
+ * Creates a user namespace where uid 0 maps to kuid 1000 in the parent namespace.
+ * Verifies that uid_owns_ns correctly identifies kuid 1000 as owning the namespace.
+ *
+ * Note: uid_owns_ns walks up the namespace hierarchy, so it checks the current
+ * namespace first, then parent, then parent's parent, etc. So:
+ * - kuid 1000 owns test_ns because from_kuid(test_ns, 1000) == 0
+ * - kuid 0 also owns test_ns because from_kuid(init_user_ns, 0) == 0
+ * (checked in parent)
+ *
+ * This tests the actual functionality as requested: creating namespaces with
+ * different values for the namespace's uid 0.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_with_mapping(struct kunit *test)
+{
+ struct user_namespace *test_ns;
+ struct user_namespace *parent_ns;
+ kuid_t mapped_kuid, other_kuid;
+
+ parent_ns = &init_user_ns;
+ mapped_kuid = KUIDT_INIT(1000); /* uid 0 in test_ns maps to kuid 1000 */
+ other_kuid = KUIDT_INIT(2000); /* This should not own the namespace */
+
+ /* Create test namespace where uid 0 maps to kuid 1000 */
+ test_ns = create_test_user_ns_with_mapping(test, parent_ns, mapped_kuid);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_ns);
+
+ /* kuid 1000 should own the namespace (because uid 0 in test_ns maps to it) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, mapped_kuid));
+
+ /* kuid 0 also owns the namespace because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(test_ns, KUIDT_INIT(0)));
+
+ /* Other kuids that don't map to 0 in test_ns or any parent should not own */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, other_kuid));
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(test_ns, KUIDT_INIT(500)));
+}
+
+/**
+ * test_uid_owns_ns_with_different_mappings - Test with multiple namespaces
+ * having different mappings
+ *
+ * Creates multiple test namespaces with different uid 0 mappings to verify
+ * the function correctly identifies ownership based on the mapping.
+ *
+ * Since uid_owns_ns walks up the hierarchy, kuids that map to 0 in init_user_ns
+ * (like kuid 0) will own all namespaces. But we can still verify that the
+ * specific mapped kuids own their respective namespaces.
+ *
+ * @test: KUnit test context
+ */
+static void test_uid_owns_ns_with_different_mappings(struct kunit *test)
+{
+ struct user_namespace *ns1, *ns2, *ns3;
+ struct user_namespace *parent_ns;
+
+ parent_ns = &init_user_ns;
+
+ /* Namespace 1: uid 0 maps to kuid 1000 */
+ ns1 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(1000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns1);
+ /* kuid 1000 owns ns1 because it maps to uid 0 in ns1 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(1000)));
+ /* kuid 0 also owns ns1 because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns1, KUIDT_INIT(0)));
+ /* kuid 2000 doesn't map to 0 in ns1 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns1, KUIDT_INIT(2000)));
+
+ /* Namespace 2: uid 0 maps to kuid 2000 */
+ ns2 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(2000));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns2);
+ /* kuid 2000 owns ns2 because it maps to uid 0 in ns2 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(2000)));
+ /* kuid 0 also owns ns2 because it maps to 0 in init_user_ns (parent) */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns2, KUIDT_INIT(0)));
+ /* kuid 1000 doesn't map to 0 in ns2 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns2, KUIDT_INIT(1000)));
+
+ /* Namespace 3: uid 0 maps to kuid 0 (identity mapping) */
+ ns3 = create_test_user_ns_with_mapping(test, parent_ns, KUIDT_INIT(0));
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ns3);
+ /* kuid 0 owns ns3 because it maps to uid 0 in ns3 */
+ KUNIT_EXPECT_TRUE(test, uid_owns_ns(ns3, KUIDT_INIT(0)));
+ /* kuid 1000 doesn't map to 0 in ns3 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(1000)));
+ /* kuid 2000 doesn't map to 0 in ns3 or any parent */
+ KUNIT_EXPECT_FALSE(test, uid_owns_ns(ns3, KUIDT_INIT(2000)));
}
static struct kunit_case commoncap_test_cases[] = {
- KUNIT_CASE(test_rootid_owns_currentns_init_ns),
- KUNIT_CASE(test_rootid_owns_currentns_invalid),
- KUNIT_CASE(test_rootid_owns_currentns_nonroot),
- KUNIT_CASE(test_rootid_owns_userns_init_ns),
- KUNIT_CASE(test_rootid_owns_userns_nonroot),
+ KUNIT_CASE(test_uid_owns_currentns_init_ns),
+ KUNIT_CASE(test_uid_owns_currentns_invalid),
+ KUNIT_CASE(test_uid_owns_currentns_nonzero),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_uid0),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_nonzero),
+ KUNIT_CASE(test_uid_owns_ns_init_ns_various_uids),
+ KUNIT_CASE(test_uid_owns_ns_with_mapping),
+ KUNIT_CASE(test_uid_owns_ns_with_different_mappings),
{}
};
--
2.43.0
^ permalink raw reply related
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Anirudh Venkataramanan @ 2025-11-21 19:13 UTC (permalink / raw)
To: Roberto Sassu, linux-integrity
Cc: Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <7722dff4e68ef5fb7f39bd732a8a77422bad5549.camel@huaweicloud.com>
On 11/20/2025 3:02 AM, Roberto Sassu wrote:
> On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
>> ==========================================================================
>>> A. Introduction |
>> ==========================================================================
>>
>> IMA events are kept in kernel memory and preserved across kexec soft
>> reboots. This can lead to increased kernel memory usage over time,
>> especially with aggressive IMA policies that measure everything. To reduce
>> memory pressure, it becomes necessary to discard IMA events but given that
>> IMA events are extended into PCRs in the TPM, just discarding events will
>> break the PCR extension chain, making future verification of the IMA event
>> log impossible.
>>
>> This patch series proposes a method to discard IMA events while keeping the
>> log verifiable. While reducing memory pressure is the primary objective,
>> the second order benefit of trimming the IMA log is that IMA log verifiers
>> (local userspace daemon or a remote cloud service) can process smaller IMA
>> logs on a rolling basis, thus avoiding re-verification of previously
>> verified events.
>
> Hi Anirudh
Hi Roberto,
Thanks for the feedback! Few questions below.
>
> I will rephrase this paragraph, to be sure that I understood it
> correctly.
>
> You are proposing a method to trim the measurement list and, at the
> same time, to keep the measurement list verifiable after trimming. The
> way you would like to achieve that is to keep the verification state in
> the kernel in the form of PCR values.
>
> Those values mean what verifiers have already verified. Thus, for the
> next verification attempt, verifiers take the current PCR values as
> starting values, replay the truncated IMA measurement list, and again
> you match with the current PCR values and you trim until that point.
>
> So the benefit of this proposal is that you keep the verification of
> the IMA measurement list self-contained by using the last verification
> state (PCR starting value) and the truncated IMA measurement list as
> the inputs of your verification.
Your understanding as described above is correct.
>
> Let me reiterate on the trusted computing principles IMA relies on for
> providing the evidence about a system's integrity.
>
> Unless you are at the beginning of the measurement chain, where the
> Root of Trust for Measurement (RTM) is trusted by assumption, the
> measurements done by a component can be trusted because that component
> was already measured by the previous component in the boot chain,
> before it had any chance to corrupt the system.
>
> In the context of IMA, IMA can be trusted to make new measurements
> because it measures every file before those files could cause any harm
> to the system. So, potentially IMA and the kernel can be corrupted by
> any file.
>
> What you are proposing would not work, because you are placing trust in
> an input (the PCR starting value) that can be manipulated at any time
> by a corrupted kernel, before you had the chance to detect such
> corruption.
If starting PCR values can be corrupted, the IMA measurements list can
also be corrupted, right?
More generally, what integrity guarantees can be provided (if any) if
the kernel itself is corrupted?
>
> Let me describe a scenario where I could take advantage of such
> weakness. After the first measurement list trim, I perform an attack on
> the system such that it corrupts the kernel. IMA added a new entry in
> the measurement list, which would reveal the attack.
>
> But, since I have control of the kernel, I conveniently update the PCR
> starting value to replay the new measurement entry, and remove the
> measurement entry from the measurement list.
>
> Now, from the perspective of the user space verifiers everything is
> fine, the truncated IMA measurement list is clean, no attack, and the
> current PCR values match by replaying the new PCR starting value with
> the remaining of the IMA measurement list.
Wouldn't the verifier detect the attack when it sees that its
recalculated PCR values don't match up to the PCR digest in the TPM quote?
>
> So, in my opinion the kernel should just offer the ability to trim the
> measurement list, and a remote verifier should be responsible to verify
> the measurement list, without relying on anything from the system being
> evaluated.
>
> Sure, the remote verifier can verify just the trimmed IMA measurement
> list, but the remote verifier must solely rely on state information
> maintained internally.
>
> Roberto
>
>> The method has other advantages too:
>>
>> 1. It provides a userspace interface that can be used to precisely control
>> trim point, allowing for trim points to be optionally aligned with
>> userspace IMA event log validation.
>>
>> 2. It ensures that all necessary information required for continued IMA
>> log validation is made available via the userspace interface at all
>> times.
>>
>> 3. It provides a simple mechanism for userspace applications to determine
>> if the event log has been unexpectedly trimmed.
>>
>> 4. The duration for which the IMA Measurement list mutex must be held (for
>> trimming) is minimal.
>>
>> ==========================================================================
>>> B. Solution |
>> ==========================================================================
>>
>> --------------------------------------------------------------------------
>>> B.1 Overview |
>> --------------------------------------------------------------------------
>>
>> The kernel trims the IMA event log based on PCR values supplied by userspace.
>> The core principles leveraged are as follows:
>>
>> - Given an IMA event log, PCR values for each IMA event can be obtained by
>> recalulating the PCR extension for each event. Thus processing N events
>> from the start will yield PCR values as of event N. This is referred to
>> as "IMA event log replay".
>>
>> - To get the PCR value for event N + 1, only the PCR value as of event N
>> is needed. If this can be known, events till and including N can be
>> safely purged.
>>
>> Putting it all together, we get the following userspace + kernel flow:
>>
>> 1. A userspace application replays the IMA event log to generate PCR
>> values and then triggers a trim by providing these values to the kernel
>> (by writing to a pseudo file).
>>
>> Optionally, the userspace application may verify these PCR values
>> against the corresponding TPM quote, and trigger trimming only if
>> the calculated PCR values match up to the expectations in the quote's
>> PCR digest.
>>
>> 2. The kernel uses the userspace supplied PCR values to trim the IMA
>> measurements list at a specific point, and so these are referred to as
>> "trim-to PCR values" in this context.
>>
>> Note that the kernel doesn't really understand what these userspace
>> provided PCR values mean or what IMA event they correspond to, and so
>> it does its own IMA event replay till either the replayed PCR values
>> match with the userspace provided ones, or it runs out of events.
>>
>> If a match is found, the kernel can proceed with trimming the IMA
>> measurements list. This is done in two steps, to keep locking context
>> minimal.
>>
>> step 1: Find and return the list entry (as a count from head) of exact
>> match. This does not lock the measurements list mutex, ensuring
>> new events can be appended to the log.
>>
>> step 2: Lock the measurements list mutex and trim the measurements list
>> at the previously identified list entry.
>>
>> If the trim is successful, the trim-to PCR values are saved as "starting
>> PCR values". The next time userspace wants to replay the IMA event log,
>> it will use the starting PCR values as the base for the IMA event log
>> replay.
>>
>> --------------------------------------------------------------------------
>>> B.2 Kernel Interfaces |
>> --------------------------------------------------------------------------
>>
>> A new configfs pseudo file /sys/kernel/config/ima/pcrs that supports the
>> following operations is exposed.
>>
>> read: returns starting PCR values stored in the kernel (within IMA
>> specifically).
>>
>> write: writes trim-to PCR values to trigger trimming. If trimming is
>> successful, trim-to PCR values are stored as starting PCR values.
>> requires root privileges.
>>
>> --------------------------------------------------------------------------
>>> B.3 Walk-through with a real example |
>> --------------------------------------------------------------------------
>>
>> This is a real example from a test run.
>>
>> Suppose this IMA policy is deployed:
>>
>> measure func=FILE_CHECK mask=MAY_READ pcr=10
>> measure func=FILE_CHECK mask=MAY_WRITE pcr=11
>>
>> When the policy is deployed, a zero digest starting PCR value will be set
>> for each PCR used. If the TPM supports multiple hashbanks, there will be
>> one starting PCR value per PCR, per TPM hashbank. This can be seen in the
>> following hexdump:
>>
>> $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
>> 00000000 70 63 72 31 30 3a 73 68 61 31 3a 00 00 00 00 00 |pcr10:sha1:.....|
>> 00000010 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 |...............p|
>> 00000020 63 72 31 31 3a 73 68 61 31 3a 00 00 00 00 00 00 |cr11:sha1:......|
>> 00000030 00 00 00 00 00 00 00 00 00 00 00 00 00 00 70 63 |..............pc|
>> 00000040 72 31 30 3a 73 68 61 32 35 36 3a 00 00 00 00 00 |r10:sha256:.....|
>> 00000050 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000060 00 00 00 00 00 00 00 00 00 00 00 70 63 72 31 31 |...........pcr11|
>> 00000070 3a 73 68 61 32 35 36 3a 00 00 00 00 00 00 00 00 |:sha256:........|
>> 00000080 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000090 00 00 00 00 00 00 00 00 70 63 72 31 30 3a 73 68 |........pcr10:sh|
>> 000000a0 61 33 38 34 3a 00 00 00 00 00 00 00 00 00 00 00 |a384:...........|
>> 000000b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 000000c0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 000000d0 00 00 00 00 00 70 63 72 31 31 3a 73 68 61 33 38 |.....pcr11:sha38|
>> 000000e0 34 3a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |4:..............|
>> 000000f0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000100 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 |................|
>> 00000110 00 00 |..|
>> 00000112
>>
>> Let's say that a userspace utility replays the IMA event log, and triggers
>> trimming by writing the following PCR values (i.e. trim-to PCR values) to the
>> pseudo file:
>>
>> pcr10:sha256:8268782906555cf3aefc179f815c878527dd4e67eaa836572ebabab31977922c
>> pcr11:sha256:4c7f31927183eacb53d51d95b0162916fd3fca51a8d1efc6dde3805eb891fe41
>>
>> The trim is successful,
>>
>> 1. Some number of entries from the measurements log will disappear. This
>> can be verified by reading out the ASCII or binary IMA measurements
>> file.
>>
>> 2. The trim-to PCR values are saved as starting PCR values. This can be
>> verified by reading out the pseudo file again as shown below. Note that
>> even through only sha256 PCR values were written, the kernel populated
>> sha1 and sha384 starting values as well.
>>
>> $ sudo hexdump -vC /sys/kernel/config/ima/pcrs
>>
>> 00000000 70 63 72 31 30 3a 73 68 61 31 3a c4 7f 9d 00 68 |pcr10:sha1:....h|
>> 00000010 e4 86 71 bf bc ae f0 10 12 ff 68 e2 9e 74 e4 70 |..q.......h..t.p|
>> 00000020 63 72 31 31 3a 73 68 61 31 3a 90 d7 17 ac 60 4d |cr11:sha1:....`M|
>> 00000030 c8 25 ce 77 7d 9d 94 cf 44 7b b2 2e 2e e2 70 63 |.%.w}...D{....pc|
>> 00000040 72 31 30 3a 73 68 61 32 35 36 3a 82 68 78 29 06 |r10:sha256:.hx).|
>> 00000050 55 5c f3 ae fc 17 9f 81 5c 87 85 27 dd 4e 67 ea |U\......\..'.Ng.|
>> 00000060 a8 36 57 2e ba ba b3 19 77 92 2c 70 63 72 31 31 |.6W.....w.,pcr11|
>> 00000070 3a 73 68 61 32 35 36 3a 4c 7f 31 92 71 83 ea cb |:sha256:L.1.q...|
>> 00000080 53 d5 1d 95 b0 16 29 16 fd 3f ca 51 a8 d1 ef c6 |S.....)..?.Q....|
>> 00000090 dd e3 80 5e b8 91 fe 41 70 63 72 31 30 3a 73 68 |...^...Apcr10:sh|
>> 000000a0 61 33 38 34 3a 8e d6 12 18 b1 d6 cd 95 16 98 33 |a384:..........3|
>> 000000b0 2b 7d a2 d6 d9 05 c7 e8 5b 15 b0 91 c5 fc 23 d1 |+}......[.....#.|
>> 000000c0 f9 a8 8d 60 50 5c e9 64 5f d7 b3 b2 f1 9c 90 0a |...`P\.d_.......|
>> 000000d0 45 53 5d b2 57 70 63 72 31 31 3a 73 68 61 33 38 |ES].Wpcr11:sha38|
>> 000000e0 34 3a 25 fc 21 28 31 5a f7 c6 fb 0f 40 c9 06 e6 |4:%.!(1Z....@...|
>> 000000f0 c5 da ed 20 61 a1 03 54 4f 67 18 88 82 0f 48 d1 |... a..TOg....H.|
>> 00000100 2f e0 3d 36 46 5e 94 a4 88 51 f8 91 39 7e e5 97 |/.=6F^...Q..9~..|
>> 00000110 2c c5 |,.|
>> 00000112
>>
>> --------------------------------------------------------------------------
>>> C. Footnotes |
>> --------------------------------------------------------------------------
>>
>> 1. The 'pcrs' pseudo file is currently part of configfs. This was due to
>> some early internal feedback in a different context. This can as well be
>> in securityfs with the rest of the IMA pseudo files.
>>
>> 2. PCR values are never read out of the TPM at any point. All PCR values
>> used are derived from IMA event log replay.
>>
>> 3. Code is "RFC quality". Refinements can be made if the method is accepted.
>>
>> 4. For functional validation, base kernel version was 6.17 stable, with the
>> most recent tested version being 6.17.8.
>>
>> 5. Code has been validated to some degree using a python-based internal test
>> tool. This can be published if there is community interest.
>>
>> Steven Chen (1):
>> ima: Implement IMA event log trimming
>>
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/ima/Kconfig | 13 +
>> drivers/ima/Makefile | 2 +
>> drivers/ima/ima_config_pcrs.c | 291 ++++++++++++++++++
>> include/linux/ima.h | 27 ++
>> security/integrity/ima/Makefile | 4 +
>> security/integrity/ima/ima.h | 8 +
>> security/integrity/ima/ima_init.c | 44 +++
>> security/integrity/ima/ima_log_trim.c | 421 ++++++++++++++++++++++++++
>> security/integrity/ima/ima_policy.c | 7 +-
>> security/integrity/ima/ima_queue.c | 5 +-
>> 12 files changed, 821 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/ima/Kconfig
>> create mode 100644 drivers/ima/Makefile
>> create mode 100644 drivers/ima/ima_config_pcrs.c
>> create mode 100644 security/integrity/ima/ima_log_trim.c
>>
>
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Eric W. Biederman @ 2025-11-21 19:19 UTC (permalink / raw)
To: Bernd Edlinger
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet
In-Reply-To: <GV2PPF74270EBEED0840E45459881C0EDD4E4D5A@GV2PPF74270EBEE.EURP195.PROD.OUTLOOK.COM>
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> On 11/21/25 10:35, Bernd Edlinger wrote:
>> On 11/21/25 08:18, Eric W. Biederman wrote:
>>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>>
>>>> Hi Eric,
>>>>
>>>> thanks for you valuable input on the topic.
>>>>
>>>> On 11/21/25 00:50, Eric W. Biederman wrote:
>>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>>>
>>>>>> Instead of computing the new cred before we pass the point of no
>>>>>> return compute the new cred just before we use it.
>>>>>>
>>>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
>>>>>>
>>>>>> I am not certain why we wanted to compute the cred for the new
>>>>>> executable so early. Perhaps I missed something but I did not see any
>>>>>> common errors being signaled. So I don't think we loose anything by
>>>>>> computing the new cred later.
>>>>>
>>>>> I should add that the permission checks happen in open_exec,
>>>>> everything that follows credential wise is just about representing in
>>>>> struct cred the credentials the new executable will have.
>>>>>
>>>>> So I am really at a loss why we have had this complicated way of
>>>>> computing of computed the credentials all of these years full of
>>>>> time of check to time of use problems.
>>>>>
>>>>
>>>> Well, I think I see a problem with your patch:
>>>>
>>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
>>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
>>>> or the smack security engines at least. Previously that callback
>>>> was called before the point of no return, and the return code should
>>>> be returned as a return code the the caller of execve. But if we move
>>>> that check after the point of no return, the caller will get killed
>>>> due to the failed security check.
>>>>
>>>> Or did I miss something?
>>>
>>> I think we definitely need to document this change in behavior. I would
>>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
>>> issue. The exec is failing one way or the other so I don't see it as a
>>> correctness issue.
>>>
>>> In the case of ptrace in general I think it is a bug if the mere act of
>>> debugging a program changes it's behavior. So which buggy behavior
>>> should we prefer? SIGSEGV where it is totally clear that the behavior
>>> has changed or -EPERM and ask the debugged program to handle it.
>>> I lean towards SIGSEGV because then it is clear the code should not
>>> handle it.
>>>
>>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
>>> handle unexpected things happening is to terminate the application.
>>>
>>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
>>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
>>> of CLONE_THREAD.
>>>
>>>
>>> Plus all of these things are only considerations if we are exec'ing a
>>> program that transitions to a different set of credentials. Something
>>> that happens but is quite rare itself.
>>>
>>> In practice I don't expect there is anything that depends on the exact
>>> behavior of what happens when exec'ing a suid executable to gain
>>> privileges when ptraced. The closes I can imagine is upstart and
>>> I think upstart ran as root when ptracing other programs so there is no
>>> gaining of privilege and thus no reason for a security module to
>>> complain.
>>>
>>> Who knows I could be wrong, and someone could actually care. Which is
>>> hy I think we should document it.>>
>>
>>
>> Well, I dont know for sure, but the security engine could deny the execution
>> for any reason, not only because of being ptraced.
>> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
>>
>>
>> Bernd.
>>
>
> Hmm, funny..
>
> I installed this patch on top of
>
> commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
> Merge: c966813ea120 7b6216baae75
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Thu Nov 20 11:04:37 2025 -0800
>
> but it does panic when I try to boot:
>
> [ 0.870539] TERM=1inux
> [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
> [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
> [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
> [ 0.870580] Call Trace:
> [ 0.870590] <TASK>
> [ 0.870592] vpanic+0x36d/0x380
> [ 0.870607] ? __pfx_kernel_init+0x10/0x10
> [ 0.870615] panic+0x5b/0x60
> [ 0.870617] kernel_init+0x17d/0x1c0
> [ 0.870623] ret_from_fork+0x124/0x150
> [ 0.870625} ? __pfx_kernel_init+0x10/0x10
> [ 0.870627] ret_from_fork_asm+0x1a/0x30
> [ 0.870632] </TASK>
> [ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
> [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
>
>
> Is that a known problem?
Nope. It looks like the code needs a little bit bug fixing testing.
I will take see about taking a look.
Eric
^ permalink raw reply
* Re: [RFC v1 0/1] Implement IMA Event Log Trimming
From: Anirudh Venkataramanan @ 2025-11-21 19:53 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Paul Moore,
James Morris, Serge E . Hallyn, linux-security-module,
Steven Chen, Gregory Lumen, Lakshmi Ramasubramanian,
Sush Shringarputale
In-Reply-To: <629289c119a1a71d8d7a1208ec3676e006d4d527.camel@linux.ibm.com>
On 11/20/2025 2:58 PM, Mimi Zohar wrote:
> On Wed, 2025-11-19 at 13:33 -0800, Anirudh Venkataramanan wrote:
>>
>>
>> 2. The kernel uses the userspace supplied PCR values to trim the IMA
>> measurements list at a specific point, and so these are referred to as
>> "trim-to PCR values" in this context.
>>
>> Note that the kernel doesn't really understand what these userspace
>> provided PCR values mean or what IMA event they correspond to, and so
>> it does its own IMA event replay till either the replayed PCR values
>> match with the userspace provided ones, or it runs out of events.
>>
>> If a match is found, the kernel can proceed with trimming the IMA
>> measurements list. This is done in two steps, to keep locking context
>> minimal.
>>
>> step 1: Find and return the list entry (as a count from head) of exact
>> match. This does not lock the measurements list mutex, ensuring
>> new events can be appended to the log.
>>
>> step 2: Lock the measurements list mutex and trim the measurements list
>> at the previously identified list entry.
>>
>> If the trim is successful, the trim-to PCR values are saved as "starting
>> PCR values". The next time userspace wants to replay the IMA event log,
>> it will use the starting PCR values as the base for the IMA event log
>> replay.
>
> Depending on the IMA policy, the IMA measurement list may contain integrity
> violations, such as "ToM/ToU" (Time of Measurement/Time of Use) or "open-
> writer". Either the userspace supplied PCR values will not match any measurement
> record or the PCR values will be "fixed" to match the well known violation hash
> value extended into the TPM. Depending on how the userspace PCR values will
> subsequently be used, saving the "fixed" PCR values could result in whitewashing
> the integrity of the measurement list.
>
Yes, we account for this. IMA documentation [1] notes that:
"An all zeros hash indicates a measurement log violation. IMA is
invalidating an entry. Trust in entries after that are up to the end
user. If the Template Data Hash is all zeros, an all ones digest is
extended."
A userspace verifier could be designed with a user option like
"--ignore-violations". This would extend a digest of all ones in the
event replay process and arrive at the same per-event PCR values that
the TPM originally did. The trimming logic in the kernel would do the
same thing.
By "whitewashing", you mean that violations will also get trimmed? Given
that trust in entries post violation is up to the user, is this a
problem? The IMA log itself can still be saved by userspace for further
analysis, auditing, etc. The point of trimming is to get it out of the
kernel memory.
Thanks!
Ani
[1]
https://ima-doc.readthedocs.io/en/latest/event-log-format.html#template-data-hash
^ permalink raw reply
* Re: [PATCH] KEYS: encrypted: Use pr_fmt()
From: Jarkko Sakkinen @ 2025-11-21 20:10 UTC (permalink / raw)
To: Thorsten Blum
Cc: Mimi Zohar, David Howells, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, keyrings, linux-security-module,
linux-kernel
In-Reply-To: <83C83079-0354-4642-A980-DBC7AE572A53@linux.dev>
On Wed, Nov 19, 2025 at 03:45:02PM +0100, Thorsten Blum wrote:
> On 19. Nov 2025, at 03:48, Jarkko Sakkinen wrote:
> > On Thu, Nov 13, 2025 at 01:35:44PM +0100, Thorsten Blum wrote:
> >> Use pr_fmt() to automatically prefix all pr_<level>() log messages with
> >
> > This fails to describe what "use" means.
>
> I don't understand what you mean. What's wrong with "use ... to ..."?
I think e.g., "Rewrite the definition of ..." describes better what
you're doing.
>
> >> "encrypted_key: " and remove all manually added prefixes.
> >>
> >> Reformat the code accordingly and avoid line breaks in log messages.
> >>
> >> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >> ---
> >> security/keys/encrypted-keys/encrypted.c | 74 +++++++++++-------------
> >> security/keys/encrypted-keys/encrypted.h | 2 +-
> >> 2 files changed, 35 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> >> index 513c09e2b01c..a8e8bf949b4b 100644
> >> --- a/security/keys/encrypted-keys/encrypted.c
> >> +++ b/security/keys/encrypted-keys/encrypted.c
> >> @@ -11,6 +11,8 @@
> >> * See Documentation/security/keys/trusted-encrypted.rst
> >> */
> >>
> >
> > Should have undef prepending.
>
> Why is this necessary when the #define is at the top of a source file?
> The kernel documentation [1] doesn't mention this anywhere. Isn't #undef
> only needed when redefining 'pr_fmt' in the middle of a file to avoid a
> compiler warning/error?
>
> >> +#define pr_fmt(fmt) "encrypted_key: " fmt
> >> +
> >> [...]
>
> Thanks,
> Thorsten
>
> [1] https://docs.kernel.org/core-api/printk-basics.html
>
BR, Jarkko
^ permalink raw reply
* [GIT PULL] selinux/selinux-pr-20251121
From: Paul Moore @ 2025-11-21 20:57 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Three SELinux patches for v6.18 to fix issues around accessing the
per-task decision cache that we introduced in v6.16 to help reduce
SELinux overhead on path walks. The problem was that despite the
cache being located in the SELinux "task_security_struct", the
parent struct wasn't actually tied to the task, it was tied to a cred.
Historically SELinux did locate the task_security_struct in the
task_struct's security blob, but it was later relocated to the cred
struct when the cred work happened, as it made the most sense at the
time. Unfortunately we never did the task_security_struct to
cred_security_struct rename work (avoid code churn maybe? who knows)
because it didn't really matter at the time. However, it suddenly
became a problem when we added a per-task cache to a per-cred object
and didn't notice because of the old, no-longer-correct struct naming.
Thanks to KCSAN for flagging this, as the silly humans running
things forgot that the task_security_struct was a big lie.
This pull request contains three patches, only one of which actually
fixes the problem described above and moves the SELinux decision cache
from the per-cred struct to a newly (re)created per-task struct. The
other two patches, which form the bulk of the diffstat, take care of
the associated renaming tasks so we can hopefully avoid making the
same stupid mistake in the future. For the record, I did contemplate
sending just a fix for the cache, leaving the renaming patches for the
upcoming merge window, but the type/variable naming ended up being
pretty awful and would have made v6.18 an outlier stuck between the
"old" names and the "new" names in v6.19. The renaming patches are
also fairly mechanical/trivial and shouldn't pose much risk despite
their size.
TLDR; naming things may be hard, but if you mess it up bad things
happen.
Paul
--
The following changes since commit 211ddde0823f1442e4ad052a2f30f050145ccada:
Linux 6.18-rc2 (2025-10-19 15:19:16 -1000)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20251121
for you to fetch changes up to 3ded250b97c3ae94a642bc2e710a95700e72dfb0:
selinux: rename the cred_security_struct variables to "crsec"
(2025-11-20 16:47:50 -0500)
----------------------------------------------------------------
selinux/stable-6.18 PR 20251121
----------------------------------------------------------------
Paul Moore (1):
selinux: rename the cred_security_struct variables to "crsec"
Stephen Smalley (2):
selinux: rename task_security_struct to cred_security_struct
selinux: move avdcache to per-task security struct
security/selinux/hooks.c | 251 +++++++++++++++---------------
security/selinux/include/objsec.h | 22 +-
2 files changed, 144 insertions(+), 129 deletions(-)
--
paul-moore.com
^ permalink raw reply
* Re: [RFC][PATCH] exec: Move cred computation under exec_update_lock
From: Ryan Lee @ 2025-11-21 23:06 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Bernd Edlinger, Alexander Viro, Alexey Dobriyan, Oleg Nesterov,
Kees Cook, Andy Lutomirski, Will Drewry, Christian Brauner,
Andrew Morton, Michal Hocko, Serge Hallyn, James Morris,
Randy Dunlap, Suren Baghdasaryan, Yafang Shao, Helge Deller,
Adrian Reber, Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, apparmor
In-Reply-To: <87ikf3w5us.fsf@email.froward.int.ebiederm.org>
On Fri, Nov 21, 2025 at 11:20 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>
> > On 11/21/25 10:35, Bernd Edlinger wrote:
> >> On 11/21/25 08:18, Eric W. Biederman wrote:
> >>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> >>>
> >>>> Hi Eric,
> >>>>
> >>>> thanks for you valuable input on the topic.
> >>>>
> >>>> On 11/21/25 00:50, Eric W. Biederman wrote:
> >>>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> >>>>>
> >>>>>> Instead of computing the new cred before we pass the point of no
> >>>>>> return compute the new cred just before we use it.
> >>>>>>
> >>>>>> This allows the removal of fs_struct->in_exec and cred_guard_mutex.
> >>>>>>
> >>>>>> I am not certain why we wanted to compute the cred for the new
> >>>>>> executable so early. Perhaps I missed something but I did not see any
> >>>>>> common errors being signaled. So I don't think we loose anything by
> >>>>>> computing the new cred later.
> >>>>>
> >>>>> I should add that the permission checks happen in open_exec,
> >>>>> everything that follows credential wise is just about representing in
> >>>>> struct cred the credentials the new executable will have.
> >>>>>
> >>>>> So I am really at a loss why we have had this complicated way of
> >>>>> computing of computed the credentials all of these years full of
> >>>>> time of check to time of use problems.
> >>>>>
> >>>>
> >>>> Well, I think I see a problem with your patch:
> >>>>
> >>>> When the security engine gets the LSM_UNSAFE_PTRACE flag, it might
> >>>> e.g. return -EPERM in bprm_creds_for_exec in the apparmor, selinux
> >>>> or the smack security engines at least. Previously that callback
> >>>> was called before the point of no return, and the return code should
> >>>> be returned as a return code the the caller of execve. But if we move
> >>>> that check after the point of no return, the caller will get killed
> >>>> due to the failed security check.
> >>>>
> >>>> Or did I miss something?
> >>>
> >>> I think we definitely need to document this change in behavior. I would
> >>> call ending the exec with SIGSEGV vs -EPERM a quality of implementation
> >>> issue. The exec is failing one way or the other so I don't see it as a
> >>> correctness issue.
> >>>
> >>> In the case of ptrace in general I think it is a bug if the mere act of
> >>> debugging a program changes it's behavior. So which buggy behavior
> >>> should we prefer? SIGSEGV where it is totally clear that the behavior
> >>> has changed or -EPERM and ask the debugged program to handle it.
> >>> I lean towards SIGSEGV because then it is clear the code should not
> >>> handle it.
> >>>
> >>> In the case of LSM_UNSAFE_NO_NEW_PRIVS I believe the preferred way to
> >>> handle unexpected things happening is to terminate the application.
> >>>
> >>> In the case of LSM_UNSAFE_SHARE -EPERM might be better. I don't know
> >>> of any good uses of any good uses of sys_clone(CLONE_FS ...) outside
> >>> of CLONE_THREAD.
> >>>
> >>>
> >>> Plus all of these things are only considerations if we are exec'ing a
> >>> program that transitions to a different set of credentials. Something
> >>> that happens but is quite rare itself.
AppArmor's exec rules rely heavily on transitioning to different creds
on exec. For example, an AppArmor policy like
profile example_1 /usr/bin/example_1 {
/usr/bin/example_2 Px -> example_2_profile,
/usr/bin/example_3 Px,
}
will allow binary example_1 to execute binaries example_2 and
example_3, launching those processes under a different confinement
(example_2_profile and a profile that attaches to /usr/bin/example_3,
respectively). We will need to look into how much this patch (or a
corresponding change in behavior) would affect our use case, but
confinement transitions (where the confinement information is stored
as an LSM blob on the cred struct) are extremely common in a system
that uses AppArmor as an LSM.
> >>>
> >>> In practice I don't expect there is anything that depends on the exact
> >>> behavior of what happens when exec'ing a suid executable to gain
> >>> privileges when ptraced. The closes I can imagine is upstart and
> >>> I think upstart ran as root when ptracing other programs so there is no
> >>> gaining of privilege and thus no reason for a security module to
> >>> complain.
> >>>
> >>> Who knows I could be wrong, and someone could actually care. Which is
> >>> hy I think we should document it.>>
> >>
> >>
> >> Well, I dont know for sure, but the security engine could deny the execution
> >> for any reason, not only because of being ptraced.
> >> Maybe there can be a policy which denies user X to execute e.g. any suid programs.
> >>
> >>
> >> Bernd.
> >>
> >
> > Hmm, funny..
> >
> > I installed this patch on top of
> >
> > commit fd95357fd8c6778ac7dea6c57a19b8b182b6e91f (HEAD -> master, origin/master, origin/HEAD)
> > Merge: c966813ea120 7b6216baae75
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date: Thu Nov 20 11:04:37 2025 -0800
> >
> > but it does panic when I try to boot:
> >
> > [ 0.870539] TERM=1inux
> > [ 0.870573] Starting init: /bin/sh exists but couldn't execute it (error -14) 0.8705751 Kernel panic- not syncing: No working init found. Try passing i mit= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance
> > [ 0.870577] CPU: UID: 0 PID: 1 Comm: sh Not tainted 6.18.0-rc6+ #1 PREEMPT(voluntary)
> > [ 0.870579] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBo x 12/01/2006
> > [ 0.870580] Call Trace:
> > [ 0.870590] <TASK>
> > [ 0.870592] vpanic+0x36d/0x380
> > [ 0.870607] ? __pfx_kernel_init+0x10/0x10
> > [ 0.870615] panic+0x5b/0x60
> > [ 0.870617] kernel_init+0x17d/0x1c0
> > [ 0.870623] ret_from_fork+0x124/0x150
> > [ 0.870625} ? __pfx_kernel_init+0x10/0x10
> > [ 0.870627] ret_from_fork_asm+0x1a/0x30
> > [ 0.870632] </TASK>
> > [ 0.8706631 Kernel Offset: 0x3a800000 from Oxffffffff81000000 (relocation ran ge: 0xffffffff80000000-0xffffffffbfffffff)
> > [ 0.880034] ---[ end Kernel panic - not syncing: No working init found. Try passing init option to kernel. See Linux Documentation/admin-guide/init.rst for guidance. 1---`
> >
> >
> > Is that a known problem?
>
> Nope. It looks like the code needs a little bit bug fixing testing.
>
> I will take see about taking a look.
>
> Eric
>
I've also CC'ed the AppArmor mailing list on this patch to facilitate
discussion if, upon further investigation, this patch would require
changes or cause other problems on the AppArmor side.
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Günther Noack @ 2025-11-22 10:16 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-13-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index e22e10edb103..d1a004c2e0f5 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
> }
> }
>
> +static int test_socketpair(int family, int type, int protocol)
> +{
> + int fds[2];
> + int err;
> +
> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
> + if (err)
> + return errno;
> + /*
> + * Mixing error codes from close(2) and socketpair(2) should not lead to
> + * any (access type) confusion for this test.
> + */
> + if (close(fds[0]) != 0)
> + return errno;
> + if (close(fds[1]) != 0)
> + return errno;
Very minor nit: the function leaks an FD if it returns early after the
first close() call failed. (Highly unlikely to happen though.)
> + return 0;
> +}
> +
> +TEST_F(mini, socketpair)
> +{
> + const struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_socket_attr unix_socket_create = {
> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> + .family = AF_UNIX,
> + .type = SOCK_STREAM,
> + .protocol = 0,
> + };
> + int ruleset_fd;
> +
> + /* Tries to create socket when ruleset is not established. */
> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> + ASSERT_LE(0, ruleset_fd);
> +
> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> + &unix_socket_create, 0));
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is allowed */
> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +
> + ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
You may want to check that landlock_create_ruleset() succeeded here:
ASSERT_LE(0, ruleset_fd)
> + enforce_ruleset(_metadata, ruleset_fd);
> + ASSERT_EQ(0, close(ruleset_fd));
> +
> + /* Tries to create socket when protocol is restricted. */
> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
> +}
> +
> TEST_HARNESS_MAIN
> --
> 2.34.1
>
Otherwise, looks good.
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 12/19] selftests/landlock: Test socketpair(2) restriction
From: Mikhail Ivanov @ 2025-11-22 10:21 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.4795c4c3bb03@gnoack.org>
On 11/22/2025 1:16 PM, Günther Noack wrote:
> On Tue, Nov 18, 2025 at 09:46:32PM +0800, Mikhail Ivanov wrote:
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index e22e10edb103..d1a004c2e0f5 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -866,4 +866,59 @@ TEST_F(tcp_protocol, alias_restriction)
>> }
>> }
>>
>> +static int test_socketpair(int family, int type, int protocol)
>> +{
>> + int fds[2];
>> + int err;
>> +
>> + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds);
>> + if (err)
>> + return errno;
>> + /*
>> + * Mixing error codes from close(2) and socketpair(2) should not lead to
>> + * any (access type) confusion for this test.
>> + */
>> + if (close(fds[0]) != 0)
>> + return errno;
>> + if (close(fds[1]) != 0)
>> + return errno;
>
> Very minor nit: the function leaks an FD if it returns early after the
> first close() call failed. (Highly unlikely to happen though.)
Yeah, but AFAIK fd[0] may be leaked anyway if close() fails. Anyway
this shouldn't be an issue for tests.
>
>> + return 0;
>> +}
>> +
>> +TEST_F(mini, socketpair)
>> +{
>> + const struct landlock_ruleset_attr ruleset_attr = {
>> + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + };
>> + const struct landlock_socket_attr unix_socket_create = {
>> + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + .family = AF_UNIX,
>> + .type = SOCK_STREAM,
>> + .protocol = 0,
>> + };
>> + int ruleset_fd;
>> +
>> + /* Tries to create socket when ruleset is not established. */
>> + ASSERT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> + ruleset_fd =
>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> + ASSERT_LE(0, ruleset_fd);
>> +
>> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> + &unix_socket_create, 0));
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is allowed */
>> + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +
>> + ruleset_fd =
>> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>
> You may want to check that landlock_create_ruleset() succeeded here:
>
> ASSERT_LE(0, ruleset_fd)
thanks, I'll fix it.
>
>> + enforce_ruleset(_metadata, ruleset_fd);
>> + ASSERT_EQ(0, close(ruleset_fd));
>> +
>> + /* Tries to create socket when protocol is restricted. */
>> + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0));
>> +}
>> +
>> TEST_HARNESS_MAIN
>> --
>> 2.34.1
>>
>
> Otherwise, looks good.
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Günther Noack @ 2025-11-22 10:49 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-2-ivanov.mikhail1@huawei-partners.com>
Hello!
On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
> It is possible to create sockets of the same protocol with different
> protocol number values. For example, TCP sockets can be created using one
> of the following commands:
> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
> protocol of the given protocol family and can be mapped to another
> value.
>
> Socket rules do not perform such mappings to not increase complexity
> of rules definition and their maintenance.
Minor phrasing nit: Maybe we can phrase this constructively, like
"rules operate on the socket(2) parameters as they are passed by the
user, before this mapping happens"?
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f030adc462ee..030c96cb5d25 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
> * flags`_).
> */
> __u64 handled_access_net;
> + /**
> + * @handled_access_socket: Bitmask of handled actions performed on sockets
> + * (cf. `Socket flags`).
> + */
> + __u64 handled_access_socket;
This struct can only be extended at the end, for ABI compatibility reasons.
In the call to landlock_create_ruleset(2), the user passes the __user
pointer to this struct along with its size (as known to the user at
compile time). When we copy this into the kernel, we blank out the
struct and only copy the prefix of the caller-supplied size. The
implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
When you rearrange the order, please also update it in other places
where these fields are mentioned next to each other, for
consistency. I'll try to point it out where I see it in the review,
but I might miss some places.
> /**
> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
> * restricting a Landlock domain from accessing outside
> @@ -140,6 +145,11 @@ enum landlock_rule_type {
> * landlock_net_port_attr .
> */
> LANDLOCK_RULE_NET_PORT,
> + /**
> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
> + * landlock_socket_attr.
^
Nit: Adjacent documentation has a space before the dot.
I assume this is needed for kernel doc formatting?
> + */
> + LANDLOCK_RULE_SOCKET,
> };
>
> /**
> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
> __u64 port;
> };
>
> +/**
> + * struct landlock_socket_attr - Socket protocol definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_socket_attr {
> + /**
> + * @allowed_access: Bitmask of allowed access for a socket protocol
> + * (cf. `Socket flags`_).
> + */
> + __u64 allowed_access;
> + /**
> + * @family: Protocol family used for communication
> + * (cf. include/linux/socket.h).
> + */
> + __s32 family;
> + /**
> + * @type: Socket type (cf. include/linux/net.h)
> + */
> + __s32 type;
> + /**
> + * @protocol: Communication protocol specific to protocol family set in
> + * @family field.
This is specific to both the @family and the @type, not just the @family.
From socket(2):
Normally only a single protocol exists to support a particular
socket type within a given protocol family.
For instance, in your commit message above the protocol in the example
is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
with SOCK_DGRAM.
> + */
> + __s32 protocol;
> +} __attribute__((packed));
Since we are in the UAPI header, please also document the wildcard
values for @type and @protocol.
(Remark, should those be exposed as constants?)
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 7961c6630a2d..03ccd6fbfe83 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> /* Makes sure all network access rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
> +/* Makes sure all socket access rights can be stored. */
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
> /* Makes sure all scoped rights can be stored. */
> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> struct access_masks {
> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
> access_mask_t scope : LANDLOCK_NUM_SCOPE;
(Please re-adjust field order for consistency with UAPI)
> };
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index dfcdc19ea268..a34d2dbe3954 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> return new_ruleset;
> }
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t fs_access_mask,
> - const access_mask_t net_access_mask,
> - const access_mask_t scope_mask)
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
(Please re-adjust field order for consistency with UAPI)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask && !net_access_mask && !scope_mask)
> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
> + !scope_mask)
(Please re-adjust field order for consistency with UAPI)
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (IS_ERR(new_ruleset))
> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
> if (net_access_mask)
> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> + if (socket_access_mask)
> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
> + 0);
(Please re-adjust order of these "if"s for consistency with UAPI)
> if (scope_mask)
> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
> return new_ruleset;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 1a78cba662b2..a60ede2fc2a5 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -189,10 +204,9 @@ struct landlock_ruleset {
> };
> };
>
> -struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t access_mask_fs,
> - const access_mask_t access_mask_net,
> - const access_mask_t scope_mask);
> +struct landlock_ruleset *landlock_create_ruleset(
> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
(Please re-adjust field order for consistency with UAPI)
> index 000000000000..28a80dcad629
> --- /dev/null
> +++ b/security/landlock/socket.c
> @@ -0,0 +1,105 @@
> [...]
> +#define TYPE_ALL (-1)
> +#define PROTOCOL_ALL (-1)
Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 33eafb71e4f3..e9f500f97c86 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -101,9 +104,10 @@ static void build_check_abi(void)
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> ruleset_size += sizeof(ruleset_attr.handled_access_net);
> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
> ruleset_size += sizeof(ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
> [...]
> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> LANDLOCK_MASK_ACCESS_NET)
> return -EINVAL;
>
> + /* Checks socket content (and 32-bits cast). */
> + if ((ruleset_attr.handled_access_socket |
> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
> + return -EINVAL;
> +
> /* Checks IPC scoping content (and 32-bits cast). */
> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
> return -EINVAL;
> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> /* Checks arguments and transforms to kernel struct. */
> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> ruleset_attr.handled_access_net,
> + ruleset_attr.handled_access_socket,
> ruleset_attr.scoped);
(Please re-adjust field order for consistency with UAPI)
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
> [...]
> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
> * handled accesses)
> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
> + * &landlock_socket_attr.protocol is greater than 65534;
Hmm, this is a bit annoying that these values have such unusual
bounds, even though the input parameters are 32 bit. We are exposing
a little bit that we are internally storing this with only 8 and 16
bits... (I don't know a better solution immediately either, though. I
think we discussed this on a previous version of the patch set as well
and ended up with permitting larger values than the narrower SOCK_MAX
etc bounds.)
> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
> * 0);
> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
> return add_rule_path_beneath(ruleset, rule_attr);
> case LANDLOCK_RULE_NET_PORT:
> return add_rule_net_port(ruleset, rule_attr);
> + case LANDLOCK_RULE_SOCKET:
> + return add_rule_socket(ruleset, rule_attr);
> default:
> return -EINVAL;
> }
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Mikhail Ivanov @ 2025-11-22 11:13 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.e645d2f1b8a1@gnoack.org>
On 11/22/2025 1:49 PM, Günther Noack wrote:
> Hello!
>
> On Tue, Nov 18, 2025 at 09:46:21PM +0800, Mikhail Ivanov wrote:
>> It is possible to create sockets of the same protocol with different
>> protocol number values. For example, TCP sockets can be created using one
>> of the following commands:
>> 1. fd = socket(AF_INET, SOCK_STREAM, 0);
>> 2. fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
>> Whereas IPPROTO_TCP = 6. Protocol number 0 correspond to the default
>> protocol of the given protocol family and can be mapped to another
>> value.
>>
>> Socket rules do not perform such mappings to not increase complexity
>> of rules definition and their maintenance.
>
> Minor phrasing nit: Maybe we can phrase this constructively, like
> "rules operate on the socket(2) parameters as they are passed by the
> user, before this mapping happens"?
OK, thats sounds good.
>
>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f030adc462ee..030c96cb5d25 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -45,6 +45,11 @@ struct landlock_ruleset_attr {
>> * flags`_).
>> */
>> __u64 handled_access_net;
>> + /**
>> + * @handled_access_socket: Bitmask of handled actions performed on sockets
>> + * (cf. `Socket flags`).
>> + */
>> + __u64 handled_access_socket;
>
> This struct can only be extended at the end, for ABI compatibility reasons.
>
> In the call to landlock_create_ruleset(2), the user passes the __user
> pointer to this struct along with its size (as known to the user at
> compile time). When we copy this into the kernel, we blank out the
> struct and only copy the prefix of the caller-supplied size. The
> implementation is in copy_min_struct_from_user() in landlock/syscalls.c.
Indeed... Thanks for pointing on this, I'll move this field in the end
of the structure.
>
> When you rearrange the order, please also update it in other places
> where these fields are mentioned next to each other, for
> consistency. I'll try to point it out where I see it in the review,
> but I might miss some places.
ok
>
>> /**
>> * @scoped: Bitmask of scopes (cf. `Scope flags`_)
>> * restricting a Landlock domain from accessing outside
>> @@ -140,6 +145,11 @@ enum landlock_rule_type {
>> * landlock_net_port_attr .
>> */
>> LANDLOCK_RULE_NET_PORT,
>> + /**
>> + * @LANDLOCK_RULE_SOCKET: Type of a &struct
>> + * landlock_socket_attr.
> ^
>
> Nit: Adjacent documentation has a space before the dot.
> I assume this is needed for kernel doc formatting?
Probably, I'll fix this anyway.
>
>> + */
>> + LANDLOCK_RULE_SOCKET,
>> };
>>
>> /**
>> @@ -191,6 +201,33 @@ struct landlock_net_port_attr {
>> __u64 port;
>> };
>>
>> +/**
>> + * struct landlock_socket_attr - Socket protocol definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_socket_attr {
>> + /**
>> + * @allowed_access: Bitmask of allowed access for a socket protocol
>> + * (cf. `Socket flags`_).
>> + */
>> + __u64 allowed_access;
>> + /**
>> + * @family: Protocol family used for communication
>> + * (cf. include/linux/socket.h).
>> + */
>> + __s32 family;
>> + /**
>> + * @type: Socket type (cf. include/linux/net.h)
>> + */
>> + __s32 type;
>> + /**
>> + * @protocol: Communication protocol specific to protocol family set in
>> + * @family field.
>
> This is specific to both the @family and the @type, not just the @family.
>
>>From socket(2):
>
> Normally only a single protocol exists to support a particular
> socket type within a given protocol family.
>
> For instance, in your commit message above the protocol in the example
> is IPPROTO_TCP, which would imply the type SOCK_STREAM, but not work
> with SOCK_DGRAM.
You're right.
>
>> + */
>> + __s32 protocol;
>> +} __attribute__((packed));
>
> Since we are in the UAPI header, please also document the wildcard
> values for @type and @protocol.
I'll add the description, thanks!
>
> (Remark, should those be exposed as constants?)
I thought it could overcomplicate socket rules definition and Landlock
API. Do you think introducing such constants will be better decision?
>
>
>> diff --git a/security/landlock/access.h b/security/landlock/access.h
>> index 7961c6630a2d..03ccd6fbfe83 100644
>> --- a/security/landlock/access.h
>> +++ b/security/landlock/access.h
>> @@ -40,6 +40,8 @@ typedef u16 access_mask_t;
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> /* Makes sure all network access rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>> +/* Makes sure all socket access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_SOCKET);
>> /* Makes sure all scoped rights can be stored. */
>> static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_SCOPE);
>> /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>> @@ -49,6 +51,7 @@ static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> struct access_masks {
>> access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
>> access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
>> + access_mask_t socket : LANDLOCK_NUM_ACCESS_SOCKET;
>> access_mask_t scope : LANDLOCK_NUM_SCOPE;
>
> (Please re-adjust field order for consistency with UAPI)
ok, will be fixed in all such places.
>
>> };
>
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index dfcdc19ea268..a34d2dbe3954 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -55,15 +56,15 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>> return new_ruleset;
>> }
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask,
>> - const access_mask_t net_access_mask,
>> - const access_mask_t scope_mask)
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t fs_access_mask, const access_mask_t net_access_mask,
>> + const access_mask_t socket_access_mask, const access_mask_t scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> {
>> struct landlock_ruleset *new_ruleset;
>>
>> /* Informs about useless ruleset. */
>> - if (!fs_access_mask && !net_access_mask && !scope_mask)
>> + if (!fs_access_mask && !net_access_mask && !socket_access_mask &&
>> + !scope_mask)
>
> (Please re-adjust field order for consistency with UAPI)
>
>> return ERR_PTR(-ENOMSG);
>> new_ruleset = create_ruleset(1);
>> if (IS_ERR(new_ruleset))
>> @@ -72,6 +73,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>> landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>> if (net_access_mask)
>> landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> + if (socket_access_mask)
>> + landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
>> + 0);
>
> (Please re-adjust order of these "if"s for consistency with UAPI)
>
>> if (scope_mask)
>> landlock_add_scope_mask(new_ruleset, scope_mask, 0);
>> return new_ruleset;
>
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index 1a78cba662b2..a60ede2fc2a5 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -189,10 +204,9 @@ struct landlock_ruleset {
>> };
>> };
>>
>> -struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t access_mask_fs,
>> - const access_mask_t access_mask_net,
>> - const access_mask_t scope_mask);
>> +struct landlock_ruleset *landlock_create_ruleset(
>> + const access_mask_t access_mask_fs, const access_mask_t access_mask_net,
>> + const access_mask_t access_mask_socket, const access_mask_t scope_mask);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> index 000000000000..28a80dcad629
>> --- /dev/null
>> +++ b/security/landlock/socket.c
>> @@ -0,0 +1,105 @@
>> [...]
>> +#define TYPE_ALL (-1)
>> +#define PROTOCOL_ALL (-1)
>
> Should these definitions go into the UAPI header (with a LANDLOCK_ prefix)?
answered above.
>
>
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 33eafb71e4f3..e9f500f97c86 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -101,9 +104,10 @@ static void build_check_abi(void)
>> */
>> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>> ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> + ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>> ruleset_size += sizeof(ruleset_attr.scoped);
> (Please re-adjust field order for consistency with UAPI)
>
>> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> - BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>> + BUILD_BUG_ON(sizeof(ruleset_attr) != 32);
>> [...]
>
>> @@ -237,6 +248,11 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> LANDLOCK_MASK_ACCESS_NET)
>> return -EINVAL;
>>
>> + /* Checks socket content (and 32-bits cast). */
>> + if ((ruleset_attr.handled_access_socket |
>> + LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
>> + return -EINVAL;
>> +
>> /* Checks IPC scoping content (and 32-bits cast). */
>> if ((ruleset_attr.scoped | LANDLOCK_MASK_SCOPE) != LANDLOCK_MASK_SCOPE)
>> return -EINVAL;
>> @@ -244,6 +260,7 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>> /* Checks arguments and transforms to kernel struct. */
>> ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>> ruleset_attr.handled_access_net,
>> + ruleset_attr.handled_access_socket,
>> ruleset_attr.scoped);
>
> (Please re-adjust field order for consistency with UAPI)
>
>> if (IS_ERR(ruleset))
>> return PTR_ERR(ruleset);
>> [...]
>
>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>> * handled accesses)
>> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>> + * &landlock_socket_attr.protocol is greater than 65534;
>
> Hmm, this is a bit annoying that these values have such unusual
> bounds, even though the input parameters are 32 bit. We are exposing
> a little bit that we are internally storing this with only 8 and 16
> bits... (I don't know a better solution immediately either, though. I
> think we discussed this on a previous version of the patch set as well
> and ended up with permitting larger values than the narrower SOCK_MAX
> etc bounds.)
I agree, one of the possible solutions may be to store larger values in
socket keys (eg. s32), but this would require to make a separate
interface for storing socket rules (in order to not change key size for
other type of rules which is currently 32-64 bit depending on virtual
address size).
>
>> * - %ENOMSG: Empty accesses (e.g. &landlock_path_beneath_attr.allowed_access is
>> * 0);
>> * - %EBADF: @ruleset_fd is not a file descriptor for the current thread, or a
>> @@ -439,6 +492,8 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>> return add_rule_path_beneath(ruleset, rule_attr);
>> case LANDLOCK_RULE_NET_PORT:
>> return add_rule_net_port(ruleset, rule_attr);
>> + case LANDLOCK_RULE_SOCKET:
>> + return add_rule_socket(ruleset, rule_attr);
>> default:
>> return -EINVAL;
>> }
>
> –Günther
^ permalink raw reply
* Re: [RFC PATCH v4 06/19] landlock: Add hook on socket creation
From: Günther Noack @ 2025-11-22 11:41 UTC (permalink / raw)
To: Mikhail Ivanov
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251118134639.3314803-7-ivanov.mikhail1@huawei-partners.com>
On Tue, Nov 18, 2025 at 09:46:26PM +0800, Mikhail Ivanov wrote:
> Add hook on security_socket_create(), which checks whether the socket
> of requested protocol is allowed by domain.
>
> Due to support of masked protocols Landlock tries to find one of the
> 4 rules that can allow creation of requested protocol.
>
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> Changes since v3:
> * Changes LSM hook from socket_post_create to socket_create so
> creation would be blocked before socket allocation and initialization.
> * Uses credential instead of domain in hook_socket create.
> * Removes get_raw_handled_socket_accesses.
> * Adds checks for rules with wildcard type and protocol values.
> * Minor refactoring, fixes.
>
> Changes since v2:
> * Adds check in `hook_socket_create()` to not restrict kernel space
> sockets.
> * Inlines `current_check_access_socket()` in the `hook_socket_create()`.
> * Fixes commit message.
>
> Changes since v1:
> * Uses lsm hook arguments instead of struct socket fields as family-type
> values.
> * Packs socket family and type using helper.
> * Fixes commit message.
> * Formats with clang-format.
> ---
> security/landlock/setup.c | 2 +
> security/landlock/socket.c | 78 ++++++++++++++++++++++++++++++++++++++
> security/landlock/socket.h | 2 +
> 3 files changed, 82 insertions(+)
>
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index bd53c7a56ab9..140a53b022f7 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -17,6 +17,7 @@
> #include "fs.h"
> #include "id.h"
> #include "net.h"
> +#include "socket.h"
> #include "setup.h"
> #include "task.h"
>
> @@ -68,6 +69,7 @@ static int __init landlock_init(void)
> landlock_add_task_hooks();
> landlock_add_fs_hooks();
> landlock_add_net_hooks();
> + landlock_add_socket_hooks();
> landlock_init_id();
> landlock_initialized = true;
> pr_info("Up and running.\n");
> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
> index 28a80dcad629..d7e6e7b92b7a 100644
> --- a/security/landlock/socket.c
> +++ b/security/landlock/socket.c
> @@ -103,3 +103,81 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>
> return err;
> }
> +
> +static int check_socket_access(const struct landlock_ruleset *dom,
> + uintptr_t key,
> + layer_mask_t (*const layer_masks)[],
> + access_mask_t handled_access)
> +{
> + const struct landlock_rule *rule;
> + struct landlock_id id = {
> + .type = LANDLOCK_KEY_SOCKET,
> + };
> +
> + id.key.data = key;
This line can be made part of the designated initializer:
struct landlock_id id = {
.type = ...,
.key.data = ...,
};
> + rule = landlock_find_rule(dom, id);
> + if (landlock_unmask_layers(rule, handled_access, layer_masks,
> + LANDLOCK_NUM_ACCESS_SOCKET))
> + return 0;
> + return -EACCES;
> +}
> +
> +static int hook_socket_create(int family, int type, int protocol, int kern)
> +{
> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
> + access_mask_t handled_access;
> + const struct access_masks masks = {
> + .socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> + };
> + const struct landlock_cred_security *const subject =
> + landlock_get_applicable_subject(current_cred(), masks, NULL);
> + uintptr_t key;
> +
> + if (!subject)
> + return 0;
> + /* Checks only user space sockets. */
> + if (kern)
> + return 0;
> +
> + handled_access = landlock_init_layer_masks(
> + subject->domain, LANDLOCK_ACCESS_SOCKET_CREATE, &layer_masks,
> + LANDLOCK_KEY_SOCKET);
Nit: I had to double check to confirm that the same PF_INET/PF_PACKET
transformation (which net/socket.c refers to as the "uglymoron") has
already happened on the arguments before hook_socket_create() gets
called from there. Maybe it's worth a brief mention in a comment
here.
> + /*
> + * Error could happen due to parameters are outside of the allowed range,
Grammar nit: drop the "are"
Suggestion: "If this error happens, the parameters are outside of the
allowed range, so this combination can't have been added to the
ruleset previously."
> + * so this combination couldn't be added in ruleset previously.
> + * Therefore, it's not permitted.
> + */
> + if (pack_socket_key(family, type, protocol, &key) == -EACCES)
> + return -EACCES;
BUG: pack_socket_key() does never return -EACCES!
(Consider whether that function should really return an error? Maybe
a boolean would be better, if you anyway need a different error code
in both locations where it is called.)
Can this code path actually get hit, or do the entry points for
creating sockets refuse these wrong values at an earlier stage with
EINVAL already?
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + /* Ranges were already checked. */
> + (void)pack_socket_key(family, TYPE_ALL, protocol, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, type, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + (void)pack_socket_key(family, TYPE_ALL, PROTOCOL_ALL, &key);
> + if (check_socket_access(subject->domain, key, &layer_masks,
> + handled_access) == 0)
> + return 0;
> +
> + return -EACCES;
> +}
It initially doesn't look very nice to drop the error from
pack_socket_key() repeatedly. The call repeats the bounds checks and
requires more cross-function reasoning to understand.
Since 'key' is an uintptr_t anyway, and the wildcards are all ones,
maybe a simpler way is to define masks for the wildcards?
const uintptr_t any_type_mask = (union key){.data.type = UINT8_MAX}.packed;
const uintptr_t any_protocol_mask = (union key){.data.protocol = UINT16_MAX}.packed;
and then, after calling pack_socket_key() once with error check, use
the combinations
* key
* key | any_type
* key | any_protocol
* key | any_type | any_protocol
to construct the wildcard-enabled keys in the four calls to
check_socket_access()? You could have compile-time assertions or
tests to check that the masking does the same as packing it from
scratch when passing -1.
(That being said, I don't feel strongly about it.)
Remark on the side: I was briefly confused why we don't need to guard
on CONFIG_SECURITY_NETWORK, but this is already required by
CONFIG_LANDLOCK. So that looks good.
–Günther
^ permalink raw reply
* [PATCH RESEND] apparmor: replace sprintf with snprintf in aa_new_learning_profile
From: Thorsten Blum @ 2025-11-22 11:54 UTC (permalink / raw)
To: John Johansen, Paul Moore, James Morris, Serge E. Hallyn
Cc: Thorsten Blum, apparmor, linux-security-module, linux-kernel
Replace unbounded sprintf() calls with snprintf() to prevent potential
buffer overflows in aa_new_learning_profile(). While the current code
works correctly, snprintf() is safer and follows secure coding best
practices. No functional changes.
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/policy.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 50d5345ff5cb..b09323867fea 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -697,24 +697,27 @@ struct aa_profile *aa_new_learning_profile(struct aa_profile *parent, bool hat,
struct aa_profile *p, *profile;
const char *bname;
char *name = NULL;
+ size_t name_sz;
AA_BUG(!parent);
if (base) {
- name = kmalloc(strlen(parent->base.hname) + 8 + strlen(base),
- gfp);
+ name_sz = strlen(parent->base.hname) + 8 + strlen(base);
+ name = kmalloc(name_sz, gfp);
if (name) {
- sprintf(name, "%s//null-%s", parent->base.hname, base);
+ snprintf(name, name_sz, "%s//null-%s",
+ parent->base.hname, base);
goto name;
}
/* fall through to try shorter uniq */
}
- name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, gfp);
+ name_sz = strlen(parent->base.hname) + 2 + 7 + 8;
+ name = kmalloc(name_sz, gfp);
if (!name)
return NULL;
- sprintf(name, "%s//null-%x", parent->base.hname,
- atomic_inc_return(&parent->ns->uniq_null));
+ snprintf(name, name_sz, "%s//null-%x", parent->base.hname,
+ atomic_inc_return(&parent->ns->uniq_null));
name:
/* lookup to see if this is a dup creation */
--
2.51.1
^ permalink raw reply related
* [PATCH RESEND] apparmor: Replace sprintf/strcpy with scnprintf/strscpy in aa_policy_init
From: Thorsten Blum @ 2025-11-22 11:55 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 and sprintf() does not perform bounds checking
either. Although an overflow is unlikely, it's better to proactively
avoid it by using the safer strscpy() and scnprintf(), respectively.
Additionally, unify memory allocation for 'hname' to simplify and
improve aa_policy_init().
Link: https://github.com/KSPP/linux/issues/88
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
security/apparmor/lib.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c
index 82dbb97ad406..acf7f5189bec 100644
--- a/security/apparmor/lib.c
+++ b/security/apparmor/lib.c
@@ -478,19 +478,17 @@ bool aa_policy_init(struct aa_policy *policy, const char *prefix,
const char *name, gfp_t gfp)
{
char *hname;
+ size_t hname_sz;
+ hname_sz = (prefix ? strlen(prefix) + 2 : 0) + strlen(name) + 1;
/* freed by policy_free */
- if (prefix) {
- hname = aa_str_alloc(strlen(prefix) + strlen(name) + 3, gfp);
- if (hname)
- sprintf(hname, "%s//%s", prefix, name);
- } else {
- hname = aa_str_alloc(strlen(name) + 1, gfp);
- if (hname)
- strcpy(hname, name);
- }
+ hname = aa_str_alloc(hname_sz, gfp);
if (!hname)
return false;
+ if (prefix)
+ scnprintf(hname, hname_sz, "%s//%s", prefix, name);
+ else
+ strscpy(hname, name, hname_sz);
policy->hname = hname;
/* base.name is a substring of fqname */
policy->name = basename(policy->hname);
--
2.51.1
^ permalink raw reply related
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Günther Noack @ 2025-11-22 12:18 UTC (permalink / raw)
To: Mikhail Ivanov, mic
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <af464773-b01b-f3a4-474d-0efb2cfae142@huawei-partners.com>
On Sat, Nov 22, 2025 at 02:13:08PM +0300, Mikhail Ivanov wrote:
> On 11/22/2025 1:49 PM, Günther Noack wrote:
> > (Remark, should those be exposed as constants?)
>
> I thought it could overcomplicate socket rules definition and Landlock
> API. Do you think introducing such constants will be better decision?
No, I am not convinced either. FWIW, there is a bit of prior art for
"wildcard-like" -1 constants (grepping include/uapi for 'define.*-1'),
but then again, the places where people did the opposite are hard to
grep for. I would also be OK if we documented "-1" in that place and
left out the constant.
Mickaël, maybe you have a preference for the API style here?
> > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > > index 33eafb71e4f3..e9f500f97c86 100644
> > > --- a/security/landlock/syscalls.c
> > > +++ b/security/landlock/syscalls.c
> > > @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
> > > * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
> > > * handled accesses)
> > > * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
> > > + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
> > > + * &landlock_socket_attr.protocol is greater than 65534;
> >
> > Hmm, this is a bit annoying that these values have such unusual
> > bounds, even though the input parameters are 32 bit. We are exposing
> > a little bit that we are internally storing this with only 8 and 16
> > bits... (I don't know a better solution immediately either, though. I
> > think we discussed this on a previous version of the patch set as well
> > and ended up with permitting larger values than the narrower SOCK_MAX
> > etc bounds.)
>
> I agree, one of the possible solutions may be to store larger values in
> socket keys (eg. s32), but this would require to make a separate
> interface for storing socket rules (in order to not change key size for
> other type of rules which is currently 32-64 bit depending on virtual
> address size).
Yes, I'd be OK with it.
Do I remember this correctly that we settled on enforcing the looser
UINT8_MAX and UINT16_MAX instead of SOCK_MAX, AF_MAX, which we used in
v3 and before? I tried to find the conversation but could not find it
any more. (Or did you have other reasons why you switched the
implementation to use these larger bounds?)
Thanks,
–Günther
^ permalink raw reply
* Re: [RFC PATCH v4 01/19] landlock: Support socket access-control
From: Mikhail Ivanov @ 2025-11-22 16:51 UTC (permalink / raw)
To: Günther Noack, mic
Cc: gnoack, willemdebruijn.kernel, matthieu, linux-security-module,
netdev, netfilter-devel, yusongping, artem.kuzin,
konstantin.meskhidze
In-Reply-To: <20251122.d391a246d7dd@gnoack.org>
On 11/22/2025 3:18 PM, Günther Noack wrote:
> On Sat, Nov 22, 2025 at 02:13:08PM +0300, Mikhail Ivanov wrote:
>> On 11/22/2025 1:49 PM, Günther Noack wrote:
>>> (Remark, should those be exposed as constants?)
>>
>> I thought it could overcomplicate socket rules definition and Landlock
>> API. Do you think introducing such constants will be better decision?
>
> No, I am not convinced either. FWIW, there is a bit of prior art for
> "wildcard-like" -1 constants (grepping include/uapi for 'define.*-1'),
> but then again, the places where people did the opposite are hard to
> grep for. I would also be OK if we documented "-1" in that place and
> left out the constant.
>
> Mickaël, maybe you have a preference for the API style here?
>
>
>>>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>>>> index 33eafb71e4f3..e9f500f97c86 100644
>>>> --- a/security/landlock/syscalls.c
>>>> +++ b/security/landlock/syscalls.c
>>>> @@ -407,6 +458,8 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>>>> * &landlock_net_port_attr.allowed_access is not a subset of the ruleset
>>>> * handled accesses)
>>>> * - %EINVAL: &landlock_net_port_attr.port is greater than 65535;
>>>> + * - %EINVAL: &landlock_socket_attr.{family, type} are greater than 254 or
>>>> + * &landlock_socket_attr.protocol is greater than 65534;
>>>
>>> Hmm, this is a bit annoying that these values have such unusual
>>> bounds, even though the input parameters are 32 bit. We are exposing
>>> a little bit that we are internally storing this with only 8 and 16
>>> bits... (I don't know a better solution immediately either, though. I
>>> think we discussed this on a previous version of the patch set as well
>>> and ended up with permitting larger values than the narrower SOCK_MAX
>>> etc bounds.)
>>
>> I agree, one of the possible solutions may be to store larger values in
>> socket keys (eg. s32), but this would require to make a separate
>> interface for storing socket rules (in order to not change key size for
>> other type of rules which is currently 32-64 bit depending on virtual
>> address size).
>
> Yes, I'd be OK with it.
>
> Do I remember this correctly that we settled on enforcing the looser
> UINT8_MAX and UINT16_MAX instead of SOCK_MAX, AF_MAX, which we used in
> v3 and before? I tried to find the conversation but could not find it
> any more. (Or did you have other reasons why you switched the
> implementation to use these larger bounds?)
Mickaël mentioned that Landlock should accept rules defined even for
unsupported protocol families:
https://lore.kernel.org/all/20241128.um9voo5Woo3I@digikod.net/
>
> Thanks,
> –Günther
^ permalink raw reply
* Re: [PATCH v18] exec: Fix dead-lock in de_thread with ptrace_attach
From: Bernd Edlinger @ 2025-11-22 17:10 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Alexander Viro, Alexey Dobriyan, Oleg Nesterov, Kees Cook,
Andy Lutomirski, Will Drewry, Christian Brauner, Andrew Morton,
Michal Hocko, Serge Hallyn, James Morris, Randy Dunlap,
Suren Baghdasaryan, Yafang Shao, Helge Deller, Adrian Reber,
Thomas Gleixner, Jens Axboe, Alexei Starovoitov,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kselftest, linux-mm, linux-security-module, tiozhang,
Luis Chamberlain, Paulo Alcantara (SUSE), Sergey Senozhatsky,
Frederic Weisbecker, YueHaibing, Paul Moore, Aleksa Sarai,
Stefan Roesch, Chao Yu, xu xin, Jeff Layton, Jan Kara,
David Hildenbrand, Dave Chinner, Shuah Khan, Elena Reshetova,
David Windsor, Mateusz Guzik, Ard Biesheuvel,
Joel Fernandes (Google), Matthew Wilcox (Oracle),
Hans Liljestrand, Penglei Jiang, Lorenzo Stoakes, Adrian Ratiu,
Ingo Molnar, Peter Zijlstra (Intel), Cyrill Gorcunov,
Eric Dumazet, Ryan Lee, apparmor, selinux
In-Reply-To: <87wm3ky5n9.fsf@email.froward.int.ebiederm.org>
On 11/20/25 18:29, Eric W. Biederman wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>
>> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
>>
>>> This introduces signal->exec_bprm, which is used to
>>> fix the case when at least one of the sibling threads
>>> is traced, and therefore the trace process may dead-lock
>>> in ptrace_attach, but de_thread will need to wait for the
>>> tracer to continue execution.
>>
>> A small quibble it isn't a dead lock. It isn't even really a live lock,
>> as it is possible to SIGKILL our way out.
>>
That is of course what I meant to say with that sentence. In my language an
application process can "dead-lock" by waiting on a mutex forever.
Indeed the original problem with the dead-lock (I think in mm_access) was
worse, as both involved processes were only killable by "kill -9", but with the
remaining problem in ptrace_attach, the debugger can be killed by a simple CTRL-C.
So if I understand you right, you want me use a different term here like
"the trace process may be blocked in trace_attach" or so?
Or could you please give me a concrete suggestion how to rephrase the patch
description.
BTW, unless there are objections I would also want to rephrase the description
of cred_guard_mutex to replace the term "Deprecated" with "Not recommended",
like this:
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -260,11 +260,11 @@ struct signal_struct {
struct mutex cred_guard_mutex; /* guard against foreign influences on
* credential calculations
* (notably. ptrace)
* Held while execve runs, except when
* a sibling thread is being traced.
- * Deprecated do not use in new code.
+ * Not recommended to use in new code.
* Use exec_update_lock instead.
*/
>> Thinking about this there is a really silly and simple way we can deal
>> with this situation for PTRACE_ATTACH. We can send SIGSTOP and wait for
>> the thread to stop before doing anything with cred_guard_mutex.
>>
>> PTRACE_ATTACH already implies sending SIGSTOP so as long as we have
>> enough permissions to send SIGSTOP I don't see that being a problem.
>>
>> The worst case I can see is that we get a case where we stop the
>> process, the permission check fails under cred_guard_mutex and
>> and ptrace attach has fails and has to send SIGCONT to undo it's
>> premature SIGSTOP. That might almost be visible, but it would still
>> be legitimate because we can still check that we have permission to
>> send SIGSTOP.
>
> Bah no I am full of it.
>
> The challenging behavior is in the semantics of the kernel operations.
> We need to describe it as such please.
>
> It is the same class of problem as a single threaded process calls exec
> with a pipe attached to both stdin and stdout of the new process.
>
> For the stdin and stdout we can say just use pull and nonblocking I/O.
>
> The problem is that both PTRACE_ATTACH and PTRACE_SEIZE block over
> the duration of exec, and if exec is waiting for a thread to exit,
> and that thread is blocked in PTRACE_EVENT_EXIT waiting for that very
> same tracer those processes will hang. Not deadlock.
>
>
> I haven't seen anyone clearly describe the problem lately so I am
> repeating it.
>
>
> Just looking at the code I don't think there is any fundamental reason
> to call commit_creds after de_thread. If we can change that we can sort
> this out without any change in userspace semantics.
>
> If we can't move commit_creds we have to either give
> PTRACE_ATTACH/PTRACE_SEIZE a non-block mode, or break out of
> PTRACE_EVENT_EXIT in de_thread.
>
> I will post a proof of concept of moving commit_creds in just a minute.
>
> Eric
Note: I forgot to add apparmor and selinux mailing list to this patch, previous
versions of this did try to avoid to touch the security engine code, and did
instead temporarily install the new credentials, mostiy for the benefit of the
security engines. But that is considered an unacceptable solution, therefore
I want to use instead a new option to ptrace_may_access.
All security engines have to handle this option, but the advantage is, that the
engines could detect and maybe also deny the unsafe execve.
This is an alternative to Eric's patch: "exec: Move cred computation under
exec_update_lock" that is supposed to solve the same problem, but tries instead
to avoid any user visible API change.
Thanks
Bernd.
^ permalink raw reply
* Re: [RFC PATCH v4 06/19] landlock: Add hook on socket creation
From: Mikhail Ivanov @ 2025-11-22 17:19 UTC (permalink / raw)
To: Günther Noack
Cc: mic, gnoack, willemdebruijn.kernel, matthieu,
linux-security-module, netdev, netfilter-devel, yusongping,
artem.kuzin, konstantin.meskhidze
In-Reply-To: <20251122.78c6cd69a873@gnoack.org>
On 11/22/2025 2:41 PM, Günther Noack wrote:
> On Tue, Nov 18, 2025 at 09:46:26PM +0800, Mikhail Ivanov wrote:
>> Add hook on security_socket_create(), which checks whether the socket
>> of requested protocol is allowed by domain.
>>
>> Due to support of masked protocols Landlock tries to find one of the
>> 4 rules that can allow creation of requested protocol.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>> Changes since v3:
>> * Changes LSM hook from socket_post_create to socket_create so
>> creation would be blocked before socket allocation and initialization.
>> * Uses credential instead of domain in hook_socket create.
>> * Removes get_raw_handled_socket_accesses.
>> * Adds checks for rules with wildcard type and protocol values.
>> * Minor refactoring, fixes.
>>
>> Changes since v2:
>> * Adds check in `hook_socket_create()` to not restrict kernel space
>> sockets.
>> * Inlines `current_check_access_socket()` in the `hook_socket_create()`.
>> * Fixes commit message.
>>
>> Changes since v1:
>> * Uses lsm hook arguments instead of struct socket fields as family-type
>> values.
>> * Packs socket family and type using helper.
>> * Fixes commit message.
>> * Formats with clang-format.
>> ---
>> security/landlock/setup.c | 2 +
>> security/landlock/socket.c | 78 ++++++++++++++++++++++++++++++++++++++
>> security/landlock/socket.h | 2 +
>> 3 files changed, 82 insertions(+)
>>
>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
>> index bd53c7a56ab9..140a53b022f7 100644
>> --- a/security/landlock/setup.c
>> +++ b/security/landlock/setup.c
>> @@ -17,6 +17,7 @@
>> #include "fs.h"
>> #include "id.h"
>> #include "net.h"
>> +#include "socket.h"
>> #include "setup.h"
>> #include "task.h"
>>
>> @@ -68,6 +69,7 @@ static int __init landlock_init(void)
>> landlock_add_task_hooks();
>> landlock_add_fs_hooks();
>> landlock_add_net_hooks();
>> + landlock_add_socket_hooks();
>> landlock_init_id();
>> landlock_initialized = true;
>> pr_info("Up and running.\n");
>> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
>> index 28a80dcad629..d7e6e7b92b7a 100644
>> --- a/security/landlock/socket.c
>> +++ b/security/landlock/socket.c
>> @@ -103,3 +103,81 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>>
>> return err;
>> }
>> +
>> +static int check_socket_access(const struct landlock_ruleset *dom,
>> + uintptr_t key,
>> + layer_mask_t (*const layer_masks)[],
>> + access_mask_t handled_access)
>> +{
>> + const struct landlock_rule *rule;
>> + struct landlock_id id = {
>> + .type = LANDLOCK_KEY_SOCKET,
>> + };
>> +
>> + id.key.data = key;
>
> This line can be made part of the designated initializer:
>
> struct landlock_id id = {
> .type = ...,
> .key.data = ...,
> };
>
Indeed, thats would be better.
>
>> + rule = landlock_find_rule(dom, id);
>> + if (landlock_unmask_layers(rule, handled_access, layer_masks,
>> + LANDLOCK_NUM_ACCESS_SOCKET))
>> + return 0;
>> + return -EACCES;
>> +}
>> +
>> +static int hook_socket_create(int family, int type, int protocol, int kern)
>> +{
>> + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
>> + access_mask_t handled_access;
>> + const struct access_masks masks = {
>> + .socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> + };
>> + const struct landlock_cred_security *const subject =
>> + landlock_get_applicable_subject(current_cred(), masks, NULL);
>> + uintptr_t key;
>> +
>> + if (!subject)
>> + return 0;
>> + /* Checks only user space sockets. */
>> + if (kern)
>> + return 0;
>> +
>> + handled_access = landlock_init_layer_masks(
>> + subject->domain, LANDLOCK_ACCESS_SOCKET_CREATE, &layer_masks,
>> + LANDLOCK_KEY_SOCKET);
>
> Nit: I had to double check to confirm that the same PF_INET/PF_PACKET
> transformation (which net/socket.c refers to as the "uglymoron") has
> already happened on the arguments before hook_socket_create() gets
> called from there. Maybe it's worth a brief mention in a comment
> here.
Ok, thanks!
>
>> + /*
>> + * Error could happen due to parameters are outside of the allowed range,
>
> Grammar nit: drop the "are"
>
> Suggestion: "If this error happens, the parameters are outside of the
> allowed range, so this combination can't have been added to the
> ruleset previously."
Thanks, I'll use it.
>
>> + * so this combination couldn't be added in ruleset previously.
>> + * Therefore, it's not permitted.
>> + */
>> + if (pack_socket_key(family, type, protocol, &key) == -EACCES)
>> + return -EACCES;
>
> BUG: pack_socket_key() does never return -EACCES!
Thanks a lot, will be fixed!
>
> (Consider whether that function should really return an error? Maybe
> a boolean would be better, if you anyway need a different error code
> in both locations where it is called.)
Agreed
>
> Can this code path actually get hit, or do the entry points for
> creating sockets refuse these wrong values at an earlier stage with
> EINVAL already?
There are checks for family and type ranges in __sock_create. Protocol
ranges should be checked in methods specific to protocol family after
LSM hook is triggered. But it would be safer to keep this check in order
to be independent of the specific kernel version.
>
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + /* Ranges were already checked. */
>> + (void)pack_socket_key(family, TYPE_ALL, protocol, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + (void)pack_socket_key(family, type, PROTOCOL_ALL, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + (void)pack_socket_key(family, TYPE_ALL, PROTOCOL_ALL, &key);
>> + if (check_socket_access(subject->domain, key, &layer_masks,
>> + handled_access) == 0)
>> + return 0;
>> +
>> + return -EACCES;
>> +}
>
> It initially doesn't look very nice to drop the error from
> pack_socket_key() repeatedly. The call repeats the bounds checks and
> requires more cross-function reasoning to understand.
Agreed
>
> Since 'key' is an uintptr_t anyway, and the wildcards are all ones,
> maybe a simpler way is to define masks for the wildcards?
>
> const uintptr_t any_type_mask = (union key){.data.type = UINT8_MAX}.packed;
> const uintptr_t any_protocol_mask = (union key){.data.protocol = UINT16_MAX}.packed;
>
> and then, after calling pack_socket_key() once with error check, use
> the combinations
>
> * key
> * key | any_type
> * key | any_protocol
> * key | any_type | any_protocol
>
> to construct the wildcard-enabled keys in the four calls to
> check_socket_access()? You could have compile-time assertions or
> tests to check that the masking does the same as packing it from
> scratch when passing -1.
>
> (That being said, I don't feel strongly about it.)
It seems clearer and simpler to me, so I think we should use your
approach. Probably, pack_socket_key() should be changed to pack values
using bit operations instead of socket_key structure:
key = protocol << 16 | type << 8 | family;
>
> Remark on the side: I was briefly confused why we don't need to guard
> on CONFIG_SECURITY_NETWORK, but this is already required by
> CONFIG_LANDLOCK. So that looks good.
>
> –Günther
^ permalink raw reply
* Re: [GIT PULL] selinux/selinux-pr-20251121
From: pr-tracker-bot @ 2025-11-22 18:29 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <e6a2c26951147ce12c57ff1fd53651c0@paul-moore.com>
The pull request you sent on Fri, 21 Nov 2025 15:57:27 -0500:
> https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20251121
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5703357ede59ce8b0af11e02c374a3db73c55ee8
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* Re: [PATCH 5/6] landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is disabled.
From: Tingmao Wang @ 2025-11-22 23:35 UTC (permalink / raw)
To: Justin Suess
Cc: Günther Noack, Jan Kara, Abhinav Saxena,
Mickaël Salaün, linux-security-module
In-Reply-To: <20251120222346.1157004-6-utilityemal77@gmail.com>
On 11/20/25 22:23, Justin Suess wrote:
> This was necessary when fixing the no inherit patch in the implementation of kunit tests.
>
> When compiled without the audit flag enabled, kunit tests would fail to compile because of
> the missing quiet flag field. This fixes this issue.
>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
Thanks for spotting these issues! I will include your fix in the next
version of my patch (it's not merged yet, so mistakes can be fixed "at
source"). Feel free to reply to my patch directly if you spot more
problems :)
(For the landlock_log_denial one, we might not end up changing the
function's parameter after all - Mickaël has suggested the rule_flags be
embedded inside the landlock_request)
^ permalink raw reply
* Re: [PATCH 0/2] apparmor unaligned memory fixes
From: John Johansen @ 2025-11-23 2:08 UTC (permalink / raw)
To: Helge Deller, John Paul Adrian Glaubitz, Helge Deller
Cc: linux-kernel, apparmor, linux-security-module
In-Reply-To: <be9c143d-1d5e-4c5b-9078-4a7804489258@gmx.de>
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.
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.
^ permalink raw reply
* Re: [PATCH 5/6] landlock: Fix compilation error for kunit tests when CONFIG_AUDIT is disabled.
From: Justin Suess @ 2025-11-23 16:43 UTC (permalink / raw)
To: m; +Cc: gnoack, jack, linux-security-module, mic, utilityemal77, xandfury
In-Reply-To: <baa3449e-e238-4caa-a16e-c252016bf480@maowtm.org>
No problem it was an easy fix. I'll reply directly to your patch in the
future, I'm still figuring out mailing list development and kernel
development workflows.
I do realize now I have some behaviors in this series that impact the
quiet flag that I want your input on. I'll add those as replies to
your patch series as well.
I plan to keep rebasing this patch off the quiet flag series as you release
new versions. I also plan to introduce some selftests combining the two flags
and ensuring they interact with eachother as expected.
The next version of this patch is going to include some big refactorings
(most likely removing the xarrays) and fix some edge cases I discovered.
King Regards,
Justin Suess
^ 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