[PATCH v4 1/2] powerpc/mm: Support execute-only memory on the Radix MMU

[PATCH v4 1/2] powerpc/mm: Support execute-only memory on the Radix MMU

[Russell Currey]

Add support for execute-only memory (XOM) for the Radix MMU by using an
execute-only mapping, as opposed to the RX mapping used by powerpc's
other MMUs.

The Hash MMU already supports XOM through the execute-only pkey,
which is a separate mechanism shared with x86.  A PROT_EXEC-only mapping
will map to RX, and then the pkey will be applied on top of it.

mmap() and mprotect() consumers in userspace should observe the same
behaviour on Hash and Radix despite the differences in implementation.

Replacing the vma_is_accessible() check in access_error() with a read
check should be functionally equivalent for non-Radix MMUs, since it
follows write and execute checks.  For Radix, the change enables
detecting faults on execute-only mappings where vma_is_accessible() would
return true.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v4: Reword commit message, add changes suggested by Christophe and Aneesh

 arch/powerpc/include/asm/book3s/64/pgtable.h |  2 ++
 arch/powerpc/mm/book3s64/pgtable.c           | 11 +++++++++--
 arch/powerpc/mm/fault.c                      |  6 +++++-
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 392ff48f77df..486902aff040 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -151,6 +151,8 @@
 #define PAGE_COPY_X	__pgprot(_PAGE_BASE | _PAGE_READ | _PAGE_EXEC)
 #define PAGE_READONLY	__pgprot(_PAGE_BASE | _PAGE_READ)
 #define PAGE_READONLY_X	__pgprot(_PAGE_BASE | _PAGE_READ | _PAGE_EXEC)
+/* Radix only, Hash uses PAGE_READONLY_X + execute-only pkey instead */
+#define PAGE_EXECONLY	__pgprot(_PAGE_BASE | _PAGE_EXEC)
 
 /* Permission masks used for kernel mappings */
 #define PAGE_KERNEL	__pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index 7b9966402b25..f6151a589298 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -553,8 +553,15 @@ EXPORT_SYMBOL_GPL(memremap_compat_align);
 
 pgprot_t vm_get_page_prot(unsigned long vm_flags)
 {
-	unsigned long prot = pgprot_val(protection_map[vm_flags &
-					(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]);
+	unsigned long prot;
+
+	/* Radix supports execute-only, but protection_map maps X -> RX */
+	if (radix_enabled() && ((vm_flags & VM_ACCESS_FLAGS) == VM_EXEC)) {
+		prot = pgprot_val(PAGE_EXECONLY);
+	} else {
+		prot = pgprot_val(protection_map[vm_flags &
+						 (VM_ACCESS_FLAGS | VM_SHARED)]);
+	}
 
 	if (vm_flags & VM_SAO)
 		prot |= _PAGE_SAO;
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 014005428687..1566804e4b3d 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -270,7 +270,11 @@ static bool access_error(bool is_write, bool is_exec, struct vm_area_struct *vma
 		return false;
 	}
 
-	if (unlikely(!vma_is_accessible(vma)))
+	/*
+	 * Check for a read fault.  This could be caused by a read on an
+	 * inaccessible page (i.e. PROT_NONE), or a Radix MMU execute-only page.
+	 */
+	if (unlikely(!(vma->vm_flags & VM_READ)))
 		return true;
 	/*
 	 * We should ideally do the vma pkey access check here. But in the
-- 
2.37.2

[PATCH v4 2/2] selftests/powerpc: Add a test for execute-only memory

[Russell Currey]

From: Nicholas Miehlbradt <nicholas@linux.ibm.com>

This selftest is designed to cover execute-only protections
on the Radix MMU but will also work with Hash.

The tests are based on those found in pkey_exec_test with modifications
to use the generic mprotect() instead of the pkey variants.

Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v4: new

 tools/testing/selftests/powerpc/mm/Makefile   |   3 +-
 .../testing/selftests/powerpc/mm/exec_prot.c  | 231 ++++++++++++++++++
 2 files changed, 233 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/mm/exec_prot.c

diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
index 27dc09d0bfee..19dd0b2ea397 100644
--- a/tools/testing/selftests/powerpc/mm/Makefile
+++ b/tools/testing/selftests/powerpc/mm/Makefile
@@ -3,7 +3,7 @@ noarg:
 	$(MAKE) -C ../
 
 TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
-		  large_vm_fork_separation bad_accesses pkey_exec_prot \
+		  large_vm_fork_separation bad_accesses exec_prot pkey_exec_prot \
 		  pkey_siginfo stack_expansion_signal stack_expansion_ldst \
 		  large_vm_gpr_corruption
 TEST_PROGS := stress_code_patching.sh
@@ -22,6 +22,7 @@ $(OUTPUT)/wild_bctr: CFLAGS += -m64
 $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
 $(OUTPUT)/large_vm_gpr_corruption: CFLAGS += -m64
 $(OUTPUT)/bad_accesses: CFLAGS += -m64
+$(OUTPUT)/exec_prot: CFLAGS += -m64
 $(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
 $(OUTPUT)/pkey_siginfo: CFLAGS += -m64
 
diff --git a/tools/testing/selftests/powerpc/mm/exec_prot.c b/tools/testing/selftests/powerpc/mm/exec_prot.c
new file mode 100644
index 000000000000..db75b2225de1
--- /dev/null
+++ b/tools/testing/selftests/powerpc/mm/exec_prot.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022, Nicholas Miehlbradt, IBM Corporation
+ * based on pkey_exec_prot.c
+ *
+ * Test if applying execute protection on pages works as expected.
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <signal.h>
+
+#include <unistd.h>
+#include <sys/mman.h>
+
+#include "pkeys.h"
+
+
+#define PPC_INST_NOP	0x60000000
+#define PPC_INST_TRAP	0x7fe00008
+#define PPC_INST_BLR	0x4e800020
+
+static volatile sig_atomic_t fault_code;
+static volatile sig_atomic_t remaining_faults;
+static volatile unsigned int *fault_addr;
+static unsigned long pgsize, numinsns;
+static unsigned int *insns;
+static bool pkeys_supported;
+
+static bool is_fault_expected(int fault_code)
+{
+	if (fault_code == SEGV_ACCERR)
+		return true;
+
+	/* Assume any pkey error is fine since pkey_exec_prot test covers them */
+	if (fault_code == SEGV_PKUERR && pkeys_supported)
+		return true;
+
+	return false;
+}
+
+static void trap_handler(int signum, siginfo_t *sinfo, void *ctx)
+{
+	/* Check if this fault originated from the expected address */
+	if (sinfo->si_addr != (void *)fault_addr)
+		sigsafe_err("got a fault for an unexpected address\n");
+
+	_exit(1);
+}
+
+static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
+{
+	fault_code = sinfo->si_code;
+
+	/* Check if this fault originated from the expected address */
+	if (sinfo->si_addr != (void *)fault_addr) {
+		sigsafe_err("got a fault for an unexpected address\n");
+		_exit(1);
+	}
+
+	/* Check if too many faults have occurred for a single test case */
+	if (!remaining_faults) {
+		sigsafe_err("got too many faults for the same address\n");
+		_exit(1);
+	}
+
+
+	/* Restore permissions in order to continue */
+	if (is_fault_expected(fault_code)) {
+		if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE | PROT_EXEC)) {
+			sigsafe_err("failed to set access permissions\n");
+			_exit(1);
+		}
+	} else {
+		sigsafe_err("got a fault with an unexpected code\n");
+		_exit(1);
+	}
+
+	remaining_faults--;
+}
+
+static int check_exec_fault(int rights)
+{
+	/*
+	 * Jump to the executable region.
+	 *
+	 * The first iteration also checks if the overwrite of the
+	 * first instruction word from a trap to a no-op succeeded.
+	 */
+	fault_code = -1;
+	remaining_faults = 0;
+	if (!(rights & PROT_EXEC))
+		remaining_faults = 1;
+
+	FAIL_IF(mprotect(insns, pgsize, rights) != 0);
+	asm volatile("mtctr	%0; bctrl" : : "r"(insns));
+
+	FAIL_IF(remaining_faults != 0);
+	if (!(rights & PROT_EXEC))
+		FAIL_IF(!is_fault_expected(fault_code));
+
+	return 0;
+}
+
+static int test(void)
+{
+	struct sigaction segv_act, trap_act;
+	int i;
+
+	/* Skip the test if the CPU doesn't support Radix */
+	SKIP_IF(!have_hwcap2(PPC_FEATURE2_ARCH_3_00));
+
+	/* Check if pkeys are supported */
+	pkeys_supported = pkeys_unsupported() == 0;
+
+	/* Setup SIGSEGV handler */
+	segv_act.sa_handler = 0;
+	segv_act.sa_sigaction = segv_handler;
+	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &segv_act.sa_mask) != 0);
+	segv_act.sa_flags = SA_SIGINFO;
+	segv_act.sa_restorer = 0;
+	FAIL_IF(sigaction(SIGSEGV, &segv_act, NULL) != 0);
+
+	/* Setup SIGTRAP handler */
+	trap_act.sa_handler = 0;
+	trap_act.sa_sigaction = trap_handler;
+	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &trap_act.sa_mask) != 0);
+	trap_act.sa_flags = SA_SIGINFO;
+	trap_act.sa_restorer = 0;
+	FAIL_IF(sigaction(SIGTRAP, &trap_act, NULL) != 0);
+
+	/* Setup executable region */
+	pgsize = getpagesize();
+	numinsns = pgsize / sizeof(unsigned int);
+	insns = (unsigned int *)mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
+				      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	FAIL_IF(insns == MAP_FAILED);
+
+	/* Write the instruction words */
+	for (i = 1; i < numinsns - 1; i++)
+		insns[i] = PPC_INST_NOP;
+
+	/*
+	 * Set the first instruction as an unconditional trap. If
+	 * the last write to this address succeeds, this should
+	 * get overwritten by a no-op.
+	 */
+	insns[0] = PPC_INST_TRAP;
+
+	/*
+	 * Later, to jump to the executable region, we use a branch
+	 * and link instruction (bctrl) which sets the return address
+	 * automatically in LR. Use that to return back.
+	 */
+	insns[numinsns - 1] = PPC_INST_BLR;
+
+	/*
+	 * Pick the first instruction's address from the executable
+	 * region.
+	 */
+	fault_addr = insns;
+
+	/*
+	 * Read an instruction word from the address when the page
+	 * is execute only. This should generate an access fault.
+	 */
+	fault_code = -1;
+	remaining_faults = 1;
+	printf("Testing read on --x, should fault...");
+	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
+	i = *fault_addr;
+	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
+	printf("ok!\n");
+
+	/*
+	 * Write an instruction word to the address when the page
+	 * execute only. This should also generate an access fault.
+	 */
+	fault_code = -1;
+	remaining_faults = 1;
+	printf("Testing write on --x, should fault...");
+	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
+	*fault_addr = PPC_INST_NOP;
+	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
+	printf("ok!\n");
+
+	printf("Testing exec on ---, should fault...");
+	FAIL_IF(check_exec_fault(PROT_NONE));
+	printf("ok!\n");
+
+	printf("Testing exec on r--, should fault...");
+	FAIL_IF(check_exec_fault(PROT_READ));
+	printf("ok!\n");
+
+	printf("Testing exec on -w-, should fault...");
+	FAIL_IF(check_exec_fault(PROT_WRITE));
+	printf("ok!\n");
+
+	printf("Testing exec on rw-, should fault...");
+	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE));
+	printf("ok!\n");
+
+	printf("Testing exec on --x, should succeed...");
+	FAIL_IF(check_exec_fault(PROT_EXEC));
+	printf("ok!\n");
+
+	printf("Testing exec on r-x, should succeed...");
+	FAIL_IF(check_exec_fault(PROT_READ | PROT_EXEC));
+	printf("ok!\n");
+
+	printf("Testing exec on -wx, should succeed...");
+	FAIL_IF(check_exec_fault(PROT_WRITE | PROT_EXEC));
+	printf("ok!\n");
+
+	printf("Testing exec on rwx, should succeed...");
+	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE | PROT_EXEC));
+	printf("ok!\n");
+
+	/* Cleanup */
+	FAIL_IF(munmap((void *)insns, pgsize));
+
+	return 0;
+}
+
+int main(void)
+{
+	return test_harness(test, "exec_prot");
+}
-- 
2.37.2

Re: [PATCH v4 2/2] selftests/powerpc: Add a test for execute-only memory

[Christophe Leroy]

Le 17/08/2022 à 07:06, Russell Currey a écrit :
> From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> 
> This selftest is designed to cover execute-only protections
> on the Radix MMU but will also work with Hash.
> 
> The tests are based on those found in pkey_exec_test with modifications
> to use the generic mprotect() instead of the pkey variants.
> 
> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v4: new
> 
>   tools/testing/selftests/powerpc/mm/Makefile   |   3 +-
>   .../testing/selftests/powerpc/mm/exec_prot.c  | 231 ++++++++++++++++++

There is a lot of code in common with pkey_exec_prot.c
Isn't there a way to refactor ?

>   2 files changed, 233 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/powerpc/mm/exec_prot.c
> 
> diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
> index 27dc09d0bfee..19dd0b2ea397 100644
> --- a/tools/testing/selftests/powerpc/mm/Makefile
> +++ b/tools/testing/selftests/powerpc/mm/Makefile
> @@ -3,7 +3,7 @@ noarg:
>   	$(MAKE) -C ../
>   
>   TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
> -		  large_vm_fork_separation bad_accesses pkey_exec_prot \
> +		  large_vm_fork_separation bad_accesses exec_prot pkey_exec_prot \
>   		  pkey_siginfo stack_expansion_signal stack_expansion_ldst \
>   		  large_vm_gpr_corruption
>   TEST_PROGS := stress_code_patching.sh
> @@ -22,6 +22,7 @@ $(OUTPUT)/wild_bctr: CFLAGS += -m64
>   $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
>   $(OUTPUT)/large_vm_gpr_corruption: CFLAGS += -m64
>   $(OUTPUT)/bad_accesses: CFLAGS += -m64
> +$(OUTPUT)/exec_prot: CFLAGS += -m64
>   $(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
>   $(OUTPUT)/pkey_siginfo: CFLAGS += -m64
>   
> diff --git a/tools/testing/selftests/powerpc/mm/exec_prot.c b/tools/testing/selftests/powerpc/mm/exec_prot.c
> new file mode 100644
> index 000000000000..db75b2225de1
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mm/exec_prot.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2022, Nicholas Miehlbradt, IBM Corporation
> + * based on pkey_exec_prot.c
> + *
> + * Test if applying execute protection on pages works as expected.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <signal.h>
> +
> +#include <unistd.h>
> +#include <sys/mman.h>
> +
> +#include "pkeys.h"
> +
> +
> +#define PPC_INST_NOP	0x60000000
> +#define PPC_INST_TRAP	0x7fe00008
> +#define PPC_INST_BLR	0x4e800020
> +
> +static volatile sig_atomic_t fault_code;
> +static volatile sig_atomic_t remaining_faults;
> +static volatile unsigned int *fault_addr;
> +static unsigned long pgsize, numinsns;
> +static unsigned int *insns;
> +static bool pkeys_supported;
> +
> +static bool is_fault_expected(int fault_code)
> +{
> +	if (fault_code == SEGV_ACCERR)
> +		return true;
> +
> +	/* Assume any pkey error is fine since pkey_exec_prot test covers them */
> +	if (fault_code == SEGV_PKUERR && pkeys_supported)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void trap_handler(int signum, siginfo_t *sinfo, void *ctx)
> +{
> +	/* Check if this fault originated from the expected address */
> +	if (sinfo->si_addr != (void *)fault_addr)
> +		sigsafe_err("got a fault for an unexpected address\n");
> +
> +	_exit(1);
> +}
> +
> +static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
> +{
> +	fault_code = sinfo->si_code;
> +
> +	/* Check if this fault originated from the expected address */
> +	if (sinfo->si_addr != (void *)fault_addr) {
> +		sigsafe_err("got a fault for an unexpected address\n");
> +		_exit(1);
> +	}
> +
> +	/* Check if too many faults have occurred for a single test case */
> +	if (!remaining_faults) {
> +		sigsafe_err("got too many faults for the same address\n");
> +		_exit(1);
> +	}
> +
> +
> +	/* Restore permissions in order to continue */
> +	if (is_fault_expected(fault_code)) {
> +		if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE | PROT_EXEC)) {
> +			sigsafe_err("failed to set access permissions\n");
> +			_exit(1);
> +		}
> +	} else {
> +		sigsafe_err("got a fault with an unexpected code\n");
> +		_exit(1);
> +	}
> +
> +	remaining_faults--;
> +}
> +
> +static int check_exec_fault(int rights)
> +{
> +	/*
> +	 * Jump to the executable region.
> +	 *
> +	 * The first iteration also checks if the overwrite of the
> +	 * first instruction word from a trap to a no-op succeeded.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 0;
> +	if (!(rights & PROT_EXEC))
> +		remaining_faults = 1;
> +
> +	FAIL_IF(mprotect(insns, pgsize, rights) != 0);
> +	asm volatile("mtctr	%0; bctrl" : : "r"(insns));
> +
> +	FAIL_IF(remaining_faults != 0);
> +	if (!(rights & PROT_EXEC))
> +		FAIL_IF(!is_fault_expected(fault_code));
> +
> +	return 0;
> +}
> +
> +static int test(void)
> +{
> +	struct sigaction segv_act, trap_act;
> +	int i;
> +
> +	/* Skip the test if the CPU doesn't support Radix */
> +	SKIP_IF(!have_hwcap2(PPC_FEATURE2_ARCH_3_00));
> +
> +	/* Check if pkeys are supported */
> +	pkeys_supported = pkeys_unsupported() == 0;
> +
> +	/* Setup SIGSEGV handler */
> +	segv_act.sa_handler = 0;
> +	segv_act.sa_sigaction = segv_handler;
> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &segv_act.sa_mask) != 0);
> +	segv_act.sa_flags = SA_SIGINFO;
> +	segv_act.sa_restorer = 0;
> +	FAIL_IF(sigaction(SIGSEGV, &segv_act, NULL) != 0);
> +
> +	/* Setup SIGTRAP handler */
> +	trap_act.sa_handler = 0;
> +	trap_act.sa_sigaction = trap_handler;
> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &trap_act.sa_mask) != 0);
> +	trap_act.sa_flags = SA_SIGINFO;
> +	trap_act.sa_restorer = 0;
> +	FAIL_IF(sigaction(SIGTRAP, &trap_act, NULL) != 0);
> +
> +	/* Setup executable region */
> +	pgsize = getpagesize();
> +	numinsns = pgsize / sizeof(unsigned int);
> +	insns = (unsigned int *)mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
> +				      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	FAIL_IF(insns == MAP_FAILED);
> +
> +	/* Write the instruction words */
> +	for (i = 1; i < numinsns - 1; i++)
> +		insns[i] = PPC_INST_NOP;
> +
> +	/*
> +	 * Set the first instruction as an unconditional trap. If
> +	 * the last write to this address succeeds, this should
> +	 * get overwritten by a no-op.
> +	 */
> +	insns[0] = PPC_INST_TRAP;
> +
> +	/*
> +	 * Later, to jump to the executable region, we use a branch
> +	 * and link instruction (bctrl) which sets the return address
> +	 * automatically in LR. Use that to return back.
> +	 */
> +	insns[numinsns - 1] = PPC_INST_BLR;
> +
> +	/*
> +	 * Pick the first instruction's address from the executable
> +	 * region.
> +	 */
> +	fault_addr = insns;
> +
> +	/*
> +	 * Read an instruction word from the address when the page
> +	 * is execute only. This should generate an access fault.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 1;
> +	printf("Testing read on --x, should fault...");
> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
> +	i = *fault_addr;
> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
> +	printf("ok!\n");
> +
> +	/*
> +	 * Write an instruction word to the address when the page
> +	 * execute only. This should also generate an access fault.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 1;
> +	printf("Testing write on --x, should fault...");
> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
> +	*fault_addr = PPC_INST_NOP;
> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on ---, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_NONE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on r--, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_READ));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on -w-, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_WRITE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on rw-, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on --x, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on r-x, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on -wx, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_WRITE | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on rwx, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	/* Cleanup */
> +	FAIL_IF(munmap((void *)insns, pgsize));
> +
> +	return 0;
> +}
> +
> +int main(void)
> +{
> +	return test_harness(test, "exec_prot");
> +}

Re: [PATCH v4 2/2] selftests/powerpc: Add a test for execute-only memory

[Jordan Niethe]

On Wed, 2022-08-17 at 15:06 +1000, Russell Currey wrote:
> From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> 
> This selftest is designed to cover execute-only protections
> on the Radix MMU but will also work with Hash.
> 
> The tests are based on those found in pkey_exec_test with modifications
> to use the generic mprotect() instead of the pkey variants.

Would it make sense to rename pkey_exec_test to exec_test and have this test be apart of that?

> 
> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v4: new
> 
>  tools/testing/selftests/powerpc/mm/Makefile   |   3 +-
>  .../testing/selftests/powerpc/mm/exec_prot.c  | 231 ++++++++++++++++++
>  2 files changed, 233 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/powerpc/mm/exec_prot.c
> 
> diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
> index 27dc09d0bfee..19dd0b2ea397 100644
> --- a/tools/testing/selftests/powerpc/mm/Makefile
> +++ b/tools/testing/selftests/powerpc/mm/Makefile
> @@ -3,7 +3,7 @@ noarg:
>  	$(MAKE) -C ../
>  
>  TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
> -		  large_vm_fork_separation bad_accesses pkey_exec_prot \
> +		  large_vm_fork_separation bad_accesses exec_prot pkey_exec_prot \
>  		  pkey_siginfo stack_expansion_signal stack_expansion_ldst \
>  		  large_vm_gpr_corruption
>  TEST_PROGS := stress_code_patching.sh
> @@ -22,6 +22,7 @@ $(OUTPUT)/wild_bctr: CFLAGS += -m64
>  $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
>  $(OUTPUT)/large_vm_gpr_corruption: CFLAGS += -m64
>  $(OUTPUT)/bad_accesses: CFLAGS += -m64
> +$(OUTPUT)/exec_prot: CFLAGS += -m64
>  $(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
>  $(OUTPUT)/pkey_siginfo: CFLAGS += -m64
>  
> diff --git a/tools/testing/selftests/powerpc/mm/exec_prot.c b/tools/testing/selftests/powerpc/mm/exec_prot.c
> new file mode 100644
> index 000000000000..db75b2225de1
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mm/exec_prot.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright 2022, Nicholas Miehlbradt, IBM Corporation
> + * based on pkey_exec_prot.c
> + *
> + * Test if applying execute protection on pages works as expected.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <signal.h>
> +
> +#include <unistd.h>
> +#include <sys/mman.h>
> +
> +#include "pkeys.h"
> +
> +
> +#define PPC_INST_NOP	0x60000000
> +#define PPC_INST_TRAP	0x7fe00008
> +#define PPC_INST_BLR	0x4e800020
> +
> +static volatile sig_atomic_t fault_code;
> +static volatile sig_atomic_t remaining_faults;
> +static volatile unsigned int *fault_addr;
> +static unsigned long pgsize, numinsns;
> +static unsigned int *insns;
> +static bool pkeys_supported;
> +
> +static bool is_fault_expected(int fault_code)
> +{
> +	if (fault_code == SEGV_ACCERR)
> +		return true;
> +
> +	/* Assume any pkey error is fine since pkey_exec_prot test covers them */
> +	if (fault_code == SEGV_PKUERR && pkeys_supported)
> +		return true;
> +
> +	return false;
> +}
> +
> +static void trap_handler(int signum, siginfo_t *sinfo, void *ctx)
> +{
> +	/* Check if this fault originated from the expected address */
> +	if (sinfo->si_addr != (void *)fault_addr)
> +		sigsafe_err("got a fault for an unexpected address\n");
> +
> +	_exit(1);
> +}
> +
> +static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
> +{
> +	fault_code = sinfo->si_code;
> +
> +	/* Check if this fault originated from the expected address */
> +	if (sinfo->si_addr != (void *)fault_addr) {
> +		sigsafe_err("got a fault for an unexpected address\n");
> +		_exit(1);
> +	}
> +
> +	/* Check if too many faults have occurred for a single test case */
> +	if (!remaining_faults) {
> +		sigsafe_err("got too many faults for the same address\n");
> +		_exit(1);
> +	}
> +
> +
> +	/* Restore permissions in order to continue */
> +	if (is_fault_expected(fault_code)) {
> +		if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE | PROT_EXEC)) {
> +			sigsafe_err("failed to set access permissions\n");
> +			_exit(1);
> +		}
> +	} else {
> +		sigsafe_err("got a fault with an unexpected code\n");
> +		_exit(1);
> +	}
> +
> +	remaining_faults--;
> +}
> +
> +static int check_exec_fault(int rights)
> +{
> +	/*
> +	 * Jump to the executable region.
> +	 *
> +	 * The first iteration also checks if the overwrite of the
> +	 * first instruction word from a trap to a no-op succeeded.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 0;
> +	if (!(rights & PROT_EXEC))
> +		remaining_faults = 1;
> +
> +	FAIL_IF(mprotect(insns, pgsize, rights) != 0);
> +	asm volatile("mtctr	%0; bctrl" : : "r"(insns));
> +
> +	FAIL_IF(remaining_faults != 0);
> +	if (!(rights & PROT_EXEC))
> +		FAIL_IF(!is_fault_expected(fault_code));
> +
> +	return 0;
> +}
> +
> +static int test(void)
> +{
> +	struct sigaction segv_act, trap_act;
> +	int i;
> +
> +	/* Skip the test if the CPU doesn't support Radix */
> +	SKIP_IF(!have_hwcap2(PPC_FEATURE2_ARCH_3_00));
> +
> +	/* Check if pkeys are supported */
> +	pkeys_supported = pkeys_unsupported() == 0;
> +
> +	/* Setup SIGSEGV handler */
> +	segv_act.sa_handler = 0;
> +	segv_act.sa_sigaction = segv_handler;
> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &segv_act.sa_mask) != 0);
> +	segv_act.sa_flags = SA_SIGINFO;
> +	segv_act.sa_restorer = 0;
> +	FAIL_IF(sigaction(SIGSEGV, &segv_act, NULL) != 0);
> +
> +	/* Setup SIGTRAP handler */
> +	trap_act.sa_handler = 0;
> +	trap_act.sa_sigaction = trap_handler;
> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &trap_act.sa_mask) != 0);
> +	trap_act.sa_flags = SA_SIGINFO;
> +	trap_act.sa_restorer = 0;
> +	FAIL_IF(sigaction(SIGTRAP, &trap_act, NULL) != 0);
> +
> +	/* Setup executable region */
> +	pgsize = getpagesize();
> +	numinsns = pgsize / sizeof(unsigned int);
> +	insns = (unsigned int *)mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
> +				      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +	FAIL_IF(insns == MAP_FAILED);
> +
> +	/* Write the instruction words */
> +	for (i = 1; i < numinsns - 1; i++)
> +		insns[i] = PPC_INST_NOP;
> +
> +	/*
> +	 * Set the first instruction as an unconditional trap. If
> +	 * the last write to this address succeeds, this should
> +	 * get overwritten by a no-op.
> +	 */
> +	insns[0] = PPC_INST_TRAP;
> +
> +	/*
> +	 * Later, to jump to the executable region, we use a branch
> +	 * and link instruction (bctrl) which sets the return address
> +	 * automatically in LR. Use that to return back.
> +	 */
> +	insns[numinsns - 1] = PPC_INST_BLR;
> +
> +	/*
> +	 * Pick the first instruction's address from the executable
> +	 * region.
> +	 */
> +	fault_addr = insns;
> +
> +	/*
> +	 * Read an instruction word from the address when the page
> +	 * is execute only. This should generate an access fault.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 1;
> +	printf("Testing read on --x, should fault...");
> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
> +	i = *fault_addr;
> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
> +	printf("ok!\n");
> +
> +	/*
> +	 * Write an instruction word to the address when the page
> +	 * execute only. This should also generate an access fault.
> +	 */
> +	fault_code = -1;
> +	remaining_faults = 1;
> +	printf("Testing write on --x, should fault...");
> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
> +	*fault_addr = PPC_INST_NOP;
> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on ---, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_NONE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on r--, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_READ));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on -w-, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_WRITE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on rw-, should fault...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on --x, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on r-x, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on -wx, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_WRITE | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	printf("Testing exec on rwx, should succeed...");
> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE | PROT_EXEC));
> +	printf("ok!\n");
> +
> +	/* Cleanup */
> +	FAIL_IF(munmap((void *)insns, pgsize));
> +
> +	return 0;
> +}
> +
> +int main(void)
> +{
> +	return test_harness(test, "exec_prot");
> +}

Re: [PATCH v4 2/2] selftests/powerpc: Add a test for execute-only memory

[Nicholas Miehlbradt]

On 17/8/2022 4:15 pm, Jordan Niethe wrote:
> On Wed, 2022-08-17 at 15:06 +1000, Russell Currey wrote:
>> From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
>>
>> This selftest is designed to cover execute-only protections
>> on the Radix MMU but will also work with Hash.
>>
>> The tests are based on those found in pkey_exec_test with modifications
>> to use the generic mprotect() instead of the pkey variants.
> 
> Would it make sense to rename pkey_exec_test to exec_test and have this test be apart of that?
> 
I think might make it unnecessarily complex. The checks needed when 
testing with pkeys would mean that it would be necessary to check if 
pkeys are enabled and choose which set of tests to run depending on the 
result. The differences are substantial enough that it would be 
challenging to combine them into a single set of tests.

>>
>> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> ---
>> v4: new
>>
>>   tools/testing/selftests/powerpc/mm/Makefile   |   3 +-
>>   .../testing/selftests/powerpc/mm/exec_prot.c  | 231 ++++++++++++++++++
>>   2 files changed, 233 insertions(+), 1 deletion(-)
>>   create mode 100644 tools/testing/selftests/powerpc/mm/exec_prot.c
>>
>> diff --git a/tools/testing/selftests/powerpc/mm/Makefile b/tools/testing/selftests/powerpc/mm/Makefile
>> index 27dc09d0bfee..19dd0b2ea397 100644
>> --- a/tools/testing/selftests/powerpc/mm/Makefile
>> +++ b/tools/testing/selftests/powerpc/mm/Makefile
>> @@ -3,7 +3,7 @@ noarg:
>>   	$(MAKE) -C ../
>>   
>>   TEST_GEN_PROGS := hugetlb_vs_thp_test subpage_prot prot_sao segv_errors wild_bctr \
>> -		  large_vm_fork_separation bad_accesses pkey_exec_prot \
>> +		  large_vm_fork_separation bad_accesses exec_prot pkey_exec_prot \
>>   		  pkey_siginfo stack_expansion_signal stack_expansion_ldst \
>>   		  large_vm_gpr_corruption
>>   TEST_PROGS := stress_code_patching.sh
>> @@ -22,6 +22,7 @@ $(OUTPUT)/wild_bctr: CFLAGS += -m64
>>   $(OUTPUT)/large_vm_fork_separation: CFLAGS += -m64
>>   $(OUTPUT)/large_vm_gpr_corruption: CFLAGS += -m64
>>   $(OUTPUT)/bad_accesses: CFLAGS += -m64
>> +$(OUTPUT)/exec_prot: CFLAGS += -m64
>>   $(OUTPUT)/pkey_exec_prot: CFLAGS += -m64
>>   $(OUTPUT)/pkey_siginfo: CFLAGS += -m64
>>   
>> diff --git a/tools/testing/selftests/powerpc/mm/exec_prot.c b/tools/testing/selftests/powerpc/mm/exec_prot.c
>> new file mode 100644
>> index 000000000000..db75b2225de1
>> --- /dev/null
>> +++ b/tools/testing/selftests/powerpc/mm/exec_prot.c
>> @@ -0,0 +1,231 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +/*
>> + * Copyright 2022, Nicholas Miehlbradt, IBM Corporation
>> + * based on pkey_exec_prot.c
>> + *
>> + * Test if applying execute protection on pages works as expected.
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <signal.h>
>> +
>> +#include <unistd.h>
>> +#include <sys/mman.h>
>> +
>> +#include "pkeys.h"
>> +
>> +
>> +#define PPC_INST_NOP	0x60000000
>> +#define PPC_INST_TRAP	0x7fe00008
>> +#define PPC_INST_BLR	0x4e800020
>> +
>> +static volatile sig_atomic_t fault_code;
>> +static volatile sig_atomic_t remaining_faults;
>> +static volatile unsigned int *fault_addr;
>> +static unsigned long pgsize, numinsns;
>> +static unsigned int *insns;
>> +static bool pkeys_supported;
>> +
>> +static bool is_fault_expected(int fault_code)
>> +{
>> +	if (fault_code == SEGV_ACCERR)
>> +		return true;
>> +
>> +	/* Assume any pkey error is fine since pkey_exec_prot test covers them */
>> +	if (fault_code == SEGV_PKUERR && pkeys_supported)
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +static void trap_handler(int signum, siginfo_t *sinfo, void *ctx)
>> +{
>> +	/* Check if this fault originated from the expected address */
>> +	if (sinfo->si_addr != (void *)fault_addr)
>> +		sigsafe_err("got a fault for an unexpected address\n");
>> +
>> +	_exit(1);
>> +}
>> +
>> +static void segv_handler(int signum, siginfo_t *sinfo, void *ctx)
>> +{
>> +	fault_code = sinfo->si_code;
>> +
>> +	/* Check if this fault originated from the expected address */
>> +	if (sinfo->si_addr != (void *)fault_addr) {
>> +		sigsafe_err("got a fault for an unexpected address\n");
>> +		_exit(1);
>> +	}
>> +
>> +	/* Check if too many faults have occurred for a single test case */
>> +	if (!remaining_faults) {
>> +		sigsafe_err("got too many faults for the same address\n");
>> +		_exit(1);
>> +	}
>> +
>> +
>> +	/* Restore permissions in order to continue */
>> +	if (is_fault_expected(fault_code)) {
>> +		if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE | PROT_EXEC)) {
>> +			sigsafe_err("failed to set access permissions\n");
>> +			_exit(1);
>> +		}
>> +	} else {
>> +		sigsafe_err("got a fault with an unexpected code\n");
>> +		_exit(1);
>> +	}
>> +
>> +	remaining_faults--;
>> +}
>> +
>> +static int check_exec_fault(int rights)
>> +{
>> +	/*
>> +	 * Jump to the executable region.
>> +	 *
>> +	 * The first iteration also checks if the overwrite of the
>> +	 * first instruction word from a trap to a no-op succeeded.
>> +	 */
>> +	fault_code = -1;
>> +	remaining_faults = 0;
>> +	if (!(rights & PROT_EXEC))
>> +		remaining_faults = 1;
>> +
>> +	FAIL_IF(mprotect(insns, pgsize, rights) != 0);
>> +	asm volatile("mtctr	%0; bctrl" : : "r"(insns));
>> +
>> +	FAIL_IF(remaining_faults != 0);
>> +	if (!(rights & PROT_EXEC))
>> +		FAIL_IF(!is_fault_expected(fault_code));
>> +
>> +	return 0;
>> +}
>> +
>> +static int test(void)
>> +{
>> +	struct sigaction segv_act, trap_act;
>> +	int i;
>> +
>> +	/* Skip the test if the CPU doesn't support Radix */
>> +	SKIP_IF(!have_hwcap2(PPC_FEATURE2_ARCH_3_00));
>> +
>> +	/* Check if pkeys are supported */
>> +	pkeys_supported = pkeys_unsupported() == 0;
>> +
>> +	/* Setup SIGSEGV handler */
>> +	segv_act.sa_handler = 0;
>> +	segv_act.sa_sigaction = segv_handler;
>> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &segv_act.sa_mask) != 0);
>> +	segv_act.sa_flags = SA_SIGINFO;
>> +	segv_act.sa_restorer = 0;
>> +	FAIL_IF(sigaction(SIGSEGV, &segv_act, NULL) != 0);
>> +
>> +	/* Setup SIGTRAP handler */
>> +	trap_act.sa_handler = 0;
>> +	trap_act.sa_sigaction = trap_handler;
>> +	FAIL_IF(sigprocmask(SIG_SETMASK, 0, &trap_act.sa_mask) != 0);
>> +	trap_act.sa_flags = SA_SIGINFO;
>> +	trap_act.sa_restorer = 0;
>> +	FAIL_IF(sigaction(SIGTRAP, &trap_act, NULL) != 0);
>> +
>> +	/* Setup executable region */
>> +	pgsize = getpagesize();
>> +	numinsns = pgsize / sizeof(unsigned int);
>> +	insns = (unsigned int *)mmap(NULL, pgsize, PROT_READ | PROT_WRITE,
>> +				      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +	FAIL_IF(insns == MAP_FAILED);
>> +
>> +	/* Write the instruction words */
>> +	for (i = 1; i < numinsns - 1; i++)
>> +		insns[i] = PPC_INST_NOP;
>> +
>> +	/*
>> +	 * Set the first instruction as an unconditional trap. If
>> +	 * the last write to this address succeeds, this should
>> +	 * get overwritten by a no-op.
>> +	 */
>> +	insns[0] = PPC_INST_TRAP;
>> +
>> +	/*
>> +	 * Later, to jump to the executable region, we use a branch
>> +	 * and link instruction (bctrl) which sets the return address
>> +	 * automatically in LR. Use that to return back.
>> +	 */
>> +	insns[numinsns - 1] = PPC_INST_BLR;
>> +
>> +	/*
>> +	 * Pick the first instruction's address from the executable
>> +	 * region.
>> +	 */
>> +	fault_addr = insns;
>> +
>> +	/*
>> +	 * Read an instruction word from the address when the page
>> +	 * is execute only. This should generate an access fault.
>> +	 */
>> +	fault_code = -1;
>> +	remaining_faults = 1;
>> +	printf("Testing read on --x, should fault...");
>> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
>> +	i = *fault_addr;
>> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
>> +	printf("ok!\n");
>> +
>> +	/*
>> +	 * Write an instruction word to the address when the page
>> +	 * execute only. This should also generate an access fault.
>> +	 */
>> +	fault_code = -1;
>> +	remaining_faults = 1;
>> +	printf("Testing write on --x, should fault...");
>> +	FAIL_IF(mprotect(insns, pgsize, PROT_EXEC) != 0);
>> +	*fault_addr = PPC_INST_NOP;
>> +	FAIL_IF(remaining_faults != 0 || !is_fault_expected(fault_code));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on ---, should fault...");
>> +	FAIL_IF(check_exec_fault(PROT_NONE));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on r--, should fault...");
>> +	FAIL_IF(check_exec_fault(PROT_READ));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on -w-, should fault...");
>> +	FAIL_IF(check_exec_fault(PROT_WRITE));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on rw-, should fault...");
>> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on --x, should succeed...");
>> +	FAIL_IF(check_exec_fault(PROT_EXEC));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on r-x, should succeed...");
>> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_EXEC));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on -wx, should succeed...");
>> +	FAIL_IF(check_exec_fault(PROT_WRITE | PROT_EXEC));
>> +	printf("ok!\n");
>> +
>> +	printf("Testing exec on rwx, should succeed...");
>> +	FAIL_IF(check_exec_fault(PROT_READ | PROT_WRITE | PROT_EXEC));
>> +	printf("ok!\n");
>> +
>> +	/* Cleanup */
>> +	FAIL_IF(munmap((void *)insns, pgsize));
>> +
>> +	return 0;
>> +}
>> +
>> +int main(void)
>> +{
>> +	return test_harness(test, "exec_prot");
>> +}
> 

Re: [PATCH v4 2/2] selftests/powerpc: Add a test for execute-only memory

[Michael Ellerman]

Nicholas Miehlbradt <nicholas@linux.ibm.com> writes:
> On 17/8/2022 4:15 pm, Jordan Niethe wrote:
>> On Wed, 2022-08-17 at 15:06 +1000, Russell Currey wrote:
>>> From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
>>>
>>> This selftest is designed to cover execute-only protections
>>> on the Radix MMU but will also work with Hash.
>>>
>>> The tests are based on those found in pkey_exec_test with modifications
>>> to use the generic mprotect() instead of the pkey variants.
>> 
>> Would it make sense to rename pkey_exec_test to exec_test and have this test be apart of that?
>> 
> I think might make it unnecessarily complex. The checks needed when 
> testing with pkeys would mean that it would be necessary to check if 
> pkeys are enabled and choose which set of tests to run depending on the 
> result. The differences are substantial enough that it would be 
> challenging to combine them into a single set of tests.

Yeah I think I agree. Having each test stand on its own is nice for
debugging also.

In general I'm less bothered about code duplication in tests. We should
try and share code where we can, but it's more important that we have
tests at all, rather than blocking new tests because they duplicate some
code from another test.

So I'm inclined to merge this as-is, we can always refactor it to share
code in future if we think there's enough commonality to warrant it.

cheers

Re: [PATCH v4 1/2] powerpc/mm: Support execute-only memory on the Radix MMU

[Michael Ellerman]

On Wed, 17 Aug 2022 15:06:39 +1000, Russell Currey wrote:
> Add support for execute-only memory (XOM) for the Radix MMU by using an
> execute-only mapping, as opposed to the RX mapping used by powerpc's
> other MMUs.
> 
> The Hash MMU already supports XOM through the execute-only pkey,
> which is a separate mechanism shared with x86.  A PROT_EXEC-only mapping
> will map to RX, and then the pkey will be applied on top of it.
> 
> [...]

Applied to powerpc/next.

[1/2] powerpc/mm: Support execute-only memory on the Radix MMU
      https://git.kernel.org/powerpc/c/395cac7752b905318ae454a8b859d4c190485510
[2/2] selftests/powerpc: Add a test for execute-only memory
      https://git.kernel.org/powerpc/c/98acee3f8db451eaab9fbd422e523c228aacf08c

cheers

Re: [PATCH v4 1/2] powerpc/mm: Support execute-only memory on the Radix MMU

[Michal Suchánek]

Hello,

On Wed, Aug 31, 2022 at 11:13:59PM +1000, Michael Ellerman wrote:
> On Wed, 17 Aug 2022 15:06:39 +1000, Russell Currey wrote:
> > Add support for execute-only memory (XOM) for the Radix MMU by using an
> > execute-only mapping, as opposed to the RX mapping used by powerpc's
> > other MMUs.
> > 
> > The Hash MMU already supports XOM through the execute-only pkey,
> > which is a separate mechanism shared with x86.  A PROT_EXEC-only mapping
> > will map to RX, and then the pkey will be applied on top of it.
> > 
> > [...]
> 
> Applied to powerpc/next.
> 
> [1/2] powerpc/mm: Support execute-only memory on the Radix MMU
>       https://git.kernel.org/powerpc/c/395cac7752b905318ae454a8b859d4c190485510

This breaks libaio tests (on POWER9 hash PowerVM):
https://pagure.io/libaio/blob/master/f/harness/cases/5.t#_43

cases/5.p
expect   512: (w), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   -14: (r), res =   -14 [Bad address]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
test cases/5.t completed PASSED.

cases/5.p
expect   512: (w), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (r), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   512: (w), res =   512 [Success]
expect   -14: (r), res =   -14 [Bad address]
expect   512: (r), res =   512 [Success]
expect   -14: (w), res =   512 [Success] -- FAILED
test cases/5.t completed FAILED.

Can you have a look if that test assumption is OK?

Thanks

Michal

Re: [PATCH v4 1/2] powerpc/mm: Support execute-only memory on the Radix MMU

[Russell Currey]

On Wed, 2023-03-08 at 16:27 +0100, Michal Suchánek wrote:
> Hello,
> 
> On Wed, Aug 31, 2022 at 11:13:59PM +1000, Michael Ellerman wrote:
> > On Wed, 17 Aug 2022 15:06:39 +1000, Russell Currey wrote:
> > > Add support for execute-only memory (XOM) for the Radix MMU by
> > > using an
> > > execute-only mapping, as opposed to the RX mapping used by
> > > powerpc's
> > > other MMUs.
> > > 
> > > The Hash MMU already supports XOM through the execute-only pkey,
> > > which is a separate mechanism shared with x86.  A PROT_EXEC-only
> > > mapping
> > > will map to RX, and then the pkey will be applied on top of it.
> > > 
> > > [...]
> > 
> > Applied to powerpc/next.
> > 
> > [1/2] powerpc/mm: Support execute-only memory on the Radix MMU
> >      
> > https://git.kernel.org/powerpc/c/395cac7752b905318ae454a8b859d4c190485510
> 
> This breaks libaio tests (on POWER9 hash PowerVM):
> https://pagure.io/libaio/blob/master/f/harness/cases/5.t#_43
> 
> cases/5.p
> expect   512: (w), res =   512 [Success]
> expect   512: (r), res =   512 [Success]
> expect   512: (r), res =   512 [Success]
> expect   512: (w), res =   512 [Success]
> expect   512: (w), res =   512 [Success]
> expect   -14: (r), res =   -14 [Bad address]
> expect   512: (r), res =   512 [Success]
> expect   512: (w), res =   512 [Success]
> test cases/5.t completed PASSED.
> 
> cases/5.p
> expect   512: (w), res =   512 [Success]
> expect   512: (r), res =   512 [Success]
> expect   512: (r), res =   512 [Success]
> expect   512: (w), res =   512 [Success]
> expect   512: (w), res =   512 [Success]
> expect   -14: (r), res =   -14 [Bad address]
> expect   512: (r), res =   512 [Success]
> expect   -14: (w), res =   512 [Success] -- FAILED
> test cases/5.t completed FAILED.
> 
> Can you have a look if that test assumption is OK?

Hi Michal, thanks for the report.

This wasn't an intended behaviour change, so it is a bug.  I have no
idea why we hit the fault in write() but not in io_submit(), though. 
The same issue applies under Radix.

What's happening here is that we're taking a page fault and calling
into access_error() and returning true when we shouldn't.  Previously
we didn't check for read faults and only checked for PROT_NONE.  My
patch checks the vma flags to see if they lack VM_READ after we check
for exec and write, which ignores that VM_WRITE implies read 

This means we're mishandling faults for write-only mappings by assuming
that the lack of VM_READ means we're faulting from read, when that
should only be possible under a PROT_EXEC-only mapping.

I think the correct behaviour is

	if (unlikely(!(vma->vm_flags & (VM_READ | VM_WRITE))))

in access_error().

Will do some more testing and send a patch soon.  I also need to verify
that write implying read is true for all powerpc platforms.

- Russell

> 
> Thanks
> 
> Michal