Re: [PATCH 6/6] selftests/landlock: Test capabilities guarding other LSMs' policies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] selftests/landlock: missing set_root_euid() in
  apparmor_policy_load()
--

commit 04d94a6e7f98554d70c200b902dd42758c1ea088
Author: Mickaël Salaün <mic@digikod.net>

selftests/landlock: Test capabilities guarding other LSMs' policies

This commit adds selftests verifying that Landlock capability restrictions
on CAP_MAC_ADMIN and CAP_MAC_OVERRIDE apply to Smack label operations and
AppArmor policy loading, along with an audit test for Smack capability
denials.

> diff --git a/tools/testing/selftests/landlock/cap_test.c b/tools/testing/selftests/landlock/cap_test.c
> index 741567c5ab417..7f9b7fb2bade8 100644
> --- a/tools/testing/selftests/landlock/cap_test.c
> +++ b/tools/testing/selftests/landlock/cap_test.c
[ ... ]
> @@ -969,6 +974,180 @@ TEST_F(cap_other_hooks, capset_inheritable)
[ ... ]
> +/* AppArmor requires CAP_MAC_ADMIN to load policy. */
> +TEST_F(cap_other_hooks, apparmor_policy_load)
> +{
> +	if (!is_lsm_active(_metadata, LSM_ID_APPARMOR))
> +		SKIP(return, "AppArmor is not active");
> +
> +	if (access(APPARMOR_LOAD_PATH, F_OK)) {
> +		set_cap(_metadata, CAP_SYS_ADMIN);
> +		ASSERT_EQ(0, unshare(CLONE_NEWNS));
> +		ASSERT_EQ(0, mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, NULL));
> +		ASSERT_EQ(0, mount("securityfs", "/sys/kernel/security",
> +				   "securityfs", 0, NULL));
> +		clear_cap(_metadata, CAP_SYS_ADMIN);
> +	}
> +	ASSERT_EQ(0, access(APPARMOR_LOAD_PATH, F_OK));
> +
> +	enforce_cap_variant(_metadata, variant, CAP_MAC_ADMIN);
> +
> +	EXPECT_EQ(EACCES, write_apparmor_load(_metadata));
> +
> +	set_cap(_metadata, CAP_MAC_ADMIN);
> +	if (variant->is_allowed) {
> +		EXPECT_EQ(EPROTONOSUPPORT, write_apparmor_load(_metadata));
> +	} else {
> +		EXPECT_EQ(EACCES, write_apparmor_load(_metadata));
> +	}
> +	clear_cap(_metadata, CAP_MAC_ADMIN);
> +}

[Severity: Low]
Does apparmor_policy_load need to call set_root_euid(_metadata) before
testing write_apparmor_load()?

Writing to /sys/kernel/security/apparmor/.load reaches
aa_may_manage_policy() -> aa_policy_admin_capable() ->
aa_policy_view_capable():

security/apparmor/policy.c:aa_policy_view_capable() {
	...
	bool root_in_user_ns = uid_eq(current_euid(), make_kuid(user_ns, 0)) ||
			       in_egroup_p(make_kgid(user_ns, 0));
	...
}

When cap_test is run as a non-root user with file capabilities,
root_in_user_ns is false, so write_apparmor_load() returns EACCES instead
of EPROTONOSUPPORT in the allowed variants even when CAP_MAC_ADMIN is
effective.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110203042.2410140-1-mic@digikod.net?part=6