| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202503030848.30D0E54E7@keescook>
Date: Mon, 3 Mar 2025 09:01:11 -0800
From: Kees Cook <kees@...nel.org>
To: jeffxu@...omium.org
Cc: akpm@...ux-foundation.org, jannh@...gle.com,
torvalds@...ux-foundation.org, vbabka@...e.cz,
lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com,
adhemerval.zanella@...aro.org, oleg@...hat.com, avagin@...il.com,
benjamin@...solutions.net, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org, linux-mm@...ck.org,
linux-kselftest@...r.kernel.org, jorgelo@...omium.org,
sroettger@...gle.com, hch@....de, ojeda@...nel.org,
thomas.weissschuh@...utronix.de, adobriyan@...il.com,
johannes@...solutions.net, pedro.falcato@...il.com,
hca@...ux.ibm.com, willy@...radead.org, anna-maria@...utronix.de,
mark.rutland@....com, linus.walleij@...aro.org, Jason@...c4.com,
deller@....de, rdunlap@...radead.org, davem@...emloft.net,
peterx@...hat.com, f.fainelli@...il.com, gerg@...nel.org,
dave.hansen@...ux.intel.com, mingo@...nel.org, ardb@...nel.org,
mhocko@...e.com, 42.hyeyoo@...il.com, peterz@...radead.org,
ardb@...gle.com, enh@...gle.com, rientjes@...gle.com,
groeck@...omium.org, mpe@...erman.id.au,
aleksandr.mikhalitsyn@...onical.com, mike.rapoport@...il.com
Subject: Re: [PATCH v8 7/7] selftest: test system mappings are sealed.
On Mon, Mar 03, 2025 at 05:09:21AM +0000, jeffxu@...omium.org wrote:
> From: Jeff Xu <jeffxu@...omium.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@...omium.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. :)
> +#define VMFLAGS "VmFlags:"
This one gets a strlen() on it, so it feels better to have a macro.
> +#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?
> +};
> +
> +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. :)
> +
> +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));
--
Kees Cook
Powered by blists - more mailing lists