Re: [PATCH v8 7/7] selftest: test system mappings are sealed.

[Jeff Xu]

On Mon, Mar 3, 2025 at 12:20 PM Jeff Xu <jeffxu@xxxxxxxxxxxx> wrote:
>
> On Mon, Mar 3, 2025 at 9:01 AM Kees Cook <kees@xxxxxxxxxx> wrote:
> >
> > On Mon, Mar 03, 2025 at 05:09:21AM +0000, jeffxu@xxxxxxxxxxxx wrote:
> > > From: Jeff Xu <jeffxu@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > ---
> > >  .../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.local/
>
> > --
> > Kees Cook