Date: Mon, 21 Feb 2022 16:09:34 +0000 From: Jessica Clarke <jrtc27@freebsd.org> To: Eric van Gyzen <vangyzen@FreeBSD.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 766c2466ff46 - main - mmap map_at_zero test: handle W^X Message-ID: <5DF1CA02-6AF7-4588-A342-29F769CD5F23@freebsd.org> In-Reply-To: <202202211546.21LFkKhX053274@gitrepo.freebsd.org> References: <202202211546.21LFkKhX053274@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21 Feb 2022, at 15:46, Eric van Gyzen <vangyzen@FreeBSD.org> wrote: >=20 > The branch main has been updated by vangyzen: >=20 > URL: = https://cgit.FreeBSD.org/src/commit/?id=3D766c2466ff465b3c7c1a46be729b42a6= da47de03 >=20 > commit 766c2466ff465b3c7c1a46be729b42a6da47de03 > Author: Arka Sharma <arka_sharma@dell.com> > AuthorDate: 2022-02-18 15:34:15 +0000 > Commit: Eric van Gyzen <vangyzen@FreeBSD.org> > CommitDate: 2022-02-21 15:43:42 +0000 >=20 > mmap map_at_zero test: handle W^X >=20 > Use kern.elfXX.allow_wx to decide whether to map W+X or W-only = memory. >=20 > Future work could expand this test to add an "allow_wx" axis to the > test matrix, but I would argue that a separate test should be = written, > since that's orthogonal to map_at_zero. >=20 > MFC after: 1 week > Sponsored by: Dell EMC Isilon > --- > tests/sys/vm/mmap_test.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) >=20 > diff --git a/tests/sys/vm/mmap_test.c b/tests/sys/vm/mmap_test.c > index 61ede96fc49b..dc01a23fff21 100644 > --- a/tests/sys/vm/mmap_test.c > +++ b/tests/sys/vm/mmap_test.c > @@ -34,6 +34,7 @@ > #include <errno.h> > #include <fcntl.h> > #include <stdarg.h> > +#include <stdbool.h> > #include <stdio.h> > #include <stdlib.h> >=20 > @@ -54,6 +55,12 @@ static const struct { >=20 > #define MAP_AT_ZERO "security.bsd.map_at_zero" >=20 > +#ifdef __LP64__ > +#define ALLOW_WX "kern.elf64.allow_wx" > +#else > +#define ALLOW_WX "kern.elf32.allow_wx" > +#endif > + > ATF_TC_WITHOUT_HEAD(mmap__map_at_zero); > ATF_TC_BODY(mmap__map_at_zero, tc) > { > @@ -61,6 +68,8 @@ ATF_TC_BODY(mmap__map_at_zero, tc) > size_t len; > unsigned int i; > int map_at_zero; > + bool allow_wx; > + int prot_flags; >=20 > len =3D sizeof(map_at_zero); > if (sysctlbyname(MAP_AT_ZERO, &map_at_zero, &len, NULL, 0) =3D=3D = -1) { > @@ -69,13 +78,27 @@ ATF_TC_BODY(mmap__map_at_zero, tc) > return; > } >=20 > + len =3D sizeof(allow_wx); > + if (sysctlbyname(ALLOW_WX, &allow_wx, &len, NULL, 0) =3D=3D -1) = { > + if (errno =3D=3D ENOENT) { > + /* Allow W+X if sysctl isn't present */ > + allow_wx =3D true; > + } else { > + atf_tc_skip("sysctl for %s failed: %s\n", = ALLOW_WX, > + strerror(errno)); > + return; > + } > + } > + > /* Normalize to 0 or 1 for array access. */ > map_at_zero =3D !!map_at_zero; >=20 > for (i =3D 0; i < nitems(map_at_zero_tests); i++) { > + prot_flags =3D PROT_READ | PROT_WRITE; > + if (allow_wx) > + prot_flags |=3D PROT_EXEC; > p =3D mmap((void *)map_at_zero_tests[i].addr, PAGE_SIZE, > - PROT_READ | PROT_WRITE | PROT_EXEC, MAP_ANON | = MAP_FIXED, > - -1, 0); > + prot_flags, MAP_ANON | MAP_FIXED, -1, 0); > if (p =3D=3D MAP_FAILED) { > = ATF_CHECK_MSG(map_at_zero_tests[i].ok[map_at_zero] =3D=3D 0, > "mmap(%p, ...) failed", = map_at_zero_tests[i].addr); If the test is just as legitimate without PROT_EXEC, what=E2=80=99s the justification for not just removing PROT_EXEC entirely rather than making its behaviour depend on the sysctl, which could become confusing (and complicates the test)? IMO either the test should be skipped for !allow_wx or it should always just make a RW mapping; this choice is rather odd. Jess
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5DF1CA02-6AF7-4588-A342-29F769CD5F23>