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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/lsm: duplicated setup/teardown and double open in lockdown_test.c
--

commit 5cebacd6e5feb5064c2370aa4d99aad209688038
Author: Cai Xinchen <caixinchen1@huawei.com>

selftests/lsm: add lockdown securityfs test

This commit adds a kselftest for the lockdown securityfs interface to
check the read format, reject unknown levels, and verify that an already
locked down kernel rejects the current or a lower level. It also moves
the file reading logic in common.c into a shared read_file() helper.

> diff --git a/tools/testing/selftests/lsm/lockdown_test.c b/tools/testing/selftests/lsm/lockdown_test.c
> new file mode 100644
> index 0000000000000..35109833da373
> --- /dev/null
> +++ b/tools/testing/selftests/lsm/lockdown_test.c
> @@ -0,0 +1,176 @@

[ ... ]

> +static int write_status(const char *value)
> +{
> +	ssize_t len;
> +	int fd, err;
> +
> +	fd = open(LOCKDOWN_PATH, O_WRONLY);
> +	if (fd < 0)
> +		return -1;
> +
> +	len = write(fd, value, strlen(value));
> +	err = errno;
> +	close(fd);
> +	if (len < 0) {
> +		errno = err;
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int status_writable(void)
> +{
> +	int fd;
> +
> +	fd = open(LOCKDOWN_PATH, O_WRONLY);
> +	if (fd < 0)
> +		return -1;
> +
> +	close(fd);
> +	return 0;
> +}

[ ... ]

> +TEST(lockdown_invalid_write)
> +{
> +	char before[128], after[128];
> +
> +	if (read_file(LOCKDOWN_PATH, before, sizeof(before))) {
> +		if (errno == ENOENT)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		SKIP(return, "cannot read %s: %s", LOCKDOWN_PATH,
> +		     strerror(errno));
> +	}
> +	if (geteuid() != 0)
> +		SKIP(return, "requires root");
> +	if (status_writable())
> +		SKIP(return, "cannot open %s for writing: %s",
> +		     LOCKDOWN_PATH, strerror(errno));
> +
> +	errno = 0;
> +	ASSERT_EQ(-1, write_status("invalid-level"));
> +	ASSERT_EQ(EINVAL, errno);
> +
> +	ASSERT_EQ(0, read_file(LOCKDOWN_PATH, after, sizeof(after)));
> +	ASSERT_STREQ(before, after);
> +}
> +
> +TEST(lockdown_no_downgrade)
> +{
> +	char before[128], after[128], level[32];
> +	const char *lower;
> +
> +	if (read_file(LOCKDOWN_PATH, before, sizeof(before))) {
> +		if (errno == ENOENT)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		SKIP(return, "cannot read %s: %s", LOCKDOWN_PATH,
> +		     strerror(errno));
> +	}
> +	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, refusing to escalate");
> +
> +	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];
> +
> +	if (read_file(LOCKDOWN_PATH, before, sizeof(before))) {
> +		if (errno == ENOENT)
> +			SKIP(return, "%s is not present", LOCKDOWN_PATH);
> +		SKIP(return, "cannot read %s: %s", LOCKDOWN_PATH,
> +		     strerror(errno));
> +	}
> +	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);
> +}

[Severity: Low]
This isn't a bug, but lockdown_invalid_write, lockdown_no_downgrade, and
lockdown_same_level_rejected duplicate the same initial read_file() and
SKIP() checks, root check, status_writable() check, active level parsing,
and trailing read_file() plus ASSERT_STREQ(before, after) check.

Additionally, in each of those write tests, status_writable() opens
LOCKDOWN_PATH with O_WRONLY and immediately closes it, only for
write_status() to open and close LOCKDOWN_PATH with O_WRONLY a second
time.

Could this use FIXTURE(), FIXTURE_SETUP(), FIXTURE_TEARDOWN(), and
TEST_F() from kselftest_harness.h (or a shared setup/write helper) to
avoid the duplicated prerequisite/post-condition blocks and redundant
file opens?

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