Re: git: 766c2466ff46 - main - mmap map_at_zero test: handle W^X

On 21 Feb 2022, at 15:46, Eric van Gyzen <vangyzen@FreeBSD.org> wrote:
> 
> The branch main has been updated by vangyzen:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=766c2466ff465b3c7c1a46be729b42a6da47de03
> 
> 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
> 
>    mmap map_at_zero test: handle W^X
> 
>    Use kern.elfXX.allow_wx to decide whether to map W+X or W-only memory.
> 
>    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.
> 
>    MFC after:      1 week
>    Sponsored by:   Dell EMC Isilon
> ---
> tests/sys/vm/mmap_test.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
> 
> 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>
> 
> @@ -54,6 +55,12 @@ static const struct {
> 
> #define	MAP_AT_ZERO	"security.bsd.map_at_zero"
> 
> +#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;
> 
> 	len = sizeof(map_at_zero);
> 	if (sysctlbyname(MAP_AT_ZERO, &map_at_zero, &len, NULL, 0) == -1) {
> @@ -69,13 +78,27 @@ ATF_TC_BODY(mmap__map_at_zero, tc)
> 		return;
> 	}
> 
> +	len = sizeof(allow_wx);
> +	if (sysctlbyname(ALLOW_WX, &allow_wx, &len, NULL, 0) == -1) {
> +		if (errno == ENOENT) {
> +			/* Allow W+X if sysctl isn't present */
> +			allow_wx = 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 = !!map_at_zero;
> 
> 	for (i = 0; i < nitems(map_at_zero_tests); i++) {
> +		prot_flags = PROT_READ | PROT_WRITE;
> +		if (allow_wx)
> +			prot_flags |= PROT_EXEC;
> 		p = 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 == MAP_FAILED) {
> 			ATF_CHECK_MSG(map_at_zero_tests[i].ok[map_at_zero] == 0,
> 			    "mmap(%p, ...) failed", map_at_zero_tests[i].addr);

If the test is just as legitimate without PROT_EXEC, what’s 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