Re: [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU

Christophe Leroy [off-list ref] writes:

Le 08/03/2019 à 02:16, Michael Ellerman a écrit :

...

quoted

diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
new file mode 100644
index 000000000000..3d60b04fc3f6
--- /dev/null
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h

@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
+#define _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H
+
+#include <linux/const.h>
+
+#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
+#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
+#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
+#define AMR_KUAP_SHIFT		62
+
+#ifdef __ASSEMBLY__
+
+.macro kuap_restore_amr	gpr

What about calling it just kuap_restore (kuap_check and 
kuap_save_and_lock) , for the day we add an different implementation for 
non RADIX ?

I don't think we have any plan to use anything other than AMR. We can
always rename them if we do.

quoted

+#ifdef CONFIG_PPC_KUAP
+	BEGIN_MMU_FTR_SECTION_NESTED(67)
+	ld	\gpr, STACK_REGS_KUAP(r1)
+	mtspr	SPRN_AMR, \gpr
+	END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+#endif
+.endm
+
+.macro kuap_check_amr gpr1 gpr2

What about using .macro kuap_check_amr gpr1, gpr2 instead to have a 
standard form with a ',' like kuap_save_amr_and_lock

Yep, that was not intentional.

quoted

+#ifdef CONFIG_PPC_KUAP_DEBUG
+	BEGIN_MMU_FTR_SECTION_NESTED(67)
+	mfspr	\gpr1, SPRN_AMR
+	li	\gpr2, (AMR_KUAP_BLOCKED >> AMR_KUAP_SHIFT)
+	sldi	\gpr2, \gpr2, AMR_KUAP_SHIFT
+999:	tdne	\gpr1, \gpr2
+	EMIT_BUG_ENTRY 999b,__FILE__,__LINE__, \
+		(BUGFLAG_WARNING|BUGFLAG_ONCE)

This should fit on a single line.
Also add space after , and around |

Yep, copy/pasted from elsewhere.

quoted

+.macro kuap_save_amr_and_lock gpr1, gpr2, use_cr, msr_pr_cr
+#ifdef CONFIG_PPC_KUAP
+	BEGIN_MMU_FTR_SECTION_NESTED(67)
+	.ifnb \msr_pr_cr
+	bne	\msr_pr_cr, 99f
+	.endif
+	mfspr	\gpr1, SPRN_AMR
+	std	\gpr1, STACK_REGS_KUAP(r1)
+	li	\gpr2, (AMR_KUAP_BLOCKED >> AMR_KUAP_SHIFT)
+	sldi	\gpr2, \gpr2, AMR_KUAP_SHIFT
+	cmpd	\use_cr, \gpr1, \gpr2

On ppc32, I would have used rlwinm. to do the test. Is there an 
equivalent rldinm. (unless we need to preserve cr0) ?

I prefer to have the CR field specified, so we don't accidentally
clobber cr0 in some path where it is live.

quoted

+	beq	\use_cr, 99f
+	// We don't isync here because we very recently entered via rfid

Is this form of comment acceptable now ? It used to be /* */ only.

Yeah it is, at least in powerpc code :)

quoted

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 937bb630093f..bef4e05a6823 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h

@@ -497,6 +497,7 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
  	RESTORE_CTR(r1, area);						   \
  	b	bad_stack;						   \
  3:	EXCEPTION_PROLOG_COMMON_1();					   \
+	kuap_save_amr_and_lock r9, r10, cr1, cr0;			   \

What about moving this to 4f, to avoid the cr test and branch in 
kuap_save_amr_and_lock  ?

Moving it there wouldn't help, because user & kernel entry both go
through 4f. So we'd still need to check if we're coming from user.

Or did I misunderstand?

quoted

  	beq	4f;			/* if from kernel mode		*/ \
  	ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);				   \
  	SAVE_PPR(area, r9);						   \

quoted

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 224bcd4be5ae..0c87832ddd6c 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c

@@ -553,6 +554,23 @@ void __init setup_kuep(bool disabled)
  }
  #endif
  
+#ifdef CONFIG_PPC_KUAP
+void __init setup_kuap(bool disabled)
+{
+	if (disabled || !early_radix_enabled())
+		return;
+
+	if (smp_processor_id() == boot_cpuid) {
+		pr_info("Activating Kernel Userspace Access Prevention\n");
+		cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KUAP;
+	}
+
+	/* Make sure userspace can't change the AMR */
+	mtspr(SPRN_UAMOR, 0);
+	mtspr(SPRN_AMR, AMR_KUAP_BLOCKED);

No isync() needed here ?

Will add.

quoted

diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 60371784c9f1..5e53b9fd62aa 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype

@@ -370,6 +371,13 @@ config PPC_KUAP
  
  	  If you're unsure, say Y.
  
+config PPC_KUAP_DEBUG
+	bool "Extra debugging for Kernel Userspace Access Protection"
+	depends on PPC_HAVE_KUAP && PPC_RADIX_MMU

Why only PPC_RADIX_MMU ?

Because it doesn't do anything useful unless radix is enabled.

We can remove it when another platform wants it.

Thanks for the review.

cheers