| Message ID | 20250212032155.1276806-3-jeffxu@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mseal system mappings | expand |
On Wed, Feb 12, 2025 at 03:21:50AM +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> > --- > .../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; This only tests that any mapping after the vdso is sealed. There is a real parser for /proc/self/smaps in tools/testing/selftests/mm/vm_util.[ch], see check_vmflag_io(). > + } > + } > + > + return false; > +} <snip>
On Wed, Feb 12, 2025 at 5:04 AM Thomas Weißschuh <thomas.weissschuh@linutronix.de> wrote: > > On Wed, Feb 12, 2025 at 03:21:50AM +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> > > --- > > .../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; > > This only tests that any mapping after the vdso is sealed. The code above begins by searching for the "[vdso]" string, then looks for the first line that starts with "VmFlags:", and looks for the "sl" substring within that line. We're assuming the format of smaps won't change from release to release. > There is a real parser for /proc/self/smaps in > tools/testing/selftests/mm/vm_util.[ch], see check_vmflag_io(). > This test is in selftest/x86, which makes it hard to include the vm_util from selftest/mm, if that's what you're asking. If you are asking reusing the code logic from vm_util, the check_vmflag_io() calls __get_smap_entry(addr, "VmFlags:", ...), which begins by searching for address, then looks for the first line that started with "VmFlags:", then check for the "io" within that line. This is the same logic as my code, if I read the code correctly. Thanks -Jeff > > + } > > + } > > + > > + return false; > > +} > > <snip>
On February 13, 2025 6:14:00 AM PST, Jeff Xu <jeffxu@chromium.org> wrote: >On Wed, Feb 12, 2025 at 5:04 AM Thomas Weißschuh ><thomas.weissschuh@linutronix.de> wrote: >> >> On Wed, Feb 12, 2025 at 03:21:50AM +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> >> > --- >> > .../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; >> >> This only tests that any mapping after the vdso is sealed. > >The code above begins by searching for the "[vdso]" string, then looks >for the first line that starts with "VmFlags:", and looks for the "sl" >substring within that line. We're assuming the format of smaps won't >change from release to release. Oh, I missed this in my reviews: nothing _resets_ has_vdso to false, so if any other mapping follows vdso that happens to be sealed, this will return true... > >> There is a real parser for /proc/self/smaps in >> tools/testing/selftests/mm/vm_util.[ch], see check_vmflag_io(). >> >This test is in selftest/x86, which makes it hard to include the >vm_util from selftest/mm, if that's what you're asking. Hm, we have done these kinds of inter-tests dependencies before. (E.g. seccomp includes stuff from the clone tests.) I think it should be possible if it can just be a header include. Linking across tests would be more frustrating. -Kees
On Thu, Feb 13, 2025 at 11:28 AM Kees Cook <kees@kernel.org> wrote: > > > > On February 13, 2025 6:14:00 AM PST, Jeff Xu <jeffxu@chromium.org> wrote: > >On Wed, Feb 12, 2025 at 5:04 AM Thomas Weißschuh > ><thomas.weissschuh@linutronix.de> wrote: > >> > >> On Wed, Feb 12, 2025 at 03:21:50AM +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> > >> > --- > >> > .../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; > >> > >> This only tests that any mapping after the vdso is sealed. > > > >The code above begins by searching for the "[vdso]" string, then looks > >for the first line that starts with "VmFlags:", and looks for the "sl" > >substring within that line. We're assuming the format of smaps won't > >change from release to release. > > Oh, I missed this in my reviews: nothing _resets_ has_vdso to false, so if any other mapping follows vdso that happens to be sealed, this will return true... > It won't return the next mapping's sealing flag. After finding the "[vdso]", if the next line that contains VMFLAGS doesn't have the "sl" flag, the function returns false immediately. > > > >> There is a real parser for /proc/self/smaps in > >> tools/testing/selftests/mm/vm_util.[ch], see check_vmflag_io(). > >> > >This test is in selftest/x86, which makes it hard to include the > >vm_util from selftest/mm, if that's what you're asking. > > Hm, we have done these kinds of inter-tests dependencies before. (E.g. seccomp includes stuff from the clone tests.) I think it should be possible if it can just be a header include. Linking across tests would be more frustrating. > I can switch to vm_util, I will need to add a new parsing function in vm_util, the existing __get_smap_entry() only searches for vm's starting address, not name. Thanks -Jeff > -Kees > > -- > Kees Cook
On February 13, 2025 2:20:01 PM PST, Jeff Xu <jeffxu@chromium.org> wrote: >On Thu, Feb 13, 2025 at 11:28 AM Kees Cook <kees@kernel.org> wrote: >> >> >> >> On February 13, 2025 6:14:00 AM PST, Jeff Xu <jeffxu@chromium.org> wrote: >> >On Wed, Feb 12, 2025 at 5:04 AM Thomas Weißschuh >> ><thomas.weissschuh@linutronix.de> wrote: >> >> >> >> On Wed, Feb 12, 2025 at 03:21:50AM +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> >> >> > --- >> >> > .../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; >> >> >> >> This only tests that any mapping after the vdso is sealed. >> > >> >The code above begins by searching for the "[vdso]" string, then looks >> >for the first line that starts with "VmFlags:", and looks for the "sl" >> >substring within that line. We're assuming the format of smaps won't >> >change from release to release. >> >> Oh, I missed this in my reviews: nothing _resets_ has_vdso to false, so if any other mapping follows vdso that happens to be sealed, this will return true... >> >It won't return the next mapping's sealing flag. >After finding the "[vdso]", if the next line that contains VMFLAGS >doesn't have the "sl" flag, the function returns false immediately. Oh! Agh, yes. You are right, this is all fine. >I can switch to vm_util, I will need to add a new parsing function in >vm_util, the existing __get_smap_entry() only searches for vm's >starting address, not name. Unless someone feels strongly about this, my instinct is to avoid the higher complexity of a cross-test thing. -Kees
On Thu, Feb 13, 2025 at 6:52 PM Kees Cook <kees@kernel.org> wrote: > > > > On February 13, 2025 2:20:01 PM PST, Jeff Xu <jeffxu@chromium.org> wrote: > >On Thu, Feb 13, 2025 at 11:28 AM Kees Cook <kees@kernel.org> wrote: > >> > >> > >> > >> On February 13, 2025 6:14:00 AM PST, Jeff Xu <jeffxu@chromium.org> wrote: > >> >On Wed, Feb 12, 2025 at 5:04 AM Thomas Weißschuh > >> ><thomas.weissschuh@linutronix.de> wrote: > >> >> > >> >> On Wed, Feb 12, 2025 at 03:21:50AM +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> > >> >> > --- > >> >> > .../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; > >> >> > >> >> This only tests that any mapping after the vdso is sealed. > >> > > >> >The code above begins by searching for the "[vdso]" string, then looks > >> >for the first line that starts with "VmFlags:", and looks for the "sl" > >> >substring within that line. We're assuming the format of smaps won't > >> >change from release to release. > >> > >> Oh, I missed this in my reviews: nothing _resets_ has_vdso to false, so if any other mapping follows vdso that happens to be sealed, this will return true... > >> > >It won't return the next mapping's sealing flag. > >After finding the "[vdso]", if the next line that contains VMFLAGS > >doesn't have the "sl" flag, the function returns false immediately. > > Oh! Agh, yes. You are right, this is all fine. > > >I can switch to vm_util, I will need to add a new parsing function in > >vm_util, the existing __get_smap_entry() only searches for vm's > >starting address, not name. > > Unless someone feels strongly about this, my instinct is to avoid the higher complexity of a cross-test thing. > OK. I will keep the existing test. If we decide to use vm_util, it would be best to refactor it separately later on. The existing vm_util can't be used as is for my needs, so some refactoring would be necessary. Thanks -Jeff > -Kees > > > -- > Kees Cook
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);