| Message ID | 20250224225246.3712295-3-jeffxu@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mseal system mappings | expand |
On Mon, Feb 24, 2025 at 10:52:41PM +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. I feel this is a little succinct of a commit message, but I guess it gets to the heart of what you're doing here. Fundamentally I mean it makes sense, but I'm concerned that x86 has a test -expliictly checking- whether mremap() of VDSO is possible - are there cases where x86 might want to do this internal to the kernel? I guess not since this is essentially a userland self test and probably asserting you can do this in the way rr, etc. do. > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > Reviewed-by: Kees Cook <kees@kernel.org> Anyway, this aside, this looks fine, aside from nit below, so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > .../testing/selftests/x86/test_mremap_vdso.c | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c > index d53959e03593..94bee6e0c813 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,55 @@ 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) Should be static? > +{ > + 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, errno=%d\n", > + errno); > + > + return 0; > + } > + > + if (vdso_sealed(maps)) { > + ksft_test_result_skip("vdso is sealed\n"); > + return 0; > + } > + > + fclose(maps); > + > child = fork(); > if (child == -1) > ksft_exit_fail_msg("failed to fork (%d): %m\n", errno); > -- > 2.48.1.658.g4767266eb4-goog >
On Mon, Feb 24, 2025 at 10:15 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Mon, Feb 24, 2025 at 10:52:41PM +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. > > I feel this is a little succinct of a commit message, but I guess it gets to the > heart of what you're doing here. > > Fundamentally I mean it makes sense, but I'm concerned that x86 has a test > -expliictly checking- whether mremap() of VDSO is possible - are there cases > where x86 might want to do this internal to the kernel? > > I guess not since this is essentially a userland self test and probably > asserting you can do this in the way rr, etc. do. > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > Reviewed-by: Kees Cook <kees@kernel.org> > > Anyway, this aside, this looks fine, aside from nit below, so: > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > --- > > .../testing/selftests/x86/test_mremap_vdso.c | 43 +++++++++++++++++++ > > 1 file changed, 43 insertions(+) > > > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c > > index d53959e03593..94bee6e0c813 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,55 @@ 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) > > Should be static? > sure. > > +{ > > + 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, errno=%d\n", > > + errno); > > + > > + return 0; > > + } > > + > > + if (vdso_sealed(maps)) { > > + ksft_test_result_skip("vdso is sealed\n"); > > + return 0; > > + } > > + > > + fclose(maps); > > + > > child = fork(); > > if (child == -1) > > ksft_exit_fail_msg("failed to fork (%d): %m\n", errno); > > -- > > 2.48.1.658.g4767266eb4-goog > >
On Tue, Feb 25, 2025 at 02:37:46PM -0800, Jeff Xu wrote: > On Mon, Feb 24, 2025 at 10:15 PM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > On Mon, Feb 24, 2025 at 10:52:41PM +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. > > > > I feel this is a little succinct of a commit message, but I guess it gets to the > > heart of what you're doing here. > > > > Fundamentally I mean it makes sense, but I'm concerned that x86 has a test > > -expliictly checking- whether mremap() of VDSO is possible - are there cases > > where x86 might want to do this internal to the kernel? > > > > I guess not since this is essentially a userland self test and probably > > asserting you can do this in the way rr, etc. do. > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > > Reviewed-by: Kees Cook <kees@kernel.org> > > > > Anyway, this aside, this looks fine, aside from nit below, so: > > > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > > > --- > > > .../testing/selftests/x86/test_mremap_vdso.c | 43 +++++++++++++++++++ > > > 1 file changed, 43 insertions(+) > > > > > > diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c > > > index d53959e03593..94bee6e0c813 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,55 @@ 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) > > > > Should be static? > > > sure. Thanks! :) > > > > +{ > > > + 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, errno=%d\n", > > > + errno); > > > + > > > + return 0; > > > + } > > > + > > > + if (vdso_sealed(maps)) { > > > + ksft_test_result_skip("vdso is sealed\n"); > > > + return 0; > > > + } > > > + > > > + fclose(maps); > > > + > > > child = fork(); > > > if (child == -1) > > > ksft_exit_fail_msg("failed to fork (%d): %m\n", errno); > > > -- > > > 2.48.1.658.g4767266eb4-goog > > >
diff --git a/tools/testing/selftests/x86/test_mremap_vdso.c b/tools/testing/selftests/x86/test_mremap_vdso.c index d53959e03593..94bee6e0c813 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,55 @@ 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, errno=%d\n", + errno); + + return 0; + } + + if (vdso_sealed(maps)) { + ksft_test_result_skip("vdso is sealed\n"); + return 0; + } + + fclose(maps); + child = fork(); if (child == -1) ksft_exit_fail_msg("failed to fork (%d): %m\n", errno);