On Mon, Mar 3, 2025 at 12:20 PM Jeff Xu jeffxu@chromium.org wrote:
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.
After connecting with Kees offline, it seems I misunderstood. We need to add a check for heap so the test can use it as a negative test. I'll add that in V9.
We can postpone the discussion of detecting KCONFIG during the selftest runtime, which isn't necessary for this selftest. Instead, it can rely on the added config file.
I will start working on V9.
Thanks -Jeff
Thanks -Jeff [1] https://lore.kernel.org/all/60f5b979-2969-4cb0-ad3d-262908869c5f@lucifer.loc...
-- Kees Cook