[PATCH 1/4] powerpc/kuap: Avoid unnecessary reads of MD_AP

[PATCH 1/4] powerpc/kuap: Avoid unnecessary reads of MD_AP

[Christophe Leroy]

A disassembly of interrupt_exit_kernel_prepare() shows a useless read
of MD_AP register. This is shown by r9 being re-used immediately without
doing anything with the value read.

  c000e0e0:       60 00 00 00     nop
  c000e0e4: ===>  7d 3a c2 a6     mfmd_ap r9	<====
  c000e0e8:       7d 20 00 a6     mfmsr   r9
  c000e0ec:       7c 51 13 a6     mtspr   81,r2
  c000e0f0:       81 3f 00 84     lwz     r9,132(r31)
  c000e0f4:       71 29 80 00     andi.   r9,r9,32768

kuap_get_and_assert_locked() is paired with kuap_kernel_restore()
and are only used in interrupt_exit_kernel_prepare(). The value
returned by kuap_get_and_assert_locked() is only used by
kuap_kernel_restore().

On 8xx, kuap_kernel_restore() doesn't use the value read by
kuap_get_and_assert_locked() so modify kuap_get_and_assert_locked()
to not perform the read of MD_AP and return 0 instead.

The same applies on BOOKE.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/nohash/32/kup-8xx.h | 8 ++------
 arch/powerpc/include/asm/nohash/kup-booke.h  | 6 ++----
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index c44d97751723..8579210f2a6a 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -41,14 +41,10 @@ static inline void __kuap_kernel_restore(struct pt_regs *regs, unsigned long kua
 
 static inline unsigned long __kuap_get_and_assert_locked(void)
 {
-	unsigned long kuap;
-
-	kuap = mfspr(SPRN_MD_AP);
-
 	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG))
-		WARN_ON_ONCE(kuap >> 16 != MD_APG_KUAP >> 16);
+		WARN_ON_ONCE(mfspr(SPRN_MD_AP) >> 16 != MD_APG_KUAP >> 16);
 
-	return kuap;
+	return 0;
 }
 
 static inline void __allow_user_access(void __user *to, const void __user *from,
diff --git a/arch/powerpc/include/asm/nohash/kup-booke.h b/arch/powerpc/include/asm/nohash/kup-booke.h
index 49bb41ed0816..823c5a3a96d8 100644
--- a/arch/powerpc/include/asm/nohash/kup-booke.h
+++ b/arch/powerpc/include/asm/nohash/kup-booke.h
@@ -58,12 +58,10 @@ static inline void __kuap_kernel_restore(struct pt_regs *regs, unsigned long kua
 
 static inline unsigned long __kuap_get_and_assert_locked(void)
 {
-	unsigned long kuap = mfspr(SPRN_PID);
-
 	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG))
-		WARN_ON_ONCE(kuap);
+		WARN_ON_ONCE(mfspr(SPRN_PID));
 
-	return kuap;
+	return 0;
 }
 
 static inline void __allow_user_access(void __user *to, const void __user *from,
-- 
2.40.1

[PATCH 2/4] powerpc/kuap: Avoid useless jump_label on empty function

[Christophe Leroy]

Disassembly of interrupt_enter_prepare() shows a pointless nop before the mftb

  c000abf0 <interrupt_enter_prepare>:
  c000abf0:       81 23 00 84     lwz     r9,132(r3)
  c000abf4:       71 29 40 00     andi.   r9,r9,16384
  c000abf8:       41 82 00 28     beq-    c000ac20 <interrupt_enter_prepare+0x30>
  c000abfc: ===>  60 00 00 00     nop	<====
  c000ac00:       7d 0c 42 e6     mftb    r8
  c000ac04:       80 e2 00 08     lwz     r7,8(r2)
  c000ac08:       81 22 00 28     lwz     r9,40(r2)
  c000ac0c:       91 02 00 24     stw     r8,36(r2)
  c000ac10:       7d 29 38 50     subf    r9,r9,r7
  c000ac14:       7d 29 42 14     add     r9,r9,r8
  c000ac18:       91 22 00 08     stw     r9,8(r2)
  c000ac1c:       4e 80 00 20     blr
  c000ac20:       60 00 00 00     nop
  c000ac24:       7d 5a c2 a6     mfmd_ap r10
  c000ac28:       3d 20 de 00     lis     r9,-8704
  c000ac2c:       91 43 00 b0     stw     r10,176(r3)
  c000ac30:       7d 3a c3 a6     mtspr   794,r9
  c000ac34:       4e 80 00 20     blr

That comes from the call to kuap_loc(), allthough __kuap_lock() is an empty
function on the 8xx.

To avoid that, only perform kuap_is_disabled() check when there is something
to do with __kuap_lock().

Do the same with __kuap_save_and_lock() and __kuap_get_and_assert_locked().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/book3s/32/kup.h     |  6 ++---
 arch/powerpc/include/asm/book3s/64/kup.h     | 10 +-------
 arch/powerpc/include/asm/kup.h               | 25 ++++++++++----------
 arch/powerpc/include/asm/nohash/32/kup-8xx.h | 11 ++++-----
 arch/powerpc/include/asm/nohash/kup-booke.h  |  8 +++++--
 5 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 678f9c9d89b6..466a19cfb4df 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -77,10 +77,6 @@ static inline void kuap_unlock(unsigned long addr, bool ool)
 		kuap_unlock_all_ool();
 }
 
-static inline void __kuap_lock(void)
-{
-}
-
 static inline void __kuap_save_and_lock(struct pt_regs *regs)
 {
 	unsigned long kuap = current->thread.kuap;
@@ -92,6 +88,7 @@ static inline void __kuap_save_and_lock(struct pt_regs *regs)
 	current->thread.kuap = KUAP_NONE;
 	kuap_lock_addr(kuap, false);
 }
+#define __kuap_save_and_lock __kuap_save_and_lock
 
 static inline void kuap_user_restore(struct pt_regs *regs)
 {
@@ -120,6 +117,7 @@ static inline unsigned long __kuap_get_and_assert_locked(void)
 
 	return kuap;
 }
+#define __kuap_get_and_assert_locked __kuap_get_and_assert_locked
 
 static __always_inline void __allow_user_access(void __user *to, const void __user *from,
 						u32 size, unsigned long dir)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index 54cf46808157..1b0215ff3710 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -297,15 +297,7 @@ static inline unsigned long __kuap_get_and_assert_locked(void)
 		WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
 	return amr;
 }
-
-/* Do nothing, book3s/64 does that in ASM */
-static inline void __kuap_lock(void)
-{
-}
-
-static inline void __kuap_save_and_lock(struct pt_regs *regs)
-{
-}
+#define __kuap_get_and_assert_locked __kuap_get_and_assert_locked
 
 /*
  * We support individually allowing read or write, but we don't support nesting
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index d751ddd08110..0a4e07175612 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -52,16 +52,9 @@ __bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
 	return false;
 }
 
-static inline void __kuap_lock(void) { }
-static inline void __kuap_save_and_lock(struct pt_regs *regs) { }
 static inline void kuap_user_restore(struct pt_regs *regs) { }
 static inline void __kuap_kernel_restore(struct pt_regs *regs, unsigned long amr) { }
 
-static inline unsigned long __kuap_get_and_assert_locked(void)
-{
-	return 0;
-}
-
 /*
  * book3s/64/kup-radix.h defines these functions for the !KUAP case to flush
  * the L1D cache after user accesses. Only include the empty stubs for other
@@ -87,27 +80,32 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
 
 static __always_inline void kuap_assert_locked(void)
 {
+#if defined(CONFIG_PPC_KUAP_DEBUG) && defined(__kuap_get_and_assert_locked)
 	if (kuap_is_disabled())
 		return;
 
-	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG))
-		__kuap_get_and_assert_locked();
+	__kuap_get_and_assert_locked();
+#endif
 }
 
 static __always_inline void kuap_lock(void)
 {
+#ifdef __kuap_lock
 	if (kuap_is_disabled())
 		return;
 
 	__kuap_lock();
+#endif
 }
 
 static __always_inline void kuap_save_and_lock(struct pt_regs *regs)
 {
+#ifdef __kuap_save_and_lock
 	if (kuap_is_disabled())
 		return;
 
 	__kuap_save_and_lock(regs);
+#endif
 }
 
 static __always_inline void kuap_kernel_restore(struct pt_regs *regs, unsigned long amr)
@@ -120,10 +118,11 @@ static __always_inline void kuap_kernel_restore(struct pt_regs *regs, unsigned l
 
 static __always_inline unsigned long kuap_get_and_assert_locked(void)
 {
-	if (kuap_is_disabled())
-		return 0;
-
-	return __kuap_get_and_assert_locked();
+#ifdef __kuap_get_and_assert_locked
+	if (!kuap_is_disabled())
+		return __kuap_get_and_assert_locked();
+#endif
+	return 0;
 }
 
 #ifndef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index 8579210f2a6a..a372cd822887 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -20,15 +20,12 @@ static __always_inline bool kuap_is_disabled(void)
 	return static_branch_unlikely(&disable_kuap_key);
 }
 
-static inline void __kuap_lock(void)
-{
-}
-
 static inline void __kuap_save_and_lock(struct pt_regs *regs)
 {
 	regs->kuap = mfspr(SPRN_MD_AP);
 	mtspr(SPRN_MD_AP, MD_APG_KUAP);
 }
+#define __kuap_save_and_lock __kuap_save_and_lock
 
 static inline void kuap_user_restore(struct pt_regs *regs)
 {
@@ -39,13 +36,15 @@ static inline void __kuap_kernel_restore(struct pt_regs *regs, unsigned long kua
 	mtspr(SPRN_MD_AP, regs->kuap);
 }
 
+#ifdef CONFIG_PPC_KUAP_DEBUG
 static inline unsigned long __kuap_get_and_assert_locked(void)
 {
-	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG))
-		WARN_ON_ONCE(mfspr(SPRN_MD_AP) >> 16 != MD_APG_KUAP >> 16);
+	WARN_ON_ONCE(mfspr(SPRN_MD_AP) >> 16 != MD_APG_KUAP >> 16);
 
 	return 0;
 }
+#define __kuap_get_and_assert_locked __kuap_get_and_assert_locked
+#endif
 
 static inline void __allow_user_access(void __user *to, const void __user *from,
 				       unsigned long size, unsigned long dir)
diff --git a/arch/powerpc/include/asm/nohash/kup-booke.h b/arch/powerpc/include/asm/nohash/kup-booke.h
index 823c5a3a96d8..71182cbe20c3 100644
--- a/arch/powerpc/include/asm/nohash/kup-booke.h
+++ b/arch/powerpc/include/asm/nohash/kup-booke.h
@@ -30,6 +30,7 @@ static inline void __kuap_lock(void)
 	mtspr(SPRN_PID, 0);
 	isync();
 }
+#define __kuap_lock __kuap_lock
 
 static inline void __kuap_save_and_lock(struct pt_regs *regs)
 {
@@ -37,6 +38,7 @@ static inline void __kuap_save_and_lock(struct pt_regs *regs)
 	mtspr(SPRN_PID, 0);
 	isync();
 }
+#define __kuap_save_and_lock __kuap_save_and_lock
 
 static inline void kuap_user_restore(struct pt_regs *regs)
 {
@@ -56,13 +58,15 @@ static inline void __kuap_kernel_restore(struct pt_regs *regs, unsigned long kua
 	/* Context synchronisation is performed by rfi */
 }
 
+#ifdef CONFIG_PPC_KUAP_DEBUG
 static inline unsigned long __kuap_get_and_assert_locked(void)
 {
-	if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG))
-		WARN_ON_ONCE(mfspr(SPRN_PID));
+	WARN_ON_ONCE(mfspr(SPRN_PID));
 
 	return 0;
 }
+#define __kuap_get_and_assert_locked __kuap_get_and_assert_locked
+#endif
 
 static inline void __allow_user_access(void __user *to, const void __user *from,
 				       unsigned long size, unsigned long dir)
-- 
2.40.1

Re: [PATCH 2/4] powerpc/kuap: Avoid useless jump_label on empty function

[Nicholas Piggin]

On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
> Disassembly of interrupt_enter_prepare() shows a pointless nop before the mftb
>
>   c000abf0 <interrupt_enter_prepare>:
>   c000abf0:       81 23 00 84     lwz     r9,132(r3)
>   c000abf4:       71 29 40 00     andi.   r9,r9,16384
>   c000abf8:       41 82 00 28     beq-    c000ac20 <interrupt_enter_prepare+0x30>
>   c000abfc: ===>  60 00 00 00     nop	<====
>   c000ac00:       7d 0c 42 e6     mftb    r8
>   c000ac04:       80 e2 00 08     lwz     r7,8(r2)
>   c000ac08:       81 22 00 28     lwz     r9,40(r2)
>   c000ac0c:       91 02 00 24     stw     r8,36(r2)
>   c000ac10:       7d 29 38 50     subf    r9,r9,r7
>   c000ac14:       7d 29 42 14     add     r9,r9,r8
>   c000ac18:       91 22 00 08     stw     r9,8(r2)
>   c000ac1c:       4e 80 00 20     blr
>   c000ac20:       60 00 00 00     nop
>   c000ac24:       7d 5a c2 a6     mfmd_ap r10
>   c000ac28:       3d 20 de 00     lis     r9,-8704
>   c000ac2c:       91 43 00 b0     stw     r10,176(r3)
>   c000ac30:       7d 3a c3 a6     mtspr   794,r9
>   c000ac34:       4e 80 00 20     blr
>
> That comes from the call to kuap_loc(), allthough __kuap_lock() is an empty
> function on the 8xx.
>
> To avoid that, only perform kuap_is_disabled() check when there is something
> to do with __kuap_lock().
>
> Do the same with __kuap_save_and_lock() and __kuap_get_and_assert_locked().

Too bad static branch nops can't be eliminated.

> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 54cf46808157..1b0215ff3710 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -297,15 +297,7 @@ static inline unsigned long __kuap_get_and_assert_locked(void)
>  		WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
>  	return amr;
>  }
> -
> -/* Do nothing, book3s/64 does that in ASM */
> -static inline void __kuap_lock(void)
> -{
> -}
> -
> -static inline void __kuap_save_and_lock(struct pt_regs *regs)
> -{
> -}
> +#define __kuap_get_and_assert_locked __kuap_get_and_assert_locked

Maybe leave in /* __kuap_lock notrequired, book3s/64 does that in ASM */
? Seems okay though

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

[PATCH 3/4] powerpc/kuap: Refactor static branch for disabling kuap

[Christophe Leroy]

All but book3s/64 use a static branch key for disabling kuap.
book3s/64 uses a memory feature.

Refactor all targets except book3s/64.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/book3s/32/kup.h     |  7 -------
 arch/powerpc/include/asm/book3s/64/kup.h     |  1 +
 arch/powerpc/include/asm/kup.h               | 15 +++++++++++++++
 arch/powerpc/include/asm/nohash/32/kup-8xx.h |  7 -------
 arch/powerpc/include/asm/nohash/kup-booke.h  |  7 -------
 arch/powerpc/mm/book3s32/kuap.c              |  3 ---
 arch/powerpc/mm/init-common.c                |  3 +++
 arch/powerpc/mm/nohash/kup.c                 |  3 ---
 8 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
index 466a19cfb4df..8da9997a67ba 100644
--- a/arch/powerpc/include/asm/book3s/32/kup.h
+++ b/arch/powerpc/include/asm/book3s/32/kup.h
@@ -11,8 +11,6 @@
 
 #include <linux/jump_label.h>
 
-extern struct static_key_false disable_kuap_key;
-
 static __always_inline bool kuep_is_disabled(void)
 {
 	return !IS_ENABLED(CONFIG_PPC_KUEP);
@@ -25,11 +23,6 @@ static __always_inline bool kuep_is_disabled(void)
 #define KUAP_NONE	(~0UL)
 #define KUAP_ALL	(~1UL)
 
-static __always_inline bool kuap_is_disabled(void)
-{
-	return static_branch_unlikely(&disable_kuap_key);
-}
-
 static inline void kuap_lock_one(unsigned long addr)
 {
 	mtsr(mfsr(addr) | SR_KS, addr);
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
index 1b0215ff3710..f8b8e93c488c 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -233,6 +233,7 @@ static __always_inline bool kuap_is_disabled(void)
 {
 	return !mmu_has_feature(MMU_FTR_BOOK3S_KUAP);
 }
+#define kuap_is_disabled kuap_is_disabled
 
 static inline void kuap_user_restore(struct pt_regs *regs)
 {
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 0a4e07175612..74b7f4cee2ed 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -6,6 +6,12 @@
 #define KUAP_WRITE	2
 #define KUAP_READ_WRITE	(KUAP_READ | KUAP_WRITE)
 
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
+static __always_inline bool kuap_is_disabled(void);
+#endif
+
 #ifdef CONFIG_PPC_BOOK3S_64
 #include <asm/book3s/64/kup.h>
 #endif
@@ -41,6 +47,15 @@ void setup_kuep(bool disabled);
 
 #ifdef CONFIG_PPC_KUAP
 void setup_kuap(bool disabled);
+
+#ifndef kuap_is_disabled
+extern struct static_key_false disable_kuap_key;
+
+static __always_inline bool kuap_is_disabled(void)
+{
+	return static_branch_unlikely(&disable_kuap_key);
+}
+#endif
 #else
 static inline void setup_kuap(bool disabled) { }
 
diff --git a/arch/powerpc/include/asm/nohash/32/kup-8xx.h b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
index a372cd822887..1d53f38c5cd5 100644
--- a/arch/powerpc/include/asm/nohash/32/kup-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/kup-8xx.h
@@ -13,13 +13,6 @@
 
 #include <asm/reg.h>
 
-extern struct static_key_false disable_kuap_key;
-
-static __always_inline bool kuap_is_disabled(void)
-{
-	return static_branch_unlikely(&disable_kuap_key);
-}
-
 static inline void __kuap_save_and_lock(struct pt_regs *regs)
 {
 	regs->kuap = mfspr(SPRN_MD_AP);
diff --git a/arch/powerpc/include/asm/nohash/kup-booke.h b/arch/powerpc/include/asm/nohash/kup-booke.h
index 71182cbe20c3..07759ae9117b 100644
--- a/arch/powerpc/include/asm/nohash/kup-booke.h
+++ b/arch/powerpc/include/asm/nohash/kup-booke.h
@@ -18,13 +18,6 @@
 
 #include <asm/reg.h>
 
-extern struct static_key_false disable_kuap_key;
-
-static __always_inline bool kuap_is_disabled(void)
-{
-	return static_branch_unlikely(&disable_kuap_key);
-}
-
 static inline void __kuap_lock(void)
 {
 	mtspr(SPRN_PID, 0);
diff --git a/arch/powerpc/mm/book3s32/kuap.c b/arch/powerpc/mm/book3s32/kuap.c
index 28676cabb005..c5484729b595 100644
--- a/arch/powerpc/mm/book3s32/kuap.c
+++ b/arch/powerpc/mm/book3s32/kuap.c
@@ -3,9 +3,6 @@
 #include <asm/kup.h>
 #include <asm/smp.h>
 
-struct static_key_false disable_kuap_key;
-EXPORT_SYMBOL(disable_kuap_key);
-
 void kuap_lock_all_ool(void)
 {
 	kuap_lock_all();
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 119ef491f797..74e140b1efef 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -32,6 +32,9 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
 bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
 bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
 
+struct static_key_false disable_kuap_key;
+EXPORT_SYMBOL(disable_kuap_key);
+
 static int __init parse_nosmep(char *p)
 {
 	if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64))
diff --git a/arch/powerpc/mm/nohash/kup.c b/arch/powerpc/mm/nohash/kup.c
index 552becf90e97..4e22adfa2aa8 100644
--- a/arch/powerpc/mm/nohash/kup.c
+++ b/arch/powerpc/mm/nohash/kup.c
@@ -13,9 +13,6 @@
 #include <asm/smp.h>
 
 #ifdef CONFIG_PPC_KUAP
-struct static_key_false disable_kuap_key;
-EXPORT_SYMBOL(disable_kuap_key);
-
 void setup_kuap(bool disabled)
 {
 	if (disabled) {
-- 
2.40.1

Re: [PATCH 3/4] powerpc/kuap: Refactor static branch for disabling kuap

[Nicholas Piggin]

On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
> All but book3s/64 use a static branch key for disabling kuap.
> book3s/64 uses a memory feature.
>
> Refactor all targets except book3s/64.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/book3s/32/kup.h     |  7 -------
>  arch/powerpc/include/asm/book3s/64/kup.h     |  1 +
>  arch/powerpc/include/asm/kup.h               | 15 +++++++++++++++
>  arch/powerpc/include/asm/nohash/32/kup-8xx.h |  7 -------
>  arch/powerpc/include/asm/nohash/kup-booke.h  |  7 -------
>  arch/powerpc/mm/book3s32/kuap.c              |  3 ---
>  arch/powerpc/mm/init-common.c                |  3 +++
>  arch/powerpc/mm/nohash/kup.c                 |  3 ---
>  8 files changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
> index 466a19cfb4df..8da9997a67ba 100644
> --- a/arch/powerpc/include/asm/book3s/32/kup.h
> +++ b/arch/powerpc/include/asm/book3s/32/kup.h
> @@ -11,8 +11,6 @@
>  
>  #include <linux/jump_label.h>
>  
> -extern struct static_key_false disable_kuap_key;
> -
>  static __always_inline bool kuep_is_disabled(void)
>  {
>  	return !IS_ENABLED(CONFIG_PPC_KUEP);
> @@ -25,11 +23,6 @@ static __always_inline bool kuep_is_disabled(void)
>  #define KUAP_NONE	(~0UL)
>  #define KUAP_ALL	(~1UL)
>  
> -static __always_inline bool kuap_is_disabled(void)
> -{
> -	return static_branch_unlikely(&disable_kuap_key);
> -}
> -
>  static inline void kuap_lock_one(unsigned long addr)
>  {
>  	mtsr(mfsr(addr) | SR_KS, addr);
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 1b0215ff3710..f8b8e93c488c 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -233,6 +233,7 @@ static __always_inline bool kuap_is_disabled(void)
>  {
>  	return !mmu_has_feature(MMU_FTR_BOOK3S_KUAP);
>  }
> +#define kuap_is_disabled kuap_is_disabled

Is there any point to doing this pattern since the code is in places
that have ifdef PPC6 S etc?

> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 119ef491f797..74e140b1efef 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -32,6 +32,9 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
>  bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
>  bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>  
> +struct static_key_false disable_kuap_key;
> +EXPORT_SYMBOL(disable_kuap_key);
> +

That's going to define it on 64s?

Nice refactoring though.

Thanks,
Nick

Re: [PATCH 3/4] powerpc/kuap: Refactor static branch for disabling kuap

[Christophe Leroy]

Le 06/06/2023 à 11:16, Nicholas Piggin a écrit :
> On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
>> All but book3s/64 use a static branch key for disabling kuap.
>> book3s/64 uses a memory feature.
>>
>> Refactor all targets except book3s/64.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/book3s/32/kup.h     |  7 -------
>>   arch/powerpc/include/asm/book3s/64/kup.h     |  1 +
>>   arch/powerpc/include/asm/kup.h               | 15 +++++++++++++++
>>   arch/powerpc/include/asm/nohash/32/kup-8xx.h |  7 -------
>>   arch/powerpc/include/asm/nohash/kup-booke.h  |  7 -------
>>   arch/powerpc/mm/book3s32/kuap.c              |  3 ---
>>   arch/powerpc/mm/init-common.c                |  3 +++
>>   arch/powerpc/mm/nohash/kup.c                 |  3 ---
>>   8 files changed, 19 insertions(+), 27 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/32/kup.h b/arch/powerpc/include/asm/book3s/32/kup.h
>> index 466a19cfb4df..8da9997a67ba 100644
>> --- a/arch/powerpc/include/asm/book3s/32/kup.h
>> +++ b/arch/powerpc/include/asm/book3s/32/kup.h
>> @@ -11,8 +11,6 @@
>>   
>>   #include <linux/jump_label.h>
>>   
>> -extern struct static_key_false disable_kuap_key;
>> -
>>   static __always_inline bool kuep_is_disabled(void)
>>   {
>>   	return !IS_ENABLED(CONFIG_PPC_KUEP);
>> @@ -25,11 +23,6 @@ static __always_inline bool kuep_is_disabled(void)
>>   #define KUAP_NONE	(~0UL)
>>   #define KUAP_ALL	(~1UL)
>>   
>> -static __always_inline bool kuap_is_disabled(void)
>> -{
>> -	return static_branch_unlikely(&disable_kuap_key);
>> -}
>> -
>>   static inline void kuap_lock_one(unsigned long addr)
>>   {
>>   	mtsr(mfsr(addr) | SR_KS, addr);
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
>> index 1b0215ff3710..f8b8e93c488c 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
>> @@ -233,6 +233,7 @@ static __always_inline bool kuap_is_disabled(void)
>>   {
>>   	return !mmu_has_feature(MMU_FTR_BOOK3S_KUAP);
>>   }
>> +#define kuap_is_disabled kuap_is_disabled
> 
> Is there any point to doing this pattern since the code is in places
> that have ifdef PPC6 S etc?

I'm not sure what you have in mind.

There is a default kuap_is_disabled() in arch/powerpc/include/asm/kup.h
For 64s we want the version from 
arch/powerpc/include/asm/book3s/64/kup.h instead of the default one.

And kuap_is_disabled() is used in some common code in 
arch/powerpc/include/asm/kup.h, not only in the code encloded by #ifndef 64s

> 
>> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
>> index 119ef491f797..74e140b1efef 100644
>> --- a/arch/powerpc/mm/init-common.c
>> +++ b/arch/powerpc/mm/init-common.c
>> @@ -32,6 +32,9 @@ EXPORT_SYMBOL_GPL(kernstart_virt_addr);
>>   bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
>>   bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>>   
>> +struct static_key_false disable_kuap_key;
>> +EXPORT_SYMBOL(disable_kuap_key);
>> +
> 
> That's going to define it on 64s?

Is that a problem at all ?

By the way I was thinking about also using that key on 64s, will look at 
it as a second step.

Thanks for the review
Christophe

> 
> Nice refactoring though.
> 
> Thanks,
> Nick

[PATCH 4/4] powerpc/kuap: Make disabling KUAP at boottime optional

[Christophe Leroy]

It is possible to disable KUAP at boottime with 'nosmap' parameter.

That is implemented with jump_label hence adds a 'nop' in front
of each open/close of userspace access.

From a security point of view it makes sence to disallow disabling
KUAP. And on processors like the 8xx where 'nop' is not seamless,
it saves a few cycles.

So add a CONFIG item to make it optionnal.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/include/asm/kup.h         |  2 +-
 arch/powerpc/mm/init-common.c          |  3 +++
 arch/powerpc/platforms/Kconfig.cputype | 10 ++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 74b7f4cee2ed..f3280169aeec 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -53,7 +53,7 @@ extern struct static_key_false disable_kuap_key;
 
 static __always_inline bool kuap_is_disabled(void)
 {
-	return static_branch_unlikely(&disable_kuap_key);
+	return IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME) && static_branch_unlikely(&disable_kuap_key);
 }
 #endif
 #else
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 74e140b1efef..994ee58f0092 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -48,6 +48,9 @@ early_param("nosmep", parse_nosmep);
 
 static int __init parse_nosmap(char *p)
 {
+	if (!IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME))
+		return 0;
+
 	disable_kuap = true;
 	pr_warn("Disabling Kernel Userspace Access Protection\n");
 	return 0;
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 45fd975ef521..f75c2d5cd182 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -502,6 +502,16 @@ config PPC_KUAP
 
 	  If you're unsure, say Y.
 
+config PPC_KUAP_BOOTTIME
+	bool "Allow disabling Kernel Userspace Access Protection at boottime"
+	depends on PPC_KUAP
+	default y
+	help
+	  Allow the user to disable Kernel Userspace Access Protection (KUAP)
+	  at boot time using 'nosmap' kernel parameter.
+
+	  If you're unsure, say Y.
+
 config PPC_KUAP_DEBUG
 	bool "Extra debugging for Kernel Userspace Access Protection"
 	depends on PPC_KUAP
-- 
2.40.1

Re: [PATCH 4/4] powerpc/kuap: Make disabling KUAP at boottime optional

[Nicholas Piggin]

On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
> It is possible to disable KUAP at boottime with 'nosmap' parameter.
>
> That is implemented with jump_label hence adds a 'nop' in front
> of each open/close of userspace access.
>
> From a security point of view it makes sence to disallow disabling
> KUAP. And on processors like the 8xx where 'nop' is not seamless,
> it saves a few cycles.
>
> So add a CONFIG item to make it optionnal.

I love counting cycles, but a CONFIG option for one nop... I think
you have me beat.

Is that sustainable? What if instead of the static branch you patched in
nops to the lock/unlock asm? AFAIKS there does not look like much (any?)
C code in the kuap enabled branches.

Thanks,
Nick

>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/kup.h         |  2 +-
>  arch/powerpc/mm/init-common.c          |  3 +++
>  arch/powerpc/platforms/Kconfig.cputype | 10 ++++++++++
>  3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index 74b7f4cee2ed..f3280169aeec 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -53,7 +53,7 @@ extern struct static_key_false disable_kuap_key;
>  
>  static __always_inline bool kuap_is_disabled(void)
>  {
> -	return static_branch_unlikely(&disable_kuap_key);
> +	return IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME) && static_branch_unlikely(&disable_kuap_key);
>  }
>  #endif
>  #else
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 74e140b1efef..994ee58f0092 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -48,6 +48,9 @@ early_param("nosmep", parse_nosmep);
>  
>  static int __init parse_nosmap(char *p)
>  {
> +	if (!IS_ENABLED(CONFIG_PPC_KUAP_BOOTTIME))
> +		return 0;
> +
>  	disable_kuap = true;
>  	pr_warn("Disabling Kernel Userspace Access Protection\n");
>  	return 0;
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 45fd975ef521..f75c2d5cd182 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -502,6 +502,16 @@ config PPC_KUAP
>  
>  	  If you're unsure, say Y.
>  
> +config PPC_KUAP_BOOTTIME
> +	bool "Allow disabling Kernel Userspace Access Protection at boottime"
> +	depends on PPC_KUAP
> +	default y
> +	help
> +	  Allow the user to disable Kernel Userspace Access Protection (KUAP)
> +	  at boot time using 'nosmap' kernel parameter.
> +
> +	  If you're unsure, say Y.
> +
>  config PPC_KUAP_DEBUG
>  	bool "Extra debugging for Kernel Userspace Access Protection"
>  	depends on PPC_KUAP
> -- 
> 2.40.1

Re: [PATCH 4/4] powerpc/kuap: Make disabling KUAP at boottime optional

[Christophe Leroy]

Le 06/06/2023 à 11:27, Nicholas Piggin a écrit :
> On Mon Jun 5, 2023 at 9:04 PM AEST, Christophe Leroy wrote:
>> It is possible to disable KUAP at boottime with 'nosmap' parameter.
>>
>> That is implemented with jump_label hence adds a 'nop' in front
>> of each open/close of userspace access.
>>
>>  From a security point of view it makes sence to disallow disabling
>> KUAP. And on processors like the 8xx where 'nop' is not seamless,
>> it saves a few cycles.
>>
>> So add a CONFIG item to make it optionnal.
> 
> I love counting cycles, but a CONFIG option for one nop... I think
> you have me beat.

Unlike most other powerpc, a nop on 8xx is one cycle. So if we can avoid 
it on hot pathes ....

> 
> Is that sustainable? What if instead of the static branch you patched in
> nops to the lock/unlock asm? AFAIKS there does not look like much (any?)
> C code in the kuap enabled branches.

Yes that's probably the solution at the end.

At the moment I'm struggling with UACCESS objtool validation and I think 
I will temporarily disable boottime 'nosmap' parameter for PPC32.

The problem is that with the static branch, objtool visits both branches 
but it does it independently for each branch, it is not able to match 
two static branches using the same key. So the result is that it detects 
UACCESS enable without UACCESS disable and vice-versa.

So the solution at the end will be to patch-out the mtspr, for the time 
being I'm just going to disable it completely for PPC32.

Christophe