Re: [PATCH -next -v2 8/8] selftests/lsm: add lockdown securityfs test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] selftests/lsm: missing $(OUTPUT)/ prefix in run_lockdown_test
  Makefile target
- [Low] selftests/lsm: skipped downgrade test and duplicated setup in
  lockdown_test
--

commit 01625a3d98192538ac4075199b3f0186f4e7ed21
Author: Cai Xinchen <caixinchen1@huawei.com>

selftests/lsm: add lockdown securityfs test

This commit adds an opt-in kselftest for the /sys/kernel/security/lockdown
interface along with a shared read_file() helper in common.c and enables
CONFIG_SECURITY_LOCKDOWN_LSM in the selftest config fragment.

> diff --git a/tools/testing/selftests/lsm/Makefile b/tools/testing/selftests/lsm/Makefile
> index 3f80c0bc093de..3be7aa42761ce 100644
> --- a/tools/testing/selftests/lsm/Makefile
> +++ b/tools/testing/selftests/lsm/Makefile
> @@ -8,10 +8,18 @@ LOCAL_HDRS += common.h
>  TEST_GEN_PROGS := lsm_get_self_attr_test lsm_list_modules_test \
>  		  lsm_set_self_attr_test
>  
> +# lockdown_test changes the machine's lockdown level, so do not run it
> +# by default.
> +TEST_GEN_PROGS_EXTENDED := lockdown_test
> +
>  include ../lib.mk
>  
>  $(OUTPUT)/lsm_get_self_attr_test: lsm_get_self_attr_test.c common.c
>  $(OUTPUT)/lsm_set_self_attr_test: lsm_set_self_attr_test.c common.c
>  $(OUTPUT)/lsm_list_modules_test: lsm_list_modules_test.c common.c
> +$(OUTPUT)/lockdown_test: lockdown_test.c common.c
> +
> +run_lockdown_test: all
> +	$(call RUN_TESTS, lockdown_test)

[Severity: Medium]
Does run_lockdown_test fail on out-of-tree builds (such as make O=... or
make OUTPUT=...)?

../lib.mk prefixes TEST_GEN_PROGS_EXTENDED with $(OUTPUT)/ and the build
rule places the binary at $(OUTPUT)/lockdown_test, but run_lockdown_test
passes a bare lockdown_test to RUN_TESTS.  When OUTPUT points to a separate
directory, tools/testing/selftests/kselftest/runner.sh looks for
./lockdown_test in the source directory and fails with "Warning: file
lockdown_test is missing!".  Passing $(OUTPUT)/lockdown_test (or
$(TEST_GEN_PROGS_EXTENDED)) to RUN_TESTS avoids this.

[ ... ]

> diff --git a/tools/testing/selftests/lsm/lockdown_test.c b/tools/testing/selftests/lsm/lockdown_test.c
> new file mode 100644
> index 0000000000000..fed050551cff2
> --- /dev/null
> +++ b/tools/testing/selftests/lsm/lockdown_test.c

[ ... ]

> +TEST(lockdown_no_downgrade)
> +{
> +	char before[128], after[128], level[32];
> +	const char *lower;
> +	int ret;
> +
> +	ret = read_lockdown_status(before, sizeof(before));
> +	if (ret) {
> +		if (ret > 0)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		TH_LOG("cannot read %s: %s", LOCKDOWN_PATH, strerror(errno));
> +		ASSERT_EQ(0, ret);
> +	}
> +	if (geteuid() != 0)
> +		SKIP(return, "requires root");
> +	if (status_writable())
> +		SKIP(return, "cannot open %s for writing: %s",
> +		     LOCKDOWN_PATH, strerror(errno));
> +	ASSERT_EQ(0, get_active_level(before, level, sizeof(level)));
> +
> +	if (!strcmp(level, "confidentiality"))
> +		lower = "integrity";
> +	else if (!strcmp(level, "integrity"))
> +		lower = "none";
> +	else
> +		SKIP(return, "kernel is not locked down, nothing to downgrade");
> +
> +	errno = 0;
> +	ASSERT_EQ(-1, write_status(lower));
> +	ASSERT_EQ(EPERM, errno);
> +
> +	ASSERT_EQ(0, read_file(LOCKDOWN_PATH, after, sizeof(after)));
> +	ASSERT_STREQ(before, after);
> +}
> +
> +TEST(lockdown_same_level_rejected)
> +{
> +	char before[128], after[128], level[32];
> +	int ret;
> +
> +	ret = read_lockdown_status(before, sizeof(before));
> +	if (ret) {
> +		if (ret > 0)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		TH_LOG("cannot read %s: %s", LOCKDOWN_PATH, strerror(errno));
> +		ASSERT_EQ(0, ret);
> +	}
> +	if (geteuid() != 0)
> +		SKIP(return, "requires root");
> +	if (status_writable())
> +		SKIP(return, "cannot open %s for writing: %s",
> +		     LOCKDOWN_PATH, strerror(errno));
> +	ASSERT_EQ(0, get_active_level(before, level, sizeof(level)));
> +
> +	errno = 0;
> +	ASSERT_EQ(-1, write_status(level));
> +	ASSERT_EQ(EPERM, errno);
> +
> +	ASSERT_EQ(0, read_file(LOCKDOWN_PATH, after, sizeof(after)));
> +	ASSERT_STREQ(before, after);
> +}
> +
> +/*
> + * This test raises the lockdown level and cannot undo it, so run it last.
> + */
> +TEST(lockdown_upgrade)
> +{
> +	char before[128], after[128], level[32];
> +	const char *next;
> +	int ret;
> +
> +	ret = read_lockdown_status(before, sizeof(before));
> +	if (ret) {
> +		if (ret > 0)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		TH_LOG("cannot read %s: %s", LOCKDOWN_PATH, strerror(errno));
> +		ASSERT_EQ(0, ret);
> +	}
> +	if (geteuid() != 0)
> +		SKIP(return, "requires root");
> +	if (status_writable())
> +		SKIP(return, "cannot open %s for writing: %s",
> +		     LOCKDOWN_PATH, strerror(errno));
> +	ASSERT_EQ(0, get_active_level(before, level, sizeof(level)));
> +
> +	if (!strcmp(level, "none"))
> +		next = "integrity";
> +	else if (!strcmp(level, "integrity"))
> +		next = "confidentiality";
> +	else
> +		SKIP(return, "kernel is already at the highest level");
> +
> +	errno = 0;
> +	ASSERT_EQ(0, write_status(next));
> +
> +	ASSERT_EQ(0, read_file(LOCKDOWN_PATH, after, sizeof(after)));
> +	ASSERT_EQ(0, get_active_level(after, level, sizeof(level)));
> +	ASSERT_STREQ(next, level);
> +}

[Severity: Low]
Could lockdown_no_downgrade run after lockdown_upgrade so that a single
invocation of make run_lockdown_test exercises downgrade rejection?

Enabling CONFIG_SECURITY_LOCKDOWN_LSM=y defaults to
CONFIG_LOCK_DOWN_KERNEL_FORCE_NONE=y, so a freshly booted kernel starts at
"none".  With the current test order, lockdown_no_downgrade always skips on
that initial state right before lockdown_upgrade raises the level to
"integrity"; placing lockdown_no_downgrade after lockdown_upgrade leaves the
system in the same final state while testing that downgrading from
"integrity" to "none" fails with EPERM.

Also, the prerequisite checks (read_lockdown_status(), geteuid() != 0,
status_writable(), and get_active_level()) are duplicated across all four
write tests and could be shared via FIXTURE() / FIXTURE_SETUP() or a helper.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110132818.2231593-1-caixinchen1@huawei.com?part=8