| Message ID | 20250224174513.3600914-3-jeffxu@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mseal system mappings | expand |
On Mon, Feb 24, 2025 at 05:45:08PM +0000, jeffxu@chromium.org wrote: > From: Jeff Xu <jeffxu@chromium.org> > > Add code to detect if the vdso is memory sealed, skip the test > if it is. > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> Thanks, after my full 360 on reading this code, I'm back to where I was originally: it looks good to me. :) Reviewed-by: Kees Cook <kees@kernel.org>
* jeffxu@chromium.org <jeffxu@chromium.org> [250224 12:45]: > From: Jeff Xu <jeffxu@chromium.org> > > Add code to detect if the vdso is memory sealed, skip the test > if it is. It also skips the test if fopen fails on smaps, but maybe that's super rare? > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > --- > .../testing/selftests/x86/test_mremap_vdso.c | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c > index d53959e03593..c68077c56b22 100644 > --- a/tools/testing/selftests/x86/test_mremap_vdso.c > +++ b/tools/testing/selftests/x86/test_mremap_vdso.c > @@ -14,6 +14,7 @@ > #include <errno.h> > #include <unistd.h> > #include <string.h> > +#include <stdbool.h> > > #include <sys/mman.h> > #include <sys/auxv.h> > @@ -55,13 +56,50 @@ static int try_to_remap(void *vdso_addr, unsigned long size) > > } > > +#define VDSO_NAME "[vdso]" > +#define VMFLAGS "VmFlags:" > +#define MSEAL_FLAGS "sl" > +#define MAX_LINE_LEN 512 > + > +bool vdso_sealed(FILE *maps) > +{ > + char line[MAX_LINE_LEN]; > + bool has_vdso = false; > + > + while (fgets(line, sizeof(line), maps)) { > + if (strstr(line, VDSO_NAME)) > + has_vdso = true; > + > + if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) { > + if (strstr(line, MSEAL_FLAGS)) > + return true; > + > + return false; > + } > + } > + > + return false; > +} > + > int main(int argc, char **argv, char **envp) > { > pid_t child; > + FILE *maps; > > ksft_print_header(); > ksft_set_plan(1); > > + maps = fopen("/proc/self/smaps", "r"); > + if (!maps) { > + ksft_test_result_skip("Could not open /proc/self/smaps\n"); fork() below prints errno, should this also print errno? > + return 0; I guess the logic here is that you might fail later because it's sealed but you don't know? Is this rare enough not to matter? > + } > + > + if (vdso_sealed(maps)) { > + ksft_test_result_skip("vdso is sealed\n"); > + return 0; > + } > + This file also has an #ifdef __i386__ later, Using it here would prevent unnecessary scanning of the maps file. Probably not a big deal? Do you need to close the maps file? > child = fork(); > if (child == -1) > ksft_exit_fail_msg("failed to fork (%d): %m\n", errno); > -- > 2.48.1.601.g30ceb7b040-goog >
On Mon, Feb 24, 2025 at 1:05 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote: > > * jeffxu@chromium.org <jeffxu@chromium.org> [250224 12:45]: > > From: Jeff Xu <jeffxu@chromium.org> > > > > Add code to detect if the vdso is memory sealed, skip the test > > if it is. > > It also skips the test if fopen fails on smaps, but maybe that's super > rare? > If we can't access /proc/pid/smaps, that's an "environment" issue unrelated to this test, therefore skip(). > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > --- > > .../testing/selftests/x86/test_mremap_vdso.c | 38 +++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c > > index d53959e03593..c68077c56b22 100644 > > --- a/tools/testing/selftests/x86/test_mremap_vdso.c > > +++ b/tools/testing/selftests/x86/test_mremap_vdso.c > > @@ -14,6 +14,7 @@ > > #include <errno.h> > > #include <unistd.h> > > #include <string.h> > > +#include <stdbool.h> > > > > #include <sys/mman.h> > > #include <sys/auxv.h> > > @@ -55,13 +56,50 @@ static int try_to_remap(void *vdso_addr, unsigned long size) > > > > } > > > > +#define VDSO_NAME "[vdso]" > > +#define VMFLAGS "VmFlags:" > > +#define MSEAL_FLAGS "sl" > > +#define MAX_LINE_LEN 512 > > + > > +bool vdso_sealed(FILE *maps) > > +{ > > + char line[MAX_LINE_LEN]; > > + bool has_vdso = false; > > + > > + while (fgets(line, sizeof(line), maps)) { > > + if (strstr(line, VDSO_NAME)) > > + has_vdso = true; > > + > > + if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) { > > + if (strstr(line, MSEAL_FLAGS)) > > + return true; > > + > > + return false; > > + } > > + } > > + > > + return false; > > +} > > + > > int main(int argc, char **argv, char **envp) > > { > > pid_t child; > > + FILE *maps; > > > > ksft_print_header(); > > ksft_set_plan(1); > > > > + maps = fopen("/proc/self/smaps", "r"); > > + if (!maps) { > > + ksft_test_result_skip("Could not open /proc/self/smaps\n"); > > fork() below prints errno, should this also print errno? > Sure. > > + return 0; > > I guess the logic here is that you might fail later because it's sealed > but you don't know? Is this rare enough not to matter? > yes. We should check the "sl" flag of vdso before continuing, or the results will be unpredictable. Android currently includes this selftest. In addition, you or someone commented that the selftest framework can randomly turn on kernel config and run selftest, so it is necessary to add this check to skip. > > + } > > + > > + if (vdso_sealed(maps)) { > > + ksft_test_result_skip("vdso is sealed\n"); > > + return 0; > > + } > > + > > This file also has an #ifdef __i386__ later, Using it here would > prevent unnecessary scanning of the maps file. Probably not a big deal? > > Do you need to close the maps file? > OK, I will add close for good coding practice. Thanks -Jeff
diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c index d53959e03593..c68077c56b22 100644 --- a/tools/testing/selftests/x86/test_mremap_vdso.c +++ b/tools/testing/selftests/x86/test_mremap_vdso.c @@ -14,6 +14,7 @@ #include <errno.h> #include <unistd.h> #include <string.h> +#include <stdbool.h> #include <sys/mman.h> #include <sys/auxv.h> @@ -55,13 +56,50 @@ static int try_to_remap(void *vdso_addr, unsigned long size) } +#define VDSO_NAME "[vdso]" +#define VMFLAGS "VmFlags:" +#define MSEAL_FLAGS "sl" +#define MAX_LINE_LEN 512 + +bool vdso_sealed(FILE *maps) +{ + char line[MAX_LINE_LEN]; + bool has_vdso = false; + + while (fgets(line, sizeof(line), maps)) { + if (strstr(line, VDSO_NAME)) + has_vdso = true; + + if (has_vdso && !strncmp(line, VMFLAGS, strlen(VMFLAGS))) { + if (strstr(line, MSEAL_FLAGS)) + return true; + + return false; + } + } + + return false; +} + int main(int argc, char **argv, char **envp) { pid_t child; + FILE *maps; ksft_print_header(); ksft_set_plan(1); + maps = fopen("/proc/self/smaps", "r"); + if (!maps) { + ksft_test_result_skip("Could not open /proc/self/smaps\n"); + return 0; + } + + if (vdso_sealed(maps)) { + ksft_test_result_skip("vdso is sealed\n"); + return 0; + } + child = fork(); if (child == -1) ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);