On Mon, Mar 3, 2025 at 9:01 AM Kees Cook kees@kernel.org wrote:
On Mon, Mar 03, 2025 at 05:09:21AM +0000, jeffxu@chromium.org wrote:
From: Jeff Xu jeffxu@chromium.org
Add sysmap_is_sealed.c to test system mappings are sealed.
Note: CONFIG_MSEAL_SYSTEM_MAPPINGS must be set, as indicated in config file.
Signed-off-by: Jeff Xu jeffxu@chromium.org
.../mseal_system_mappings/.gitignore | 2 + .../selftests/mseal_system_mappings/Makefile | 6 + .../selftests/mseal_system_mappings/config | 1 + .../mseal_system_mappings/sysmap_is_sealed.c | 113 ++++++++++++++++++ 4 files changed, 122 insertions(+) create mode 100644 tools/testing/selftests/mseal_system_mappings/.gitignore create mode 100644 tools/testing/selftests/mseal_system_mappings/Makefile create mode 100644 tools/testing/selftests/mseal_system_mappings/config create mode 100644 tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c
diff --git a/tools/testing/selftests/mseal_system_mappings/.gitignore b/tools/testing/selftests/mseal_system_mappings/.gitignore new file mode 100644 index 000000000000..319c497a595e --- /dev/null +++ b/tools/testing/selftests/mseal_system_mappings/.gitignore @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +sysmap_is_sealed diff --git a/tools/testing/selftests/mseal_system_mappings/Makefile b/tools/testing/selftests/mseal_system_mappings/Makefile new file mode 100644 index 000000000000..2b4504e2f52f --- /dev/null +++ b/tools/testing/selftests/mseal_system_mappings/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0-only +CFLAGS += -std=c99 -pthread -Wall $(KHDR_INCLUDES)
+TEST_GEN_PROGS := sysmap_is_sealed
+include ../lib.mk diff --git a/tools/testing/selftests/mseal_system_mappings/config b/tools/testing/selftests/mseal_system_mappings/config new file mode 100644 index 000000000000..675cb9f37b86 --- /dev/null +++ b/tools/testing/selftests/mseal_system_mappings/config @@ -0,0 +1 @@ +CONFIG_MSEAL_SYSTEM_MAPPINGS=y diff --git a/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c b/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c new file mode 100644 index 000000000000..c1e93794a58b --- /dev/null +++ b/tools/testing/selftests/mseal_system_mappings/sysmap_is_sealed.c @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- test system mappings are sealed when
- KCONFIG_MSEAL_SYSTEM_MAPPINGS=y
- */
+#define _GNU_SOURCE +#include <stdio.h> +#include <errno.h> +#include <unistd.h> +#include <string.h> +#include <stdbool.h>
+#include "../kselftest.h" +#include "../kselftest_harness.h"
+#define VDSO_NAME "[vdso]" +#define VVAR_NAME "[vvar]" +#define VVAR_VCLOCK_NAME "[vvar_vclock]" +#define UPROBES_NAME "[uprobes]" +#define SIGPAGE_NAME "[sigpage]" +#define VECTORS_NAME "[vectors]"
These are only ever used once, and it feels like having them spelled out right in the variant definitions would be more readable, but I'm not sure I feel strongly enough about it to say it should be changed. They're available via "variant->name" as well, which makes it unlikely the macros will be used anywhere in the future? Maybe you have plans for them. :)
No plan for reuse them in other code, will move to Variant in v9.
+#define VMFLAGS "VmFlags:"
This one gets a strlen() on it, so it feels better to have a macro.
Ok, thanks for the reasoning.
+#define MSEAL_FLAGS "sl" +#define MAX_LINE_LEN 512
+bool has_mapping(char *name, FILE *maps) +{
char line[MAX_LINE_LEN];while (fgets(line, sizeof(line), maps)) {if (strstr(line, name))return true;}return false;+}
+bool mapping_is_sealed(char *name, FILE *maps) +{
char line[MAX_LINE_LEN];while (fgets(line, sizeof(line), maps)) {if (!strncmp(line, VMFLAGS, strlen(VMFLAGS))) {if (strstr(line, MSEAL_FLAGS))return true;return false;}}return false;+}
+FIXTURE(basic) {
FILE *maps;+};
+FIXTURE_SETUP(basic) +{
self->maps = fopen("/proc/self/smaps", "r");if (!self->maps)SKIP(return, "Could not open /proc/self/smap, errno=%d",errno);Good SKIP usage, though I wonder if not having /proc should be a full blown failure?
Usually, the failure is used to report failures directly related to what this code is testing. If /proc is unavailable, it's an environment setup issue, which is more fitting for SKIP, otherwise, we wouldn't need "SKIP" - we'd just report all environment requirements checked as failures.
Unless you mean that "/proc" is always available and can never be unavailable in any selftest environment? Then, I can change to use the failure reporting.
+};
+FIXTURE_TEARDOWN(basic) +{
if (self->maps)fclose(self->maps);+};
+FIXTURE_VARIANT(basic) +{
char *name;+};
+FIXTURE_VARIANT_ADD(basic, vdso) {
.name = VDSO_NAME,+};
+FIXTURE_VARIANT_ADD(basic, vvar) {
.name = VVAR_NAME,+};
+FIXTURE_VARIANT_ADD(basic, vvar_vclock) {
.name = VVAR_VCLOCK_NAME,+};
+FIXTURE_VARIANT_ADD(basic, sigpage) {
.name = SIGPAGE_NAME,+};
+FIXTURE_VARIANT_ADD(basic, vectors) {
.name = VECTORS_NAME,+};
+FIXTURE_VARIANT_ADD(basic, uprobes) {
.name = UPROBES_NAME,+};
I love seeing variants used in the test harness. :)
Ya, copied from landlock selftest :-)
+TEST_F(basic, is_sealed) +{
if (!has_mapping(variant->name, self->maps)) {SKIP(return, "could not found the mapping, %s",typo nit: "find" instead of "found"
variant->name);}EXPECT_TRUE(mapping_is_sealed(variant->name, self->maps));+};
This is a good "positive" test, but I'd like to see a negative test added as well. (This adds robustness against something going "all wrong" or "all right", like imagine that someone adds a VmFlags string named "slow", suddenly this test will always pass due to matching "sl". With a negative test added, it will fail when it finds "sl" when it's not expected.) For example, also check "[stack]" and "[heap]" and expect them NOT to be sealed.
You could update the variant as:
FIXTURE_VARIANT(basic) { char *name; bool sealed; };
FIXTURE_VARIANT_ADD(basic, vdso) { .name = "[vdso]", .sealed = true, };
FIXTURE_VARIANT_ADD(basic, stack) { .name = "[stack]", .sealed = false, };
And then update the is_sealed test to:
EXPECT_EQ(variant->sealed, mapping_is_sealed(variant->name, self->maps));
The challenge is that I'm unsure how to detect "CONFIG_MSEAL_SYSTEM_MAPPINGS" from selftest runtime.
Without that, the test can't reliably set the "sealed" flag. Lorenzo suggested parsing /proc/config.gz, but I responded, "None of the existing selftests use this pattern, and I'm not sure /proc/config.gz is enabled in the default kernel config." [1].
To work around this, in this version, I add selftests/mseal_system_mappings/config to indicate CONFIG_MSEAL_SYSTEM_MAPPINGS=y is a mandatory requirement for running this test. Therefore, this selftest assumes the ".sealed" is always true, i.e. no negative test.
I'm looking to Linux Kernel Self-Test (LKST) and Shuah Khan for guidance/suggestion on handling different kernel config variants within selftest.
Thanks -Jeff [1] https://lore.kernel.org/all/60f5b979-2969-4cb0-ad3d-262908869c5f@lucifer.loc...
-- Kees Cook