* [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle
@ 2019-03-08 1:16 Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR " Michael Ellerman
` (8 more replies)
0 siblings, 9 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Russell Currey <ruscur@russell.cc>
Without restoring the IAMR after idle, execution prevention on POWER9
with Radix MMU is overwritten and the kernel can freely execute
userspace without faulting.
This is necessary when returning from any stop state that modifies
user state, as well as hypervisor state.
To test how this fails without this patch, load the lkdtm driver and
do the following:
$ echo EXEC_USERSPACE > /sys/kernel/debug/provoke-crash/DIRECT
which won't fault, then boot the kernel with powersave=off, where it
will fault. Applying this patch will fix this.
Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel execution of user space")
Cc: stable@vger.kernel.org # v4.10+
Signed-off-by: Russell Currey <ruscur@russell.cc>
Reviewed-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
v4: mpe: Use a #define for the stack slot.
arch/powerpc/kernel/idle_book3s.S | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 7f5ac2e8581b..36178000a2f2 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -170,6 +170,9 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
bne- core_idle_lock_held
blr
+/* Reuse an unused pt_regs slot for IAMR */
+#define PNV_POWERSAVE_IAMR _DAR
+
/*
* Pass requested state in r3:
* r3 - PNV_THREAD_NAP/SLEEP/WINKLE in POWER8
@@ -200,6 +203,12 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
/* Continue saving state */
SAVE_GPR(2, r1)
SAVE_NVGPRS(r1)
+
+BEGIN_FTR_SECTION
+ mfspr r5, SPRN_IAMR
+ std r5, PNV_POWERSAVE_IAMR(r1)
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
+
mfcr r5
std r5,_CCR(r1)
std r1,PACAR1(r13)
@@ -924,6 +933,17 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
REST_NVGPRS(r1)
REST_GPR(2, r1)
+
+BEGIN_FTR_SECTION
+ /* IAMR was saved in pnv_powersave_common() */
+ ld r5, PNV_POWERSAVE_IAMR(r1)
+ mtspr SPRN_IAMR, r5
+ /*
+ * We don't need an isync here because the upcoming mtmsrd is
+ * execution synchronizing.
+ */
+END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
+
ld r4,PACAKMSR(r13)
ld r5,_LINK(r1)
ld r6,_CCR(r1)
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-13 8:16 ` Akshay Adiga
2019-03-08 1:16 ` [PATCH v5 03/10] powerpc: Add framework for Kernel Userspace Protection Michael Ellerman
` (7 subsequent siblings)
8 siblings, 1 reply; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
In order to implement KUAP (Kernel Userspace Access Protection) on
Power9 we will be using the AMR, and therefore indirectly the
UAMOR/AMOR.
So save/restore these regs in the idle code.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
v4: New.
arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
index 36178000a2f2..4a860d3b9229 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S
@@ -170,8 +170,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
bne- core_idle_lock_held
blr
-/* Reuse an unused pt_regs slot for IAMR */
+/* Reuse some unused pt_regs slots for AMR/IAMR/UAMOR/UAMOR */
+#define PNV_POWERSAVE_AMR _TRAP
#define PNV_POWERSAVE_IAMR _DAR
+#define PNV_POWERSAVE_UAMOR _DSISR
+#define PNV_POWERSAVE_AMOR RESULT
/*
* Pass requested state in r3:
@@ -205,8 +208,16 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_300)
SAVE_NVGPRS(r1)
BEGIN_FTR_SECTION
+ mfspr r4, SPRN_AMR
mfspr r5, SPRN_IAMR
+ mfspr r6, SPRN_UAMOR
+ std r4, PNV_POWERSAVE_AMR(r1)
std r5, PNV_POWERSAVE_IAMR(r1)
+ std r6, PNV_POWERSAVE_UAMOR(r1)
+BEGIN_FTR_SECTION_NESTED(42)
+ mfspr r7, SPRN_AMOR
+ std r7, PNV_POWERSAVE_AMOR(r1)
+END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
mfcr r5
@@ -935,12 +946,20 @@ END_FTR_SECTION_IFSET(CPU_FTR_HVMODE)
REST_GPR(2, r1)
BEGIN_FTR_SECTION
- /* IAMR was saved in pnv_powersave_common() */
+ /* These regs were saved in pnv_powersave_common() */
+ ld r4, PNV_POWERSAVE_AMR(r1)
ld r5, PNV_POWERSAVE_IAMR(r1)
+ ld r6, PNV_POWERSAVE_UAMOR(r1)
+ mtspr SPRN_AMR, r4
mtspr SPRN_IAMR, r5
+ mtspr SPRN_UAMOR, r6
+BEGIN_FTR_SECTION_NESTED(42)
+ ld r7, PNV_POWERSAVE_AMOR(r1)
+ mtspr SPRN_AMOR, r7
+END_FTR_SECTION_NESTED_IFSET(CPU_FTR_HVMODE, 42)
/*
- * We don't need an isync here because the upcoming mtmsrd is
- * execution synchronizing.
+ * We don't need an isync here after restoring IAMR because the upcoming
+ * mtmsrd is execution synchronizing.
*/
END_FTR_SECTION_IFSET(CPU_FTR_ARCH_207S)
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 03/10] powerpc: Add framework for Kernel Userspace Protection
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR " Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 04/10] powerpc: Add skeleton for Kernel Userspace Execution Prevention Michael Ellerman
` (6 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Christophe Leroy <christophe.leroy@c-s.fr>
This patch adds a skeleton for Kernel Userspace Protection
functionnalities like Kernel Userspace Access Protection and Kernel
Userspace Execution Prevention
The subsequent implementation of KUAP for radix makes use of a MMU
feature in order to patch out assembly when KUAP is disabled or
unsupported. This won't work unless there's an entry point for KUP
support before the feature magic happens, so for PPC64 setup_kup() is
called early in setup.
On PPC32, feature_fixup() is done too early to allow the same.
Suggested-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
v4: Unchanged.
arch/powerpc/include/asm/kup.h | 11 +++++++++++
arch/powerpc/kernel/setup_64.c | 7 +++++++
arch/powerpc/mm/init-common.c | 5 +++++
arch/powerpc/mm/init_32.c | 3 +++
4 files changed, 26 insertions(+)
create mode 100644 arch/powerpc/include/asm/kup.h
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
new file mode 100644
index 000000000000..7a88b8b9b54d
--- /dev/null
+++ b/arch/powerpc/include/asm/kup.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_KUP_H_
+#define _ASM_POWERPC_KUP_H_
+
+#ifndef __ASSEMBLY__
+
+void setup_kup(void);
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* _ASM_POWERPC_KUP_H_ */
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index daa361fc6a24..47ffa0885081 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -68,6 +68,7 @@
#include <asm/cputhreads.h>
#include <asm/hw_irq.h>
#include <asm/feature-fixups.h>
+#include <asm/kup.h>
#include "setup.h"
@@ -331,6 +332,12 @@ void __init early_setup(unsigned long dt_ptr)
*/
configure_exceptions();
+ /*
+ * Configure Kernel Userspace Protection. This needs to happen before
+ * feature fixups for platforms that implement this using features.
+ */
+ setup_kup();
+
/* Apply all the dynamic patching */
apply_feature_fixups();
setup_feature_keys();
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 1e6910eb70ed..36d28e872289 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -24,6 +24,11 @@
#include <linux/string.h>
#include <asm/pgalloc.h>
#include <asm/pgtable.h>
+#include <asm/kup.h>
+
+void __init setup_kup(void)
+{
+}
#define CTOR(shift) static void ctor_##shift(void *addr) \
{ \
diff --git a/arch/powerpc/mm/init_32.c b/arch/powerpc/mm/init_32.c
index 41a3513cadc9..80cc97cd8878 100644
--- a/arch/powerpc/mm/init_32.c
+++ b/arch/powerpc/mm/init_32.c
@@ -45,6 +45,7 @@
#include <asm/tlb.h>
#include <asm/sections.h>
#include <asm/hugetlb.h>
+#include <asm/kup.h>
#include "mmu_decl.h"
@@ -178,6 +179,8 @@ void __init MMU_init(void)
btext_unmap();
#endif
+ setup_kup();
+
/* Shortly after that, the entire linear mapping will be available */
memblock_set_current_limit(lowmem_end_addr);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 04/10] powerpc: Add skeleton for Kernel Userspace Execution Prevention
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR " Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 03/10] powerpc: Add framework for Kernel Userspace Protection Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection Michael Ellerman
` (5 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Christophe Leroy <christophe.leroy@c-s.fr>
This patch adds a skeleton for Kernel Userspace Execution Prevention.
Then subarches implementing it have to define CONFIG_PPC_HAVE_KUEP
and provide setup_kuep() function.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
[mpe: Don't split strings, use pr_crit_ratelimited()]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
v4: mpe: Don't split strings, use pr_crit_ratelimited()
Documentation/admin-guide/kernel-parameters.txt | 2 +-
arch/powerpc/include/asm/kup.h | 6 ++++++
arch/powerpc/mm/fault.c | 9 ++++-----
arch/powerpc/mm/init-common.c | 11 +++++++++++
arch/powerpc/platforms/Kconfig.cputype | 12 ++++++++++++
5 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index b799bcf67d7b..f81d79de4de0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2813,7 +2813,7 @@
Disable SMAP (Supervisor Mode Access Prevention)
even if it is supported by processor.
- nosmep [X86]
+ nosmep [X86,PPC]
Disable SMEP (Supervisor Mode Execution Prevention)
even if it is supported by processor.
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 7a88b8b9b54d..a2a959cb4e36 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -6,6 +6,12 @@
void setup_kup(void);
+#ifdef CONFIG_PPC_KUEP
+void setup_kuep(bool disabled);
+#else
+static inline void setup_kuep(bool disabled) { }
+#endif /* CONFIG_PPC_KUEP */
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_POWERPC_KUP_H_ */
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 887f11bcf330..3384354abc1d 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -229,11 +229,10 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
DSISR_PROTFAULT))) {
- printk_ratelimited(KERN_CRIT "kernel tried to execute"
- " exec-protected page (%lx) -"
- "exploit attempt? (uid: %d)\n",
- address, from_kuid(&init_user_ns,
- current_uid()));
+ pr_crit_ratelimited("kernel tried to execute %s page (%lx) - exploit attempt? (uid: %d)\n",
+ address >= TASK_SIZE ? "exec-protected" : "user",
+ address,
+ from_kuid(&init_user_ns, current_uid()));
}
return is_exec || (address >= TASK_SIZE);
}
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 36d28e872289..83f95a5565d6 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -26,8 +26,19 @@
#include <asm/pgtable.h>
#include <asm/kup.h>
+static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
+
+static int __init parse_nosmep(char *p)
+{
+ disable_kuep = true;
+ pr_warn("Disabling Kernel Userspace Execution Prevention\n");
+ return 0;
+}
+early_param("nosmep", parse_nosmep);
+
void __init setup_kup(void)
{
+ setup_kuep(disable_kuep);
}
#define CTOR(shift) static void ctor_##shift(void *addr) \
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 842b2c7e156a..7d30bbbaa3c1 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -345,6 +345,18 @@ config PPC_RADIX_MMU_DEFAULT
If you're unsure, say Y.
+config PPC_HAVE_KUEP
+ bool
+
+config PPC_KUEP
+ bool "Kernel Userspace Execution Prevention"
+ depends on PPC_HAVE_KUEP
+ default y
+ help
+ Enable support for Kernel Userspace Execution Prevention (KUEP)
+
+ If you're unsure, say Y.
+
config ARCH_ENABLE_HUGEPAGE_MIGRATION
def_bool y
depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (2 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 04/10] powerpc: Add skeleton for Kernel Userspace Execution Prevention Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 8:26 ` Christophe Leroy
2019-03-11 9:12 ` Christophe Leroy
2019-03-08 1:16 ` [PATCH v5 06/10] powerpc/64: Setup KUP on secondary CPUs Michael Ellerman
` (4 subsequent siblings)
8 siblings, 2 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Christophe Leroy <christophe.leroy@c-s.fr>
This patch implements a framework for Kernel Userspace Access
Protection.
Then subarches will have the possibility to provide their own
implementation by providing setup_kuap() and
allow/prevent_user_access().
Some platforms will need to know the area accessed and whether it is
accessed from read, write or both. Therefore source, destination and
size and handed over to the two functions.
mpe: Rename to allow/prevent rather than unlock/lock, and add
read/write wrappers. Drop the 32-bit code for now until we have an
implementation for it. Add kuap to pt_regs for 64-bit as well as
32-bit. Don't split strings, use pr_crit_ratelimited().
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Futex ops need read/write so use allow_user_acccess() there.
Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
Allow subarch to override allow_read/write_from/to_user().
v4: mpe: Rename to allow/prevent rather than unlock/lock, and add
read/write wrappers. Drop the 32-bit code for now until we have an
implementation for it. Add kuap to pt_regs for 64-bit as well as
32-bit. Don't split strings, use pr_crit_ratelimited().
.../admin-guide/kernel-parameters.txt | 2 +-
arch/powerpc/include/asm/futex.h | 4 ++
arch/powerpc/include/asm/kup.h | 24 ++++++++++++
arch/powerpc/include/asm/ptrace.h | 11 +++++-
arch/powerpc/include/asm/uaccess.h | 38 +++++++++++++++----
arch/powerpc/kernel/asm-offsets.c | 4 ++
arch/powerpc/lib/checksum_wrappers.c | 4 ++
arch/powerpc/mm/fault.c | 19 ++++++++--
arch/powerpc/mm/init-common.c | 10 +++++
arch/powerpc/platforms/Kconfig.cputype | 12 ++++++
10 files changed, 113 insertions(+), 15 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f81d79de4de0..16883f2a05fd 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2809,7 +2809,7 @@
noexec=on: enable non-executable mappings (default)
noexec=off: disable non-executable mappings
- nosmap [X86]
+ nosmap [X86,PPC]
Disable SMAP (Supervisor Mode Access Prevention)
even if it is supported by processor.
diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
index 88b38b37c21b..3a6aa57b9d90 100644
--- a/arch/powerpc/include/asm/futex.h
+++ b/arch/powerpc/include/asm/futex.h
@@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
{
int oldval = 0, ret;
+ allow_write_to_user(uaddr, sizeof(*uaddr));
pagefault_disable();
switch (op) {
@@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
if (!ret)
*oval = oldval;
+ prevent_write_to_user(uaddr, sizeof(*uaddr));
return ret;
}
@@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
if (!access_ok(uaddr, sizeof(u32)))
return -EFAULT;
+ allow_write_to_user(uaddr, sizeof(*uaddr));
__asm__ __volatile__ (
PPC_ATOMIC_ENTRY_BARRIER
"1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\
@@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
: "cc", "memory");
*uval = prev;
+ prevent_write_to_user(uaddr, sizeof(*uaddr));
return ret;
}
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index a2a959cb4e36..4410625f4364 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -4,6 +4,8 @@
#ifndef __ASSEMBLY__
+#include <asm/pgtable.h>
+
void setup_kup(void);
#ifdef CONFIG_PPC_KUEP
@@ -12,6 +14,28 @@ void setup_kuep(bool disabled);
static inline void setup_kuep(bool disabled) { }
#endif /* CONFIG_PPC_KUEP */
+#ifdef CONFIG_PPC_KUAP
+void setup_kuap(bool disabled);
+#else
+static inline void setup_kuap(bool disabled) { }
+static inline void allow_user_access(void __user *to, const void __user *from,
+ unsigned long size) { }
+static inline void prevent_user_access(void __user *to, const void __user *from,
+ unsigned long size) { }
+static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
+static inline void allow_write_to_user(void __user *to, unsigned long size) {}
+#endif /* CONFIG_PPC_KUAP */
+
+static inline void prevent_read_from_user(const void __user *from, unsigned long size)
+{
+ prevent_user_access(NULL, from, size);
+}
+
+static inline void prevent_write_to_user(void __user *to, unsigned long size)
+{
+ prevent_user_access(to, NULL, size);
+}
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_POWERPC_KUP_H_ */
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 64271e562fed..6f047730e642 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -52,10 +52,17 @@ struct pt_regs
};
};
+ union {
+ struct {
#ifdef CONFIG_PPC64
- unsigned long ppr;
- unsigned long __pad; /* Maintain 16 byte interrupt stack alignment */
+ unsigned long ppr;
+#endif
+#ifdef CONFIG_PPC_KUAP
+ unsigned long kuap;
#endif
+ };
+ unsigned long __pad[2]; /* Maintain 16 byte interrupt stack alignment */
+ };
};
#endif
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index e3a731793ea2..fb7651a5488b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -6,6 +6,7 @@
#include <asm/processor.h>
#include <asm/page.h>
#include <asm/extable.h>
+#include <asm/kup.h>
/*
* The fs value determines whether argument validity checking should be
@@ -141,6 +142,7 @@ extern long __put_user_bad(void);
#define __put_user_size(x, ptr, size, retval) \
do { \
retval = 0; \
+ allow_write_to_user(ptr, size); \
switch (size) { \
case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
@@ -148,6 +150,7 @@ do { \
case 8: __put_user_asm2(x, ptr, retval); break; \
default: __put_user_bad(); \
} \
+ prevent_write_to_user(ptr, size); \
} while (0)
#define __put_user_nocheck(x, ptr, size) \
@@ -240,6 +243,7 @@ do { \
__chk_user_ptr(ptr); \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
+ allow_read_from_user(ptr, size); \
switch (size) { \
case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
@@ -247,6 +251,7 @@ do { \
case 8: __get_user_asm2(x, ptr, retval); break; \
default: (x) = __get_user_bad(); \
} \
+ prevent_read_from_user(ptr, size); \
} while (0)
/*
@@ -306,15 +311,21 @@ extern unsigned long __copy_tofrom_user(void __user *to,
static inline unsigned long
raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
{
- return __copy_tofrom_user(to, from, n);
+ unsigned long ret;
+
+ allow_user_access(to, from, n);
+ ret = __copy_tofrom_user(to, from, n);
+ prevent_user_access(to, from, n);
+ return ret;
}
#endif /* __powerpc64__ */
static inline unsigned long raw_copy_from_user(void *to,
const void __user *from, unsigned long n)
{
+ unsigned long ret;
if (__builtin_constant_p(n) && (n <= 8)) {
- unsigned long ret = 1;
+ ret = 1;
switch (n) {
case 1:
@@ -339,14 +350,18 @@ static inline unsigned long raw_copy_from_user(void *to,
}
barrier_nospec();
- return __copy_tofrom_user((__force void __user *)to, from, n);
+ allow_read_from_user(from, n);
+ ret = __copy_tofrom_user((__force void __user *)to, from, n);
+ prevent_read_from_user(from, n);
+ return ret;
}
static inline unsigned long raw_copy_to_user(void __user *to,
const void *from, unsigned long n)
{
+ unsigned long ret;
if (__builtin_constant_p(n) && (n <= 8)) {
- unsigned long ret = 1;
+ ret = 1;
switch (n) {
case 1:
@@ -366,17 +381,24 @@ static inline unsigned long raw_copy_to_user(void __user *to,
return 0;
}
- return __copy_tofrom_user(to, (__force const void __user *)from, n);
+ allow_write_to_user(to, n);
+ ret = __copy_tofrom_user(to, (__force const void __user *)from, n);
+ prevent_write_to_user(to, n);
+ return ret;
}
extern unsigned long __clear_user(void __user *addr, unsigned long size);
static inline unsigned long clear_user(void __user *addr, unsigned long size)
{
+ unsigned long ret = size;
might_fault();
- if (likely(access_ok(addr, size)))
- return __clear_user(addr, size);
- return size;
+ if (likely(access_ok(addr, size))) {
+ allow_write_to_user(addr, size);
+ ret = __clear_user(addr, size);
+ prevent_write_to_user(addr, size);
+ }
+ return ret;
}
extern long strncpy_from_user(char *dst, const char __user *src, long count);
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 86a61e5f8285..66202e02fee2 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -332,6 +332,10 @@ int main(void)
STACK_PT_REGS_OFFSET(_PPR, ppr);
#endif /* CONFIG_PPC64 */
+#ifdef CONFIG_PPC_KUAP
+ STACK_PT_REGS_OFFSET(STACK_REGS_KUAP, kuap);
+#endif
+
#if defined(CONFIG_PPC32)
#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
DEFINE(EXC_LVL_SIZE, STACK_EXC_LVL_FRAME_SIZE);
diff --git a/arch/powerpc/lib/checksum_wrappers.c b/arch/powerpc/lib/checksum_wrappers.c
index 890d4ddd91d6..bb9307ce2440 100644
--- a/arch/powerpc/lib/checksum_wrappers.c
+++ b/arch/powerpc/lib/checksum_wrappers.c
@@ -29,6 +29,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
unsigned int csum;
might_sleep();
+ allow_read_from_user(src, len);
*err_ptr = 0;
@@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
}
out:
+ prevent_read_from_user(src, len);
return (__force __wsum)csum;
}
EXPORT_SYMBOL(csum_and_copy_from_user);
@@ -70,6 +72,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
unsigned int csum;
might_sleep();
+ allow_write_to_user(dst, len);
*err_ptr = 0;
@@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
}
out:
+ prevent_write_to_user(dst, len);
return (__force __wsum)csum;
}
EXPORT_SYMBOL(csum_and_copy_to_user);
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 3384354abc1d..463d1e9d026e 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -223,9 +223,11 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
}
/* Is this a bad kernel fault ? */
-static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
+static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
unsigned long address)
{
+ int is_exec = TRAP(regs) == 0x400;
+
/* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
DSISR_PROTFAULT))) {
@@ -234,7 +236,15 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
address,
from_kuid(&init_user_ns, current_uid()));
}
- return is_exec || (address >= TASK_SIZE);
+
+ if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
+ !search_exception_tables(regs->nip)) {
+ pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
+ address,
+ from_kuid(&init_user_ns, current_uid()));
+ }
+
+ return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip);
}
static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
@@ -454,9 +464,10 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
/*
* The kernel should never take an execute fault nor should it
- * take a page fault to a kernel address.
+ * take a page fault to a kernel address or a page fault to a user
+ * address outside of dedicated places
*/
- if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address)))
+ if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address)))
return SIGSEGV;
/*
diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
index 83f95a5565d6..ecaedfff9992 100644
--- a/arch/powerpc/mm/init-common.c
+++ b/arch/powerpc/mm/init-common.c
@@ -27,6 +27,7 @@
#include <asm/kup.h>
static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
+static bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
static int __init parse_nosmep(char *p)
{
@@ -36,9 +37,18 @@ static int __init parse_nosmep(char *p)
}
early_param("nosmep", parse_nosmep);
+static int __init parse_nosmap(char *p)
+{
+ disable_kuap = true;
+ pr_warn("Disabling Kernel Userspace Access Protection\n");
+ return 0;
+}
+early_param("nosmap", parse_nosmap);
+
void __init setup_kup(void)
{
setup_kuep(disable_kuep);
+ setup_kuap(disable_kuap);
}
#define CTOR(shift) static void ctor_##shift(void *addr) \
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 7d30bbbaa3c1..457fc3a5ed93 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -357,6 +357,18 @@ config PPC_KUEP
If you're unsure, say Y.
+config PPC_HAVE_KUAP
+ bool
+
+config PPC_KUAP
+ bool "Kernel Userspace Access Protection"
+ depends on PPC_HAVE_KUAP
+ default y
+ help
+ Enable support for Kernel Userspace Access Protection (KUAP)
+
+ If you're unsure, say Y.
+
config ARCH_ENABLE_HUGEPAGE_MIGRATION
def_bool y
depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 06/10] powerpc/64: Setup KUP on secondary CPUs
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (3 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 07/10] powerpc/mm/radix: Use KUEP API for Radix MMU Michael Ellerman
` (3 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Russell Currey <ruscur@russell.cc>
Some platforms (i.e. Radix MMU) need per-CPU initialisation for KUP.
Any platforms that only want to do KUP initialisation once
globally can just check to see if they're running on the boot CPU, or
check if whatever setup they need has already been performed.
Note that this is only for 64-bit.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
arch/powerpc/kernel/setup_64.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 47ffa0885081..72358a5022fe 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -390,6 +390,9 @@ void early_setup_secondary(void)
/* Initialize the hash table or TLB handling */
early_init_mmu_secondary();
+ /* Perform any KUP setup that is per-cpu */
+ setup_kup();
+
/*
* At this point, we can let interrupts switch to virtual mode
* (the MMU has been setup), so adjust the MSR in the PACA to
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 07/10] powerpc/mm/radix: Use KUEP API for Radix MMU
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (4 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 06/10] powerpc/64: Setup KUP on secondary CPUs Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm() Michael Ellerman
` (2 subsequent siblings)
8 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Russell Currey <ruscur@russell.cc>
Execution protection already exists on radix, this just refactors
the radix init to provide the KUEP setup function instead.
Thus, the only functional change is that it can now be disabled.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
arch/powerpc/mm/pgtable-radix.c | 12 +++++++++---
arch/powerpc/platforms/Kconfig.cputype | 1 +
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index 931156069a81..224bcd4be5ae 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -535,8 +535,15 @@ static void radix_init_amor(void)
mtspr(SPRN_AMOR, (3ul << 62));
}
-static void radix_init_iamr(void)
+#ifdef CONFIG_PPC_KUEP
+void __init setup_kuep(bool disabled)
{
+ if (disabled || !early_radix_enabled())
+ return;
+
+ if (smp_processor_id() == boot_cpuid)
+ pr_info("Activating Kernel Userspace Execution Prevention\n");
+
/*
* Radix always uses key0 of the IAMR to determine if an access is
* allowed. We set bit 0 (IBM bit 1) of key0, to prevent instruction
@@ -544,6 +551,7 @@ static void radix_init_iamr(void)
*/
mtspr(SPRN_IAMR, (1ul << 62));
}
+#endif
void __init radix__early_init_mmu(void)
{
@@ -605,7 +613,6 @@ void __init radix__early_init_mmu(void)
memblock_set_current_limit(MEMBLOCK_ALLOC_ANYWHERE);
- radix_init_iamr();
radix_init_pgtable();
/* Switch to the guard PID before turning on MMU */
radix__switch_mmu_context(NULL, &init_mm);
@@ -627,7 +634,6 @@ void radix__early_init_mmu_secondary(void)
__pa(partition_tb) | (PATB_SIZE_SHIFT - 12));
radix_init_amor();
}
- radix_init_iamr();
radix__switch_mmu_context(NULL, &init_mm);
if (cpu_has_feature(CPU_FTR_HVMODE))
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index 457fc3a5ed93..60371784c9f1 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -326,6 +326,7 @@ config PPC_RADIX_MMU
bool "Radix MMU Support"
depends on PPC_BOOK3S_64
select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
+ select PPC_HAVE_KUEP
default y
help
Enable support for the Power ISA 3.0 Radix style MMU. Currently this
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm()
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (5 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 07/10] powerpc/mm/radix: Use KUEP API for Radix MMU Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 8:32 ` Christophe Leroy
2019-03-08 1:16 ` [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults Michael Ellerman
8 siblings, 1 reply; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Russell Currey <ruscur@russell.cc>
__patch_instruction() is called in early boot, and uses
__put_user_size(), which includes the allow/prevent calls to enforce
KUAP, which could either be called too early, or in the Radix case,
forced to use "early_" versions of functions just to safely handle
this one case.
__put_user_asm() does not do this, and thus is safe to use both in
early boot, and later on since in this case it should only ever be
touching kernel memory.
__patch_instruction() was previously refactored to use
__put_user_size() in order to be able to return -EFAULT, which would
allow the kernel to patch instructions in userspace, which should
never happen. This has the functional change of causing faults on
userspace addresses if KUAP is turned on, which should never happen in
practice.
A future enhancement could be to double check the patch address is
definitely allowed to be tampered with by the kernel.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: Unchanged.
arch/powerpc/lib/code-patching.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 506413a2c25e..42fdadac6587 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -26,9 +26,9 @@
static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
unsigned int *patch_addr)
{
- int err;
+ int err = 0;
- __put_user_size(instr, patch_addr, 4, err);
+ __put_user_asm(instr, patch_addr, err, "stw");
if (err)
return err;
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (6 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm() Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 8:48 ` Christophe Leroy
2019-03-08 1:16 ` [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults Michael Ellerman
8 siblings, 1 reply; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
From: Russell Currey <ruscur@russell.cc>
Kernel Userspace Access Prevention utilises a feature of the Radix MMU
which disallows read and write access to userspace addresses. By
utilising this, the kernel is prevented from accessing user data from
outside of trusted paths that perform proper safety checks, such as
copy_{to/from}_user() and friends.
Userspace access is disabled from early boot and is only enabled when
performing an operation like copy_{to/from}_user(). The register that
controls this (AMR) does not prevent userspace from accessing itself,
so there is no need to save and restore when entering and exiting
userspace.
When entering the kernel from the kernel we save AMR and if it is not
blocking user access (because eg. we faulted doing a user access) we
reblock user access for the duration of the exception (ie. the page
fault) and then restore the AMR when returning back to the kernel.
This feature has a slight performance impact which I roughly measured
to be 3% slower in the worst case (performing 1GB of 1 byte
read()/write() syscalls), and is gated behind the CONFIG_PPC_KUAP
option for performance-critical builds.
This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y)
and performing the following:
# (echo ACCESS_USERSPACE) > [debugfs]/provoke-crash/DIRECT
If enabled, this should send SIGSEGV to the thread.
mpe:
- Drop the unused paca flags.
- Zero the UAMOR to be safe.
- Save the AMR when we enter the kernel from the kernel and then
block user access again if it's not already blocked.
- Restore on the way back to the kernel.
- This means we handle nesting of interrupts properly, ie. we are
protected inside the page fault handler caused by a user access.
- Add paranoid checking of AMR in switch and syscall return.
- Add isync()'s around AMR writes as per the ISA.
- Support selectively disabling read or write, with no support for
nesting.
Co-authored-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5:
- On kernel entry check if the AMR is already blocking user access
and if so don't do the mtspr again, because it's slow (pointed out
by Nick) (in kuap_save_amr_and_lock).
- Rework the constants to make the asm a bit cleaner and avoid any
hard coded shifts.
- Selectively disable read or write, we don't support separately
nesting read/write access (use allow_user_access() instead) and
shouldn't need to (famous last words).
- Add isync() before & after setting AMR in set_kuap() as per the
ISA. We'll investigate whether they are both really needed in
future.
- Don't touch the AMR in hmi_exception_early() it never goes to
virtual mode.
- Check the full value in kuap_check_amr
v4:
- Drop the unused paca flags.
- Zero the UAMOR to be safe.
- Save the AMR when we enter the kernel from the kernel and then lock
it again.
- Restore on the way back to the kernel.
- That means we handle nesting of interrupts properly, ie. we are
protected inside the page fault handler caused by a user access.
- Add paranoid checking of AMR in switch and syscall return.
- Add an isync() to prevent_user_access()
.../powerpc/include/asm/book3s/64/kup-radix.h | 107 ++++++++++++++++++
arch/powerpc/include/asm/exception-64s.h | 2 +
arch/powerpc/include/asm/feature-fixups.h | 3 +
arch/powerpc/include/asm/kup.h | 4 +
arch/powerpc/include/asm/mmu.h | 10 +-
arch/powerpc/kernel/entry_64.S | 27 ++++-
arch/powerpc/kernel/exceptions-64s.S | 3 +
arch/powerpc/mm/pgtable-radix.c | 18 +++
arch/powerpc/mm/pkeys.c | 1 +
arch/powerpc/platforms/Kconfig.cputype | 8 ++
10 files changed, 180 insertions(+), 3 deletions(-)
create mode 100644 arch/powerpc/include/asm/book3s/64/kup-radix.h
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
+#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
+#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)
+ END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+#endif
+.endm
+
+.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
+ beq \use_cr, 99f
+ // We don't isync here because we very recently entered via rfid
+ mtspr SPRN_AMR, \gpr2
+ isync
+99:
+ END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
+#endif
+.endm
+
+#else /* !__ASSEMBLY__ */
+
+#ifdef CONFIG_PPC_KUAP
+
+#include <asm/reg.h>
+
+/*
+ * We support individually allowing read or write, but we don't support nesting
+ * because that would require an expensive read/modify write of the AMR.
+ */
+
+static inline void set_kuap(unsigned long value)
+{
+ if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ return;
+
+ /*
+ * ISA v3.0B says we need a CSI (Context Synchronising Instruction) both
+ * before and after the move to AMR. See table 6 on page 1134.
+ */
+ isync();
+ mtspr(SPRN_AMR, value);
+ isync();
+}
+
+static inline void allow_user_access(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ set_kuap(0);
+}
+
+static inline void allow_read_from_user(const void __user *from, unsigned long size)
+{
+ set_kuap(AMR_KUAP_BLOCK_WRITE);
+}
+
+static inline void allow_write_to_user(void __user *to, unsigned long size)
+{
+ set_kuap(AMR_KUAP_BLOCK_READ);
+}
+
+static inline void prevent_user_access(void __user *to, const void __user *from,
+ unsigned long size)
+{
+ set_kuap(AMR_KUAP_BLOCKED);
+}
+
+#endif /* CONFIG_PPC_KUAP */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
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; \
beq 4f; /* if from kernel mode */ \
ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
SAVE_PPR(area, r9); \
@@ -691,6 +692,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
*/
#define EXCEPTION_COMMON_NORET_STACK(area, trap, label, hdlr, additions) \
EXCEPTION_PROLOG_COMMON_1(); \
+ kuap_save_amr_and_lock r9, r10, cr1; \
EXCEPTION_PROLOG_COMMON_2(area); \
EXCEPTION_PROLOG_COMMON_3(trap); \
/* Volatile regs are potentially clobbered here */ \
diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h
index 40a6c9261a6b..f6fc31f8baff 100644
--- a/arch/powerpc/include/asm/feature-fixups.h
+++ b/arch/powerpc/include/asm/feature-fixups.h
@@ -100,6 +100,9 @@ label##5: \
#define END_MMU_FTR_SECTION(msk, val) \
END_MMU_FTR_SECTION_NESTED(msk, val, 97)
+#define END_MMU_FTR_SECTION_NESTED_IFSET(msk, label) \
+ END_MMU_FTR_SECTION_NESTED((msk), (msk), label)
+
#define END_MMU_FTR_SECTION_IFSET(msk) END_MMU_FTR_SECTION((msk), (msk))
#define END_MMU_FTR_SECTION_IFCLR(msk) END_MMU_FTR_SECTION((msk), 0)
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 4410625f4364..f79d4d970852 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -2,6 +2,10 @@
#ifndef _ASM_POWERPC_KUP_H_
#define _ASM_POWERPC_KUP_H_
+#ifdef CONFIG_PPC64
+#include <asm/book3s/64/kup-radix.h>
+#endif
+
#ifndef __ASSEMBLY__
#include <asm/pgtable.h>
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index d34ad1657d7b..59acb4418164 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h
@@ -107,6 +107,11 @@
*/
#define MMU_FTR_1T_SEGMENT ASM_CONST(0x40000000)
+/*
+ * Supports KUAP (key 0 controlling userspace addresses) on radix
+ */
+#define MMU_FTR_RADIX_KUAP ASM_CONST(0x80000000)
+
/* MMU feature bit sets for various CPUs */
#define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \
MMU_FTR_HPTE_TABLE | MMU_FTR_PPCAS_ARCH_V2
@@ -164,7 +169,10 @@ enum {
#endif
#ifdef CONFIG_PPC_RADIX_MMU
MMU_FTR_TYPE_RADIX |
-#endif
+#ifdef CONFIG_PPC_KUAP
+ MMU_FTR_RADIX_KUAP |
+#endif /* CONFIG_PPC_KUAP */
+#endif /* CONFIG_PPC_RADIX_MMU */
0,
};
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 15c67d2c0534..020d0ad9aa51 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -46,6 +46,7 @@
#include <asm/exception-64e.h>
#endif
#include <asm/feature-fixups.h>
+#include <asm/kup.h>
/*
* System calls.
@@ -120,6 +121,9 @@ END_BTB_FLUSH_SECTION
addi r9,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
std r11,-16(r9) /* "regshere" marker */
+
+ kuap_check_amr r10 r11
+
#if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) && defined(CONFIG_PPC_SPLPAR)
BEGIN_FW_FTR_SECTION
beq 33f
@@ -275,6 +279,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
andi. r6,r8,MSR_PR
ld r4,_LINK(r1)
+ kuap_check_amr r10 r11
+
#ifdef CONFIG_PPC_BOOK3S
/*
* Clear MSR_RI, MSR_EE is already and remains disabled. We could do
@@ -296,6 +302,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
std r8, PACATMSCRATCH(r13)
#endif
+ /*
+ * We don't need to restore AMR on the way back to userspace for KUAP.
+ * The value of AMR only matters while we're in the kernel.
+ */
ld r13,GPR13(r1) /* only restore r13 if returning to usermode */
ld r2,GPR2(r1)
ld r1,GPR1(r1)
@@ -306,8 +316,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
RFI_TO_USER
b . /* prevent speculative execution */
- /* exit to kernel */
-1: ld r2,GPR2(r1)
+1: /* exit to kernel */
+ kuap_restore_amr r2
+
+ ld r2,GPR2(r1)
ld r1,GPR1(r1)
mtlr r4
mtcr r5
@@ -594,6 +606,8 @@ _GLOBAL(_switch)
std r23,_CCR(r1)
std r1,KSP(r3) /* Set old stack pointer */
+ kuap_check_amr r9 r10
+
FLUSH_COUNT_CACHE
/*
@@ -942,6 +956,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
ld r4,_XER(r1)
mtspr SPRN_XER,r4
+ kuap_check_amr r5 r6
+
REST_8GPRS(5, r1)
andi. r0,r3,MSR_RI
@@ -974,6 +990,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
REST_GPR(13, r1)
+ /*
+ * We don't need to restore AMR on the way back to userspace for KUAP.
+ * The value of AMR only matters while we're in the kernel.
+ */
mtspr SPRN_SRR1,r3
ld r2,_CCR(r1)
@@ -1006,6 +1026,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
ld r0,GPR0(r1)
ld r2,GPR2(r1)
ld r3,GPR3(r1)
+
+ kuap_restore_amr r4
+
ld r4,GPR4(r1)
ld r1,GPR1(r1)
RFI_TO_KERNEL
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index a5b8fbae56a0..99b1bc4e190b 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -19,6 +19,7 @@
#include <asm/cpuidle.h>
#include <asm/head-64.h>
#include <asm/feature-fixups.h>
+#include <asm/kup.h>
/*
* There are a few constraints to be concerned with.
@@ -309,6 +310,7 @@ TRAMP_REAL_BEGIN(machine_check_common_early)
mfspr r11,SPRN_DSISR /* Save DSISR */
std r11,_DSISR(r1)
std r9,_CCR(r1) /* Save CR in stackframe */
+ kuap_save_amr_and_lock r9, r10, cr1
/* Save r9 through r13 from EXMC save area to stack frame. */
EXCEPTION_PROLOG_COMMON_2(PACA_EXMC)
mfmsr r11 /* get MSR value */
@@ -1097,6 +1099,7 @@ TRAMP_REAL_BEGIN(hmi_exception_early)
mfspr r11,SPRN_HSRR0 /* Save HSRR0 */
mfspr r12,SPRN_HSRR1 /* Save HSRR1 */
EXCEPTION_PROLOG_COMMON_1()
+ /* We don't touch AMR here, we never go to virtual mode */
EXCEPTION_PROLOG_COMMON_2(PACA_EXGEN)
EXCEPTION_PROLOG_COMMON_3(0xe60)
addi r3,r1,STACK_FRAME_OVERHEAD
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
@@ -29,6 +29,7 @@
#include <asm/powernv.h>
#include <asm/sections.h>
#include <asm/trace.h>
+#include <asm/uaccess.h>
#include <trace/events/thp.h>
@@ -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);
+}
+#endif
+
void __init radix__early_init_mmu(void)
{
unsigned long lpcr;
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 587807763737..ae7fca40e5b3 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -7,6 +7,7 @@
#include <asm/mman.h>
#include <asm/mmu_context.h>
+#include <asm/mmu.h>
#include <asm/setup.h>
#include <linux/pkeys.h>
#include <linux/of_device.h>
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
@@ -327,6 +327,7 @@ config PPC_RADIX_MMU
depends on PPC_BOOK3S_64
select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
select PPC_HAVE_KUEP
+ select PPC_HAVE_KUAP
default y
help
Enable support for the Power ISA 3.0 Radix style MMU. Currently this
@@ -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
+ help
+ Add extra debugging for Kernel Userspace Access Protection (KUAP)
+ If you're unsure, say N.
+
config ARCH_ENABLE_HUGEPAGE_MIGRATION
def_bool y
depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
` (7 preceding siblings ...)
2019-03-08 1:16 ` [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU Michael Ellerman
@ 2019-03-08 1:16 ` Michael Ellerman
2019-03-08 8:53 ` Christophe Leroy
8 siblings, 1 reply; 25+ messages in thread
From: Michael Ellerman @ 2019-03-08 1:16 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin
When KUAP is enabled we have logic to detect page faults that occur
outside of a valid user access region and are blocked by the AMR.
What we don't have at the moment is logic to detect a fault *within* a
valid user access region, that has been incorrectly blocked by AMR.
This is not meant to ever happen, but it can if we incorrectly
save/restore the AMR, or if the AMR was overwritten for some other
reason.
Currently if that happens we assume it's just a regular fault that
will be corrected by handling the fault normally, so we just return.
But there is nothing the fault handling code can do to fix it, so the
fault just happens again and we spin forever, leading to soft lockups.
So add some logic to detect that case and WARN() if we ever see it.
Arguably it should be a BUG(), but it's more polite to fail the access
and let the kernel continue, rather than taking down the box. There
should be no data integrity issue with failing the fault rather than
BUG'ing, as we're just going to disallow an access that should have
been allowed.
To make the code a little easier to follow, unroll the condition at
the end of bad_kernel_fault() and comment each case, before adding the
call to bad_kuap_fault().
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
v5: New.
.../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++
arch/powerpc/include/asm/kup.h | 1 +
arch/powerpc/mm/fault.c | 25 ++++++++++++++++---
3 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3d60b04fc3f6..8d2ddc61e92e 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
set_kuap(AMR_KUAP_BLOCKED);
}
+static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
+{
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
+ ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
+ (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))))
+ {
+ WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
+ return true;
+ }
+
+ return false;
+}
#endif /* CONFIG_PPC_KUAP */
#endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index f79d4d970852..ccbd2a249575 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
unsigned long size) { }
static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
static inline void allow_write_to_user(void __user *to, unsigned long size) {}
+static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
#endif /* CONFIG_PPC_KUAP */
static inline void prevent_read_from_user(const void __user *from, unsigned long size)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 463d1e9d026e..b5d3578d9f65 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -44,6 +44,7 @@
#include <asm/mmu_context.h>
#include <asm/siginfo.h>
#include <asm/debug.h>
+#include <asm/kup.h>
static inline bool notify_page_fault(struct pt_regs *regs)
{
@@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
/* Is this a bad kernel fault ? */
static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
- unsigned long address)
+ unsigned long address, bool is_write)
{
int is_exec = TRAP(regs) == 0x400;
@@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
address >= TASK_SIZE ? "exec-protected" : "user",
address,
from_kuid(&init_user_ns, current_uid()));
+
+ // Kernel exec fault is always bad
+ return true;
}
if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
@@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
from_kuid(&init_user_ns, current_uid()));
}
- return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip);
+ // Kernel fault on kernel address is bad
+ if (address >= TASK_SIZE)
+ return true;
+
+ // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
+ if (!search_exception_tables(regs->nip))
+ return true;
+
+ // Read/write fault in a valid region (the exception table search passed
+ // above), but blocked by KUAP is bad, it can never succeed.
+ if (bad_kuap_fault(regs, is_write))
+ return true;
+
+ // What's left? Kernel fault on user in well defined regions (extable
+ // matched), and allowed by KUAP in the faulting context.
+ return false;
}
static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
@@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
* take a page fault to a kernel address or a page fault to a user
* address outside of dedicated places
*/
- if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address)))
+ if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write)))
return SIGSEGV;
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-08 1:16 ` [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection Michael Ellerman
@ 2019-03-08 8:26 ` Christophe Leroy
2019-03-20 12:57 ` Michael Ellerman
2019-03-11 9:12 ` Christophe Leroy
1 sibling, 1 reply; 25+ messages in thread
From: Christophe Leroy @ 2019-03-08 8:26 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
> From: Christophe Leroy <christophe.leroy@c-s.fr>
>
> This patch implements a framework for Kernel Userspace Access
> Protection.
>
> Then subarches will have the possibility to provide their own
> implementation by providing setup_kuap() and
> allow/prevent_user_access().
>
> Some platforms will need to know the area accessed and whether it is
> accessed from read, write or both. Therefore source, destination and
> size and handed over to the two functions.
>
> mpe: Rename to allow/prevent rather than unlock/lock, and add
> read/write wrappers. Drop the 32-bit code for now until we have an
> implementation for it. Add kuap to pt_regs for 64-bit as well as
> 32-bit. Don't split strings, use pr_crit_ratelimited().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v5: Futex ops need read/write so use allow_user_acccess() there.
> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
> Allow subarch to override allow_read/write_from/to_user().
Those little helpers that will just call allow_user_access() when
distinct read/write handling is not performed looks overkill to me.
Can't the subarch do it by itself based on the nullity of from/to ?
static inline void allow_user_access(void __user *to, const void __user
*from,
unsigned long size)
{
if (to & from)
set_kuap(0);
else if (to)
set_kuap(AMR_KUAP_BLOCK_READ);
else if (from)
set_kuap(AMR_KUAP_BLOCK_WRITE);
}
Christophe
>
> v4: mpe: Rename to allow/prevent rather than unlock/lock, and add
> read/write wrappers. Drop the 32-bit code for now until we have an
> implementation for it. Add kuap to pt_regs for 64-bit as well as
> 32-bit. Don't split strings, use pr_crit_ratelimited().
>
> .../admin-guide/kernel-parameters.txt | 2 +-
> arch/powerpc/include/asm/futex.h | 4 ++
> arch/powerpc/include/asm/kup.h | 24 ++++++++++++
> arch/powerpc/include/asm/ptrace.h | 11 +++++-
> arch/powerpc/include/asm/uaccess.h | 38 +++++++++++++++----
> arch/powerpc/kernel/asm-offsets.c | 4 ++
> arch/powerpc/lib/checksum_wrappers.c | 4 ++
> arch/powerpc/mm/fault.c | 19 ++++++++--
> arch/powerpc/mm/init-common.c | 10 +++++
> arch/powerpc/platforms/Kconfig.cputype | 12 ++++++
> 10 files changed, 113 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f81d79de4de0..16883f2a05fd 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2809,7 +2809,7 @@
> noexec=on: enable non-executable mappings (default)
> noexec=off: disable non-executable mappings
>
> - nosmap [X86]
> + nosmap [X86,PPC]
> Disable SMAP (Supervisor Mode Access Prevention)
> even if it is supported by processor.
>
> diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
> index 88b38b37c21b..3a6aa57b9d90 100644
> --- a/arch/powerpc/include/asm/futex.h
> +++ b/arch/powerpc/include/asm/futex.h
> @@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> {
> int oldval = 0, ret;
>
> + allow_write_to_user(uaddr, sizeof(*uaddr));
> pagefault_disable();
>
> switch (op) {
> @@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> if (!ret)
> *oval = oldval;
>
> + prevent_write_to_user(uaddr, sizeof(*uaddr));
> return ret;
> }
>
> @@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> if (!access_ok(uaddr, sizeof(u32)))
> return -EFAULT;
>
> + allow_write_to_user(uaddr, sizeof(*uaddr));
> __asm__ __volatile__ (
> PPC_ATOMIC_ENTRY_BARRIER
> "1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\
> @@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> : "cc", "memory");
>
> *uval = prev;
> + prevent_write_to_user(uaddr, sizeof(*uaddr));
> return ret;
> }
>
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index a2a959cb4e36..4410625f4364 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -4,6 +4,8 @@
>
> #ifndef __ASSEMBLY__
>
> +#include <asm/pgtable.h>
> +
> void setup_kup(void);
>
> #ifdef CONFIG_PPC_KUEP
> @@ -12,6 +14,28 @@ void setup_kuep(bool disabled);
> static inline void setup_kuep(bool disabled) { }
> #endif /* CONFIG_PPC_KUEP */
>
> +#ifdef CONFIG_PPC_KUAP
> +void setup_kuap(bool disabled);
> +#else
> +static inline void setup_kuap(bool disabled) { }
> +static inline void allow_user_access(void __user *to, const void __user *from,
> + unsigned long size) { }
> +static inline void prevent_user_access(void __user *to, const void __user *from,
> + unsigned long size) { }
> +static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
> +static inline void allow_write_to_user(void __user *to, unsigned long size) {}
> +#endif /* CONFIG_PPC_KUAP */
> +
> +static inline void prevent_read_from_user(const void __user *from, unsigned long size)
> +{
> + prevent_user_access(NULL, from, size);
> +}
> +
> +static inline void prevent_write_to_user(void __user *to, unsigned long size)
> +{
> + prevent_user_access(to, NULL, size);
> +}
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_KUP_H_ */
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 64271e562fed..6f047730e642 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -52,10 +52,17 @@ struct pt_regs
> };
> };
>
> + union {
> + struct {
> #ifdef CONFIG_PPC64
> - unsigned long ppr;
> - unsigned long __pad; /* Maintain 16 byte interrupt stack alignment */
> + unsigned long ppr;
> +#endif
> +#ifdef CONFIG_PPC_KUAP
> + unsigned long kuap;
> #endif
> + };
> + unsigned long __pad[2]; /* Maintain 16 byte interrupt stack alignment */
> + };
> };
> #endif
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index e3a731793ea2..fb7651a5488b 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -6,6 +6,7 @@
> #include <asm/processor.h>
> #include <asm/page.h>
> #include <asm/extable.h>
> +#include <asm/kup.h>
>
> /*
> * The fs value determines whether argument validity checking should be
> @@ -141,6 +142,7 @@ extern long __put_user_bad(void);
> #define __put_user_size(x, ptr, size, retval) \
> do { \
> retval = 0; \
> + allow_write_to_user(ptr, size); \
> switch (size) { \
> case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
> case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
> @@ -148,6 +150,7 @@ do { \
> case 8: __put_user_asm2(x, ptr, retval); break; \
> default: __put_user_bad(); \
> } \
> + prevent_write_to_user(ptr, size); \
> } while (0)
>
> #define __put_user_nocheck(x, ptr, size) \
> @@ -240,6 +243,7 @@ do { \
> __chk_user_ptr(ptr); \
> if (size > sizeof(x)) \
> (x) = __get_user_bad(); \
> + allow_read_from_user(ptr, size); \
> switch (size) { \
> case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
> case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
> @@ -247,6 +251,7 @@ do { \
> case 8: __get_user_asm2(x, ptr, retval); break; \
> default: (x) = __get_user_bad(); \
> } \
> + prevent_read_from_user(ptr, size); \
> } while (0)
>
> /*
> @@ -306,15 +311,21 @@ extern unsigned long __copy_tofrom_user(void __user *to,
> static inline unsigned long
> raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> {
> - return __copy_tofrom_user(to, from, n);
> + unsigned long ret;
> +
> + allow_user_access(to, from, n);
> + ret = __copy_tofrom_user(to, from, n);
> + prevent_user_access(to, from, n);
> + return ret;
> }
> #endif /* __powerpc64__ */
>
> static inline unsigned long raw_copy_from_user(void *to,
> const void __user *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -339,14 +350,18 @@ static inline unsigned long raw_copy_from_user(void *to,
> }
>
> barrier_nospec();
> - return __copy_tofrom_user((__force void __user *)to, from, n);
> + allow_read_from_user(from, n);
> + ret = __copy_tofrom_user((__force void __user *)to, from, n);
> + prevent_read_from_user(from, n);
> + return ret;
> }
>
> static inline unsigned long raw_copy_to_user(void __user *to,
> const void *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -366,17 +381,24 @@ static inline unsigned long raw_copy_to_user(void __user *to,
> return 0;
> }
>
> - return __copy_tofrom_user(to, (__force const void __user *)from, n);
> + allow_write_to_user(to, n);
> + ret = __copy_tofrom_user(to, (__force const void __user *)from, n);
> + prevent_write_to_user(to, n);
> + return ret;
> }
>
> extern unsigned long __clear_user(void __user *addr, unsigned long size);
>
> static inline unsigned long clear_user(void __user *addr, unsigned long size)
> {
> + unsigned long ret = size;
> might_fault();
> - if (likely(access_ok(addr, size)))
> - return __clear_user(addr, size);
> - return size;
> + if (likely(access_ok(addr, size))) {
> + allow_write_to_user(addr, size);
> + ret = __clear_user(addr, size);
> + prevent_write_to_user(addr, size);
> + }
> + return ret;
> }
>
> extern long strncpy_from_user(char *dst, const char __user *src, long count);
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 86a61e5f8285..66202e02fee2 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -332,6 +332,10 @@ int main(void)
> STACK_PT_REGS_OFFSET(_PPR, ppr);
> #endif /* CONFIG_PPC64 */
>
> +#ifdef CONFIG_PPC_KUAP
> + STACK_PT_REGS_OFFSET(STACK_REGS_KUAP, kuap);
> +#endif
> +
> #if defined(CONFIG_PPC32)
> #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> DEFINE(EXC_LVL_SIZE, STACK_EXC_LVL_FRAME_SIZE);
> diff --git a/arch/powerpc/lib/checksum_wrappers.c b/arch/powerpc/lib/checksum_wrappers.c
> index 890d4ddd91d6..bb9307ce2440 100644
> --- a/arch/powerpc/lib/checksum_wrappers.c
> +++ b/arch/powerpc/lib/checksum_wrappers.c
> @@ -29,6 +29,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
> unsigned int csum;
>
> might_sleep();
> + allow_read_from_user(src, len);
>
> *err_ptr = 0;
>
> @@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
> }
>
> out:
> + prevent_read_from_user(src, len);
> return (__force __wsum)csum;
> }
> EXPORT_SYMBOL(csum_and_copy_from_user);
> @@ -70,6 +72,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
> unsigned int csum;
>
> might_sleep();
> + allow_write_to_user(dst, len);
>
> *err_ptr = 0;
>
> @@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
> }
>
> out:
> + prevent_write_to_user(dst, len);
> return (__force __wsum)csum;
> }
> EXPORT_SYMBOL(csum_and_copy_to_user);
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 3384354abc1d..463d1e9d026e 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -223,9 +223,11 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
> }
>
> /* Is this a bad kernel fault ? */
> -static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
> +static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> unsigned long address)
> {
> + int is_exec = TRAP(regs) == 0x400;
> +
> /* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
> if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> DSISR_PROTFAULT))) {
> @@ -234,7 +236,15 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
> address,
> from_kuid(&init_user_ns, current_uid()));
> }
> - return is_exec || (address >= TASK_SIZE);
> +
> + if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> + !search_exception_tables(regs->nip)) {
> + pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> + address,
> + from_kuid(&init_user_ns, current_uid()));
> + }
> +
> + return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip);
> }
>
> static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> @@ -454,9 +464,10 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>
> /*
> * The kernel should never take an execute fault nor should it
> - * take a page fault to a kernel address.
> + * take a page fault to a kernel address or a page fault to a user
> + * address outside of dedicated places
> */
> - if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address)))
> + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address)))
> return SIGSEGV;
>
> /*
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 83f95a5565d6..ecaedfff9992 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -27,6 +27,7 @@
> #include <asm/kup.h>
>
> static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
> +static bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>
> static int __init parse_nosmep(char *p)
> {
> @@ -36,9 +37,18 @@ static int __init parse_nosmep(char *p)
> }
> early_param("nosmep", parse_nosmep);
>
> +static int __init parse_nosmap(char *p)
> +{
> + disable_kuap = true;
> + pr_warn("Disabling Kernel Userspace Access Protection\n");
> + return 0;
> +}
> +early_param("nosmap", parse_nosmap);
> +
> void __init setup_kup(void)
> {
> setup_kuep(disable_kuep);
> + setup_kuap(disable_kuap);
> }
>
> #define CTOR(shift) static void ctor_##shift(void *addr) \
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 7d30bbbaa3c1..457fc3a5ed93 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -357,6 +357,18 @@ config PPC_KUEP
>
> If you're unsure, say Y.
>
> +config PPC_HAVE_KUAP
> + bool
> +
> +config PPC_KUAP
> + bool "Kernel Userspace Access Protection"
> + depends on PPC_HAVE_KUAP
> + default y
> + help
> + Enable support for Kernel Userspace Access Protection (KUAP)
> +
> + If you're unsure, say Y.
> +
> config ARCH_ENABLE_HUGEPAGE_MIGRATION
> def_bool y
> depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm()
2019-03-08 1:16 ` [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm() Michael Ellerman
@ 2019-03-08 8:32 ` Christophe Leroy
0 siblings, 0 replies; 25+ messages in thread
From: Christophe Leroy @ 2019-03-08 8:32 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> __patch_instruction() is called in early boot, and uses
> __put_user_size(), which includes the allow/prevent calls to enforce
> KUAP, which could either be called too early, or in the Radix case,
> forced to use "early_" versions of functions just to safely handle
> this one case.
>
> __put_user_asm() does not do this, and thus is safe to use both in
> early boot, and later on since in this case it should only ever be
> touching kernel memory.
>
> __patch_instruction() was previously refactored to use
> __put_user_size() in order to be able to return -EFAULT, which would
> allow the kernel to patch instructions in userspace, which should
> never happen. This has the functional change of causing faults on
> userspace addresses if KUAP is turned on, which should never happen in
> practice.
What about modules patching, is there no risk of -EFAULT on module
memory, as it is in vm area ?
Christophe
>
> A future enhancement could be to double check the patch address is
> definitely allowed to be tampered with by the kernel.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v5: Unchanged.
>
> arch/powerpc/lib/code-patching.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 506413a2c25e..42fdadac6587 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -26,9 +26,9 @@
> static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
> unsigned int *patch_addr)
> {
> - int err;
> + int err = 0;
>
> - __put_user_size(instr, patch_addr, 4, err);
> + __put_user_asm(instr, patch_addr, err, "stw");
> if (err)
> return err;
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU
2019-03-08 1:16 ` [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU Michael Ellerman
@ 2019-03-08 8:48 ` Christophe Leroy
2019-04-17 13:39 ` Michael Ellerman
0 siblings, 1 reply; 25+ messages in thread
From: Christophe Leroy @ 2019-03-08 8:48 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> Kernel Userspace Access Prevention utilises a feature of the Radix MMU
> which disallows read and write access to userspace addresses. By
> utilising this, the kernel is prevented from accessing user data from
> outside of trusted paths that perform proper safety checks, such as
> copy_{to/from}_user() and friends.
>
> Userspace access is disabled from early boot and is only enabled when
> performing an operation like copy_{to/from}_user(). The register that
> controls this (AMR) does not prevent userspace from accessing itself,
> so there is no need to save and restore when entering and exiting
> userspace.
>
> When entering the kernel from the kernel we save AMR and if it is not
> blocking user access (because eg. we faulted doing a user access) we
> reblock user access for the duration of the exception (ie. the page
> fault) and then restore the AMR when returning back to the kernel.
>
> This feature has a slight performance impact which I roughly measured
> to be 3% slower in the worst case (performing 1GB of 1 byte
> read()/write() syscalls), and is gated behind the CONFIG_PPC_KUAP
> option for performance-critical builds.
>
> This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y)
> and performing the following:
>
> # (echo ACCESS_USERSPACE) > [debugfs]/provoke-crash/DIRECT
>
> If enabled, this should send SIGSEGV to the thread.
>
> mpe:
> - Drop the unused paca flags.
> - Zero the UAMOR to be safe.
> - Save the AMR when we enter the kernel from the kernel and then
> block user access again if it's not already blocked.
> - Restore on the way back to the kernel.
> - This means we handle nesting of interrupts properly, ie. we are
> protected inside the page fault handler caused by a user access.
> - Add paranoid checking of AMR in switch and syscall return.
> - Add isync()'s around AMR writes as per the ISA.
> - Support selectively disabling read or write, with no support for
> nesting.
>
> Co-authored-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>
> v5:
> - On kernel entry check if the AMR is already blocking user access
> and if so don't do the mtspr again, because it's slow (pointed out
> by Nick) (in kuap_save_amr_and_lock).
> - Rework the constants to make the asm a bit cleaner and avoid any
> hard coded shifts.
> - Selectively disable read or write, we don't support separately
> nesting read/write access (use allow_user_access() instead) and
> shouldn't need to (famous last words).
> - Add isync() before & after setting AMR in set_kuap() as per the
> ISA. We'll investigate whether they are both really needed in
> future.
> - Don't touch the AMR in hmi_exception_early() it never goes to
> virtual mode.
> - Check the full value in kuap_check_amr
>
> v4:
> - Drop the unused paca flags.
> - Zero the UAMOR to be safe.
> - Save the AMR when we enter the kernel from the kernel and then lock
> it again.
> - Restore on the way back to the kernel.
> - That means we handle nesting of interrupts properly, ie. we are
> protected inside the page fault handler caused by a user access.
> - Add paranoid checking of AMR in switch and syscall return.
> - Add an isync() to prevent_user_access()
>
> .../powerpc/include/asm/book3s/64/kup-radix.h | 107 ++++++++++++++++++
> arch/powerpc/include/asm/exception-64s.h | 2 +
> arch/powerpc/include/asm/feature-fixups.h | 3 +
> arch/powerpc/include/asm/kup.h | 4 +
> arch/powerpc/include/asm/mmu.h | 10 +-
> arch/powerpc/kernel/entry_64.S | 27 ++++-
> arch/powerpc/kernel/exceptions-64s.S | 3 +
> arch/powerpc/mm/pgtable-radix.c | 18 +++
> arch/powerpc/mm/pkeys.c | 1 +
> arch/powerpc/platforms/Kconfig.cputype | 8 ++
> 10 files changed, 180 insertions(+), 3 deletions(-)
> create mode 100644 arch/powerpc/include/asm/book3s/64/kup-radix.h
>
> 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 ?
> +#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
> +#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 |
> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
> +#endif
> +.endm
> +
> +.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) ?
> + 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.
> + mtspr SPRN_AMR, \gpr2
> + isync
> +99:
> + END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
> +#endif
> +.endm
> +
> +#else /* !__ASSEMBLY__ */
> +
> +#ifdef CONFIG_PPC_KUAP
> +
> +#include <asm/reg.h>
> +
> +/*
> + * We support individually allowing read or write, but we don't support nesting
> + * because that would require an expensive read/modify write of the AMR.
> + */
> +
> +static inline void set_kuap(unsigned long value)
> +{
> + if (!mmu_has_feature(MMU_FTR_RADIX_KUAP))
> + return;
> +
> + /*
> + * ISA v3.0B says we need a CSI (Context Synchronising Instruction) both
> + * before and after the move to AMR. See table 6 on page 1134.
> + */
> + isync();
> + mtspr(SPRN_AMR, value);
> + isync();
> +}
> +
> +static inline void allow_user_access(void __user *to, const void __user *from,
> + unsigned long size)
> +{
> + set_kuap(0);
> +}
> +
> +static inline void allow_read_from_user(const void __user *from, unsigned long size)
> +{
> + set_kuap(AMR_KUAP_BLOCK_WRITE);
> +}
> +
> +static inline void allow_write_to_user(void __user *to, unsigned long size)
> +{
> + set_kuap(AMR_KUAP_BLOCK_READ);
> +}
> +
> +static inline void prevent_user_access(void __user *to, const void __user *from,
> + unsigned long size)
> +{
> + set_kuap(AMR_KUAP_BLOCKED);
> +}
> +
> +#endif /* CONFIG_PPC_KUAP */
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _ASM_POWERPC_BOOK3S_64_KUP_RADIX_H */
> 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 ?
> beq 4f; /* if from kernel mode */ \
> ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
> SAVE_PPR(area, r9); \
> @@ -691,6 +692,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_CTRL)
> */
> #define EXCEPTION_COMMON_NORET_STACK(area, trap, label, hdlr, additions) \
> EXCEPTION_PROLOG_COMMON_1(); \
> + kuap_save_amr_and_lock r9, r10, cr1; \
> EXCEPTION_PROLOG_COMMON_2(area); \
> EXCEPTION_PROLOG_COMMON_3(trap); \
> /* Volatile regs are potentially clobbered here */ \
> diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h
> index 40a6c9261a6b..f6fc31f8baff 100644
> --- a/arch/powerpc/include/asm/feature-fixups.h
> +++ b/arch/powerpc/include/asm/feature-fixups.h
> @@ -100,6 +100,9 @@ label##5: \
> #define END_MMU_FTR_SECTION(msk, val) \
> END_MMU_FTR_SECTION_NESTED(msk, val, 97)
>
> +#define END_MMU_FTR_SECTION_NESTED_IFSET(msk, label) \
> + END_MMU_FTR_SECTION_NESTED((msk), (msk), label)
> +
> #define END_MMU_FTR_SECTION_IFSET(msk) END_MMU_FTR_SECTION((msk), (msk))
> #define END_MMU_FTR_SECTION_IFCLR(msk) END_MMU_FTR_SECTION((msk), 0)
>
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index 4410625f4364..f79d4d970852 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -2,6 +2,10 @@
> #ifndef _ASM_POWERPC_KUP_H_
> #define _ASM_POWERPC_KUP_H_
>
> +#ifdef CONFIG_PPC64
> +#include <asm/book3s/64/kup-radix.h>
> +#endif
> +
> #ifndef __ASSEMBLY__
>
> #include <asm/pgtable.h>
> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
> index d34ad1657d7b..59acb4418164 100644
> --- a/arch/powerpc/include/asm/mmu.h
> +++ b/arch/powerpc/include/asm/mmu.h
> @@ -107,6 +107,11 @@
> */
> #define MMU_FTR_1T_SEGMENT ASM_CONST(0x40000000)
>
> +/*
> + * Supports KUAP (key 0 controlling userspace addresses) on radix
> + */
> +#define MMU_FTR_RADIX_KUAP ASM_CONST(0x80000000)
> +
> /* MMU feature bit sets for various CPUs */
> #define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \
> MMU_FTR_HPTE_TABLE | MMU_FTR_PPCAS_ARCH_V2
> @@ -164,7 +169,10 @@ enum {
> #endif
> #ifdef CONFIG_PPC_RADIX_MMU
> MMU_FTR_TYPE_RADIX |
> -#endif
> +#ifdef CONFIG_PPC_KUAP
> + MMU_FTR_RADIX_KUAP |
> +#endif /* CONFIG_PPC_KUAP */
> +#endif /* CONFIG_PPC_RADIX_MMU */
> 0,
> };
>
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index 15c67d2c0534..020d0ad9aa51 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -46,6 +46,7 @@
> #include <asm/exception-64e.h>
> #endif
> #include <asm/feature-fixups.h>
> +#include <asm/kup.h>
>
> /*
> * System calls.
> @@ -120,6 +121,9 @@ END_BTB_FLUSH_SECTION
> addi r9,r1,STACK_FRAME_OVERHEAD
> ld r11,exception_marker@toc(r2)
> std r11,-16(r9) /* "regshere" marker */
> +
> + kuap_check_amr r10 r11
> +
> #if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE) && defined(CONFIG_PPC_SPLPAR)
> BEGIN_FW_FTR_SECTION
> beq 33f
> @@ -275,6 +279,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> andi. r6,r8,MSR_PR
> ld r4,_LINK(r1)
>
> + kuap_check_amr r10 r11
> +
> #ifdef CONFIG_PPC_BOOK3S
> /*
> * Clear MSR_RI, MSR_EE is already and remains disabled. We could do
> @@ -296,6 +302,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> std r8, PACATMSCRATCH(r13)
> #endif
>
> + /*
> + * We don't need to restore AMR on the way back to userspace for KUAP.
> + * The value of AMR only matters while we're in the kernel.
> + */
> ld r13,GPR13(r1) /* only restore r13 if returning to usermode */
> ld r2,GPR2(r1)
> ld r1,GPR1(r1)
> @@ -306,8 +316,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> RFI_TO_USER
> b . /* prevent speculative execution */
>
> - /* exit to kernel */
> -1: ld r2,GPR2(r1)
> +1: /* exit to kernel */
> + kuap_restore_amr r2
> +
> + ld r2,GPR2(r1)
> ld r1,GPR1(r1)
> mtlr r4
> mtcr r5
> @@ -594,6 +606,8 @@ _GLOBAL(_switch)
> std r23,_CCR(r1)
> std r1,KSP(r3) /* Set old stack pointer */
>
> + kuap_check_amr r9 r10
> +
> FLUSH_COUNT_CACHE
>
> /*
> @@ -942,6 +956,8 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> ld r4,_XER(r1)
> mtspr SPRN_XER,r4
>
> + kuap_check_amr r5 r6
> +
> REST_8GPRS(5, r1)
>
> andi. r0,r3,MSR_RI
> @@ -974,6 +990,10 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
> REST_GPR(13, r1)
>
> + /*
> + * We don't need to restore AMR on the way back to userspace for KUAP.
> + * The value of AMR only matters while we're in the kernel.
> + */
> mtspr SPRN_SRR1,r3
>
> ld r2,_CCR(r1)
> @@ -1006,6 +1026,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> ld r0,GPR0(r1)
> ld r2,GPR2(r1)
> ld r3,GPR3(r1)
> +
> + kuap_restore_amr r4
> +
> ld r4,GPR4(r1)
> ld r1,GPR1(r1)
> RFI_TO_KERNEL
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index a5b8fbae56a0..99b1bc4e190b 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -19,6 +19,7 @@
> #include <asm/cpuidle.h>
> #include <asm/head-64.h>
> #include <asm/feature-fixups.h>
> +#include <asm/kup.h>
>
> /*
> * There are a few constraints to be concerned with.
> @@ -309,6 +310,7 @@ TRAMP_REAL_BEGIN(machine_check_common_early)
> mfspr r11,SPRN_DSISR /* Save DSISR */
> std r11,_DSISR(r1)
> std r9,_CCR(r1) /* Save CR in stackframe */
> + kuap_save_amr_and_lock r9, r10, cr1
> /* Save r9 through r13 from EXMC save area to stack frame. */
> EXCEPTION_PROLOG_COMMON_2(PACA_EXMC)
> mfmsr r11 /* get MSR value */
> @@ -1097,6 +1099,7 @@ TRAMP_REAL_BEGIN(hmi_exception_early)
> mfspr r11,SPRN_HSRR0 /* Save HSRR0 */
> mfspr r12,SPRN_HSRR1 /* Save HSRR1 */
> EXCEPTION_PROLOG_COMMON_1()
> + /* We don't touch AMR here, we never go to virtual mode */
> EXCEPTION_PROLOG_COMMON_2(PACA_EXGEN)
> EXCEPTION_PROLOG_COMMON_3(0xe60)
> addi r3,r1,STACK_FRAME_OVERHEAD
> 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
> @@ -29,6 +29,7 @@
> #include <asm/powernv.h>
> #include <asm/sections.h>
> #include <asm/trace.h>
> +#include <asm/uaccess.h>
>
> #include <trace/events/thp.h>
>
> @@ -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 ?
> +}
> +#endif
> +
> void __init radix__early_init_mmu(void)
> {
> unsigned long lpcr;
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 587807763737..ae7fca40e5b3 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -7,6 +7,7 @@
>
> #include <asm/mman.h>
> #include <asm/mmu_context.h>
> +#include <asm/mmu.h>
> #include <asm/setup.h>
> #include <linux/pkeys.h>
> #include <linux/of_device.h>
> 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
> @@ -327,6 +327,7 @@ config PPC_RADIX_MMU
> depends on PPC_BOOK3S_64
> select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> select PPC_HAVE_KUEP
> + select PPC_HAVE_KUAP
> default y
> help
> Enable support for the Power ISA 3.0 Radix style MMU. Currently this
> @@ -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 ?
> + help
> + Add extra debugging for Kernel Userspace Access Protection (KUAP)
> + If you're unsure, say N.
> +
> config ARCH_ENABLE_HUGEPAGE_MIGRATION
> def_bool y
> depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
>
Christophe
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults
2019-03-08 1:16 ` [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults Michael Ellerman
@ 2019-03-08 8:53 ` Christophe Leroy
2019-03-09 12:49 ` christophe leroy
2019-04-17 13:12 ` Michael Ellerman
0 siblings, 2 replies; 25+ messages in thread
From: Christophe Leroy @ 2019-03-08 8:53 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
> When KUAP is enabled we have logic to detect page faults that occur
> outside of a valid user access region and are blocked by the AMR.
>
> What we don't have at the moment is logic to detect a fault *within* a
> valid user access region, that has been incorrectly blocked by AMR.
> This is not meant to ever happen, but it can if we incorrectly
> save/restore the AMR, or if the AMR was overwritten for some other
> reason.
>
> Currently if that happens we assume it's just a regular fault that
> will be corrected by handling the fault normally, so we just return.
> But there is nothing the fault handling code can do to fix it, so the
> fault just happens again and we spin forever, leading to soft lockups.
>
> So add some logic to detect that case and WARN() if we ever see it.
> Arguably it should be a BUG(), but it's more polite to fail the access
> and let the kernel continue, rather than taking down the box. There
> should be no data integrity issue with failing the fault rather than
> BUG'ing, as we're just going to disallow an access that should have
> been allowed.
>
> To make the code a little easier to follow, unroll the condition at
> the end of bad_kernel_fault() and comment each case, before adding the
> call to bad_kuap_fault().
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>
> v5: New.
>
> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++
> arch/powerpc/include/asm/kup.h | 1 +
> arch/powerpc/mm/fault.c | 25 ++++++++++++++++---
> 3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> index 3d60b04fc3f6..8d2ddc61e92e 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
> set_kuap(AMR_KUAP_BLOCKED);
> }
>
> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
> +{
> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))))
> + {
Should this { go on the previous line ?
> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
> + return true;
Could just be
return WARN(true, ....)
Or even
return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
(!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...);
> + }
> +
> + return false;
> +}
> #endif /* CONFIG_PPC_KUAP */
>
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index f79d4d970852..ccbd2a249575 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
> unsigned long size) { }
> static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
> static inline void allow_write_to_user(void __user *to, unsigned long size) {}
> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
> #endif /* CONFIG_PPC_KUAP */
>
> static inline void prevent_read_from_user(const void __user *from, unsigned long size)
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 463d1e9d026e..b5d3578d9f65 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -44,6 +44,7 @@
> #include <asm/mmu_context.h>
> #include <asm/siginfo.h>
> #include <asm/debug.h>
> +#include <asm/kup.h>
>
> static inline bool notify_page_fault(struct pt_regs *regs)
> {
> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>
> /* Is this a bad kernel fault ? */
> static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> - unsigned long address)
> + unsigned long address, bool is_write)
We have regs, do we need is_write in addition ?
Christophe
> {
> int is_exec = TRAP(regs) == 0x400;
>
> @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> address >= TASK_SIZE ? "exec-protected" : "user",
> address,
> from_kuid(&init_user_ns, current_uid()));
> +
> + // Kernel exec fault is always bad
> + return true;
> }
>
> if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> from_kuid(&init_user_ns, current_uid()));
> }
>
> - return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip);
> + // Kernel fault on kernel address is bad
> + if (address >= TASK_SIZE)
> + return true;
> +
> + // Fault on user outside of certain regions (eg. copy_tofrom_user()) is bad
> + if (!search_exception_tables(regs->nip))
> + return true;
> +
> + // Read/write fault in a valid region (the exception table search passed
> + // above), but blocked by KUAP is bad, it can never succeed.
> + if (bad_kuap_fault(regs, is_write))
> + return true;
> +
> + // What's left? Kernel fault on user in well defined regions (extable
> + // matched), and allowed by KUAP in the faulting context.
> + return false;
> }
>
> static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
> * take a page fault to a kernel address or a page fault to a user
> * address outside of dedicated places
> */
> - if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address)))
> + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address, is_write)))
> return SIGSEGV;
>
> /*
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults
2019-03-08 8:53 ` Christophe Leroy
@ 2019-03-09 12:49 ` christophe leroy
2019-04-17 13:17 ` Michael Ellerman
2019-04-17 13:12 ` Michael Ellerman
1 sibling, 1 reply; 25+ messages in thread
From: christophe leroy @ 2019-03-09 12:49 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 09:53, Christophe Leroy a écrit :
>
>
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>> When KUAP is enabled we have logic to detect page faults that occur
>> outside of a valid user access region and are blocked by the AMR.
>>
>> What we don't have at the moment is logic to detect a fault *within* a
>> valid user access region, that has been incorrectly blocked by AMR.
>> This is not meant to ever happen, but it can if we incorrectly
>> save/restore the AMR, or if the AMR was overwritten for some other
>> reason.
>>
>> Currently if that happens we assume it's just a regular fault that
>> will be corrected by handling the fault normally, so we just return.
>> But there is nothing the fault handling code can do to fix it, so the
>> fault just happens again and we spin forever, leading to soft lockups.
>>
>> So add some logic to detect that case and WARN() if we ever see it.
>> Arguably it should be a BUG(), but it's more polite to fail the access
>> and let the kernel continue, rather than taking down the box. There
>> should be no data integrity issue with failing the fault rather than
>> BUG'ing, as we're just going to disallow an access that should have
>> been allowed.
>>
>> To make the code a little easier to follow, unroll the condition at
>> the end of bad_kernel_fault() and comment each case, before adding the
>> call to bad_kuap_fault().
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>
>> v5: New.
>>
>> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++
>> arch/powerpc/include/asm/kup.h | 1 +
>> arch/powerpc/mm/fault.c | 25 ++++++++++++++++---
>> 3 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> index 3d60b04fc3f6..8d2ddc61e92e 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void
>> __user *to, const void __user *from,
>> set_kuap(AMR_KUAP_BLOCKED);
>> }
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
>> +{
>> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
>> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
>> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))))
>> + {
>
> Should this { go on the previous line ?
>
>> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ?
>> "Write" : "Read");
>> + return true;
>
> Could just be
> return WARN(true, ....)
>
> Or even
> return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
> ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
> (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...);
Could also be simplified as follows since (is_write && ...) and
(!is_write && ...) are mutually exclusive:
mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ))
Christophe
>
>> + }
>> +
>> + return false;
>> +}
>> #endif /* CONFIG_PPC_KUAP */
>> #endif /* __ASSEMBLY__ */
>> diff --git a/arch/powerpc/include/asm/kup.h
>> b/arch/powerpc/include/asm/kup.h
>> index f79d4d970852..ccbd2a249575 100644
>> --- a/arch/powerpc/include/asm/kup.h
>> +++ b/arch/powerpc/include/asm/kup.h
>> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user
>> *to, const void __user *from,
>> unsigned long size) { }
>> static inline void allow_read_from_user(const void __user *from,
>> unsigned long size) {}
>> static inline void allow_write_to_user(void __user *to, unsigned
>> long size) {}
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool
>> is_write) { return false; }
>> #endif /* CONFIG_PPC_KUAP */
>> static inline void prevent_read_from_user(const void __user *from,
>> unsigned long size)
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 463d1e9d026e..b5d3578d9f65 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -44,6 +44,7 @@
>> #include <asm/mmu_context.h>
>> #include <asm/siginfo.h>
>> #include <asm/debug.h>
>> +#include <asm/kup.h>
>> static inline bool notify_page_fault(struct pt_regs *regs)
>> {
>> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs,
>> unsigned long addr,
>> /* Is this a bad kernel fault ? */
>> static bool bad_kernel_fault(struct pt_regs *regs, unsigned long
>> error_code,
>> - unsigned long address)
>> + unsigned long address, bool is_write)
>
> We have regs, do we need is_write in addition ?
>
> Christophe
>
>> {
>> int is_exec = TRAP(regs) == 0x400;
>> @@ -235,6 +236,9 @@ static bool bad_kernel_fault(struct pt_regs *regs,
>> unsigned long error_code,
>> address >= TASK_SIZE ? "exec-protected" : "user",
>> address,
>> from_kuid(&init_user_ns, current_uid()));
>> +
>> + // Kernel exec fault is always bad
>> + return true;
>> }
>> if (!is_exec && address < TASK_SIZE && (error_code &
>> DSISR_PROTFAULT) &&
>> @@ -244,7 +248,22 @@ static bool bad_kernel_fault(struct pt_regs
>> *regs, unsigned long error_code,
>> from_kuid(&init_user_ns, current_uid()));
>> }
>> - return is_exec || (address >= TASK_SIZE) ||
>> !search_exception_tables(regs->nip);
>> + // Kernel fault on kernel address is bad
>> + if (address >= TASK_SIZE)
>> + return true;
>> +
>> + // Fault on user outside of certain regions (eg.
>> copy_tofrom_user()) is bad
>> + if (!search_exception_tables(regs->nip))
>> + return true;
>> +
>> + // Read/write fault in a valid region (the exception table search
>> passed
>> + // above), but blocked by KUAP is bad, it can never succeed.
>> + if (bad_kuap_fault(regs, is_write))
>> + return true;
>> +
>> + // What's left? Kernel fault on user in well defined regions
>> (extable
>> + // matched), and allowed by KUAP in the faulting context.
>> + return false;
>> }
>> static bool bad_stack_expansion(struct pt_regs *regs, unsigned long
>> address,
>> @@ -467,7 +486,7 @@ static int __do_page_fault(struct pt_regs *regs,
>> unsigned long address,
>> * take a page fault to a kernel address or a page fault to a user
>> * address outside of dedicated places
>> */
>> - if (unlikely(!is_user && bad_kernel_fault(regs, error_code,
>> address)))
>> + if (unlikely(!is_user && bad_kernel_fault(regs, error_code,
>> address, is_write)))
>> return SIGSEGV;
>> /*
>>
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-08 1:16 ` [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection Michael Ellerman
2019-03-08 8:26 ` Christophe Leroy
@ 2019-03-11 9:12 ` Christophe Leroy
1 sibling, 0 replies; 25+ messages in thread
From: Christophe Leroy @ 2019-03-11 9:12 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
> From: Christophe Leroy <christophe.leroy@c-s.fr>
>
> This patch implements a framework for Kernel Userspace Access
> Protection.
>
> Then subarches will have the possibility to provide their own
> implementation by providing setup_kuap() and
> allow/prevent_user_access().
>
> Some platforms will need to know the area accessed and whether it is
> accessed from read, write or both. Therefore source, destination and
> size and handed over to the two functions.
>
> mpe: Rename to allow/prevent rather than unlock/lock, and add
> read/write wrappers. Drop the 32-bit code for now until we have an
> implementation for it. Add kuap to pt_regs for 64-bit as well as
> 32-bit. Don't split strings, use pr_crit_ratelimited().
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v5: Futex ops need read/write so use allow_user_acccess() there.
> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
> Allow subarch to override allow_read/write_from/to_user().
>
> v4: mpe: Rename to allow/prevent rather than unlock/lock, and add
> read/write wrappers. Drop the 32-bit code for now until we have an
> implementation for it. Add kuap to pt_regs for 64-bit as well as
> 32-bit. Don't split strings, use pr_crit_ratelimited().
We know have on top of v5 an implementation for 32-bit 8xx and book3s/32
that works (tested on 8xx, 83xx and QEMU MAC99).
Christophe
>
> .../admin-guide/kernel-parameters.txt | 2 +-
> arch/powerpc/include/asm/futex.h | 4 ++
> arch/powerpc/include/asm/kup.h | 24 ++++++++++++
> arch/powerpc/include/asm/ptrace.h | 11 +++++-
> arch/powerpc/include/asm/uaccess.h | 38 +++++++++++++++----
> arch/powerpc/kernel/asm-offsets.c | 4 ++
> arch/powerpc/lib/checksum_wrappers.c | 4 ++
> arch/powerpc/mm/fault.c | 19 ++++++++--
> arch/powerpc/mm/init-common.c | 10 +++++
> arch/powerpc/platforms/Kconfig.cputype | 12 ++++++
> 10 files changed, 113 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f81d79de4de0..16883f2a05fd 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2809,7 +2809,7 @@
> noexec=on: enable non-executable mappings (default)
> noexec=off: disable non-executable mappings
>
> - nosmap [X86]
> + nosmap [X86,PPC]
> Disable SMAP (Supervisor Mode Access Prevention)
> even if it is supported by processor.
>
> diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
> index 88b38b37c21b..3a6aa57b9d90 100644
> --- a/arch/powerpc/include/asm/futex.h
> +++ b/arch/powerpc/include/asm/futex.h
> @@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> {
> int oldval = 0, ret;
>
> + allow_write_to_user(uaddr, sizeof(*uaddr));
> pagefault_disable();
>
> switch (op) {
> @@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> if (!ret)
> *oval = oldval;
>
> + prevent_write_to_user(uaddr, sizeof(*uaddr));
> return ret;
> }
>
> @@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> if (!access_ok(uaddr, sizeof(u32)))
> return -EFAULT;
>
> + allow_write_to_user(uaddr, sizeof(*uaddr));
> __asm__ __volatile__ (
> PPC_ATOMIC_ENTRY_BARRIER
> "1: lwarx %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\
> @@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> : "cc", "memory");
>
> *uval = prev;
> + prevent_write_to_user(uaddr, sizeof(*uaddr));
> return ret;
> }
>
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index a2a959cb4e36..4410625f4364 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -4,6 +4,8 @@
>
> #ifndef __ASSEMBLY__
>
> +#include <asm/pgtable.h>
> +
> void setup_kup(void);
>
> #ifdef CONFIG_PPC_KUEP
> @@ -12,6 +14,28 @@ void setup_kuep(bool disabled);
> static inline void setup_kuep(bool disabled) { }
> #endif /* CONFIG_PPC_KUEP */
>
> +#ifdef CONFIG_PPC_KUAP
> +void setup_kuap(bool disabled);
> +#else
> +static inline void setup_kuap(bool disabled) { }
> +static inline void allow_user_access(void __user *to, const void __user *from,
> + unsigned long size) { }
> +static inline void prevent_user_access(void __user *to, const void __user *from,
> + unsigned long size) { }
> +static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
> +static inline void allow_write_to_user(void __user *to, unsigned long size) {}
> +#endif /* CONFIG_PPC_KUAP */
> +
> +static inline void prevent_read_from_user(const void __user *from, unsigned long size)
> +{
> + prevent_user_access(NULL, from, size);
> +}
> +
> +static inline void prevent_write_to_user(void __user *to, unsigned long size)
> +{
> + prevent_user_access(to, NULL, size);
> +}
> +
> #endif /* !__ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_KUP_H_ */
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 64271e562fed..6f047730e642 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -52,10 +52,17 @@ struct pt_regs
> };
> };
>
> + union {
> + struct {
> #ifdef CONFIG_PPC64
> - unsigned long ppr;
> - unsigned long __pad; /* Maintain 16 byte interrupt stack alignment */
> + unsigned long ppr;
> +#endif
> +#ifdef CONFIG_PPC_KUAP
> + unsigned long kuap;
> #endif
> + };
> + unsigned long __pad[2]; /* Maintain 16 byte interrupt stack alignment */
> + };
> };
> #endif
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index e3a731793ea2..fb7651a5488b 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -6,6 +6,7 @@
> #include <asm/processor.h>
> #include <asm/page.h>
> #include <asm/extable.h>
> +#include <asm/kup.h>
>
> /*
> * The fs value determines whether argument validity checking should be
> @@ -141,6 +142,7 @@ extern long __put_user_bad(void);
> #define __put_user_size(x, ptr, size, retval) \
> do { \
> retval = 0; \
> + allow_write_to_user(ptr, size); \
> switch (size) { \
> case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
> case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
> @@ -148,6 +150,7 @@ do { \
> case 8: __put_user_asm2(x, ptr, retval); break; \
> default: __put_user_bad(); \
> } \
> + prevent_write_to_user(ptr, size); \
> } while (0)
>
> #define __put_user_nocheck(x, ptr, size) \
> @@ -240,6 +243,7 @@ do { \
> __chk_user_ptr(ptr); \
> if (size > sizeof(x)) \
> (x) = __get_user_bad(); \
> + allow_read_from_user(ptr, size); \
> switch (size) { \
> case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
> case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
> @@ -247,6 +251,7 @@ do { \
> case 8: __get_user_asm2(x, ptr, retval); break; \
> default: (x) = __get_user_bad(); \
> } \
> + prevent_read_from_user(ptr, size); \
> } while (0)
>
> /*
> @@ -306,15 +311,21 @@ extern unsigned long __copy_tofrom_user(void __user *to,
> static inline unsigned long
> raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> {
> - return __copy_tofrom_user(to, from, n);
> + unsigned long ret;
> +
> + allow_user_access(to, from, n);
> + ret = __copy_tofrom_user(to, from, n);
> + prevent_user_access(to, from, n);
> + return ret;
> }
> #endif /* __powerpc64__ */
>
> static inline unsigned long raw_copy_from_user(void *to,
> const void __user *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -339,14 +350,18 @@ static inline unsigned long raw_copy_from_user(void *to,
> }
>
> barrier_nospec();
> - return __copy_tofrom_user((__force void __user *)to, from, n);
> + allow_read_from_user(from, n);
> + ret = __copy_tofrom_user((__force void __user *)to, from, n);
> + prevent_read_from_user(from, n);
> + return ret;
> }
>
> static inline unsigned long raw_copy_to_user(void __user *to,
> const void *from, unsigned long n)
> {
> + unsigned long ret;
> if (__builtin_constant_p(n) && (n <= 8)) {
> - unsigned long ret = 1;
> + ret = 1;
>
> switch (n) {
> case 1:
> @@ -366,17 +381,24 @@ static inline unsigned long raw_copy_to_user(void __user *to,
> return 0;
> }
>
> - return __copy_tofrom_user(to, (__force const void __user *)from, n);
> + allow_write_to_user(to, n);
> + ret = __copy_tofrom_user(to, (__force const void __user *)from, n);
> + prevent_write_to_user(to, n);
> + return ret;
> }
>
> extern unsigned long __clear_user(void __user *addr, unsigned long size);
>
> static inline unsigned long clear_user(void __user *addr, unsigned long size)
> {
> + unsigned long ret = size;
> might_fault();
> - if (likely(access_ok(addr, size)))
> - return __clear_user(addr, size);
> - return size;
> + if (likely(access_ok(addr, size))) {
> + allow_write_to_user(addr, size);
> + ret = __clear_user(addr, size);
> + prevent_write_to_user(addr, size);
> + }
> + return ret;
> }
>
> extern long strncpy_from_user(char *dst, const char __user *src, long count);
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index 86a61e5f8285..66202e02fee2 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -332,6 +332,10 @@ int main(void)
> STACK_PT_REGS_OFFSET(_PPR, ppr);
> #endif /* CONFIG_PPC64 */
>
> +#ifdef CONFIG_PPC_KUAP
> + STACK_PT_REGS_OFFSET(STACK_REGS_KUAP, kuap);
> +#endif
> +
> #if defined(CONFIG_PPC32)
> #if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> DEFINE(EXC_LVL_SIZE, STACK_EXC_LVL_FRAME_SIZE);
> diff --git a/arch/powerpc/lib/checksum_wrappers.c b/arch/powerpc/lib/checksum_wrappers.c
> index 890d4ddd91d6..bb9307ce2440 100644
> --- a/arch/powerpc/lib/checksum_wrappers.c
> +++ b/arch/powerpc/lib/checksum_wrappers.c
> @@ -29,6 +29,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
> unsigned int csum;
>
> might_sleep();
> + allow_read_from_user(src, len);
>
> *err_ptr = 0;
>
> @@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
> }
>
> out:
> + prevent_read_from_user(src, len);
> return (__force __wsum)csum;
> }
> EXPORT_SYMBOL(csum_and_copy_from_user);
> @@ -70,6 +72,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
> unsigned int csum;
>
> might_sleep();
> + allow_write_to_user(dst, len);
>
> *err_ptr = 0;
>
> @@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
> }
>
> out:
> + prevent_write_to_user(dst, len);
> return (__force __wsum)csum;
> }
> EXPORT_SYMBOL(csum_and_copy_to_user);
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 3384354abc1d..463d1e9d026e 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -223,9 +223,11 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
> }
>
> /* Is this a bad kernel fault ? */
> -static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
> +static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> unsigned long address)
> {
> + int is_exec = TRAP(regs) == 0x400;
> +
> /* NX faults set DSISR_PROTFAULT on the 8xx, DSISR_NOEXEC_OR_G on others */
> if (is_exec && (error_code & (DSISR_NOEXEC_OR_G | DSISR_KEYFAULT |
> DSISR_PROTFAULT))) {
> @@ -234,7 +236,15 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code,
> address,
> from_kuid(&init_user_ns, current_uid()));
> }
> - return is_exec || (address >= TASK_SIZE);
> +
> + if (!is_exec && address < TASK_SIZE && (error_code & DSISR_PROTFAULT) &&
> + !search_exception_tables(regs->nip)) {
> + pr_crit_ratelimited("Kernel attempted to access user page (%lx) - exploit attempt? (uid: %d)\n",
> + address,
> + from_kuid(&init_user_ns, current_uid()));
> + }
> +
> + return is_exec || (address >= TASK_SIZE) || !search_exception_tables(regs->nip);
> }
>
> static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> @@ -454,9 +464,10 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
>
> /*
> * The kernel should never take an execute fault nor should it
> - * take a page fault to a kernel address.
> + * take a page fault to a kernel address or a page fault to a user
> + * address outside of dedicated places
> */
> - if (unlikely(!is_user && bad_kernel_fault(is_exec, error_code, address)))
> + if (unlikely(!is_user && bad_kernel_fault(regs, error_code, address)))
> return SIGSEGV;
>
> /*
> diff --git a/arch/powerpc/mm/init-common.c b/arch/powerpc/mm/init-common.c
> index 83f95a5565d6..ecaedfff9992 100644
> --- a/arch/powerpc/mm/init-common.c
> +++ b/arch/powerpc/mm/init-common.c
> @@ -27,6 +27,7 @@
> #include <asm/kup.h>
>
> static bool disable_kuep = !IS_ENABLED(CONFIG_PPC_KUEP);
> +static bool disable_kuap = !IS_ENABLED(CONFIG_PPC_KUAP);
>
> static int __init parse_nosmep(char *p)
> {
> @@ -36,9 +37,18 @@ static int __init parse_nosmep(char *p)
> }
> early_param("nosmep", parse_nosmep);
>
> +static int __init parse_nosmap(char *p)
> +{
> + disable_kuap = true;
> + pr_warn("Disabling Kernel Userspace Access Protection\n");
> + return 0;
> +}
> +early_param("nosmap", parse_nosmap);
> +
> void __init setup_kup(void)
> {
> setup_kuep(disable_kuep);
> + setup_kuap(disable_kuap);
> }
>
> #define CTOR(shift) static void ctor_##shift(void *addr) \
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 7d30bbbaa3c1..457fc3a5ed93 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -357,6 +357,18 @@ config PPC_KUEP
>
> If you're unsure, say Y.
>
> +config PPC_HAVE_KUAP
> + bool
> +
> +config PPC_KUAP
> + bool "Kernel Userspace Access Protection"
> + depends on PPC_HAVE_KUAP
> + default y
> + help
> + Enable support for Kernel Userspace Access Protection (KUAP)
> +
> + If you're unsure, say Y.
> +
> config ARCH_ENABLE_HUGEPAGE_MIGRATION
> def_bool y
> depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
2019-03-08 1:16 ` [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR " Michael Ellerman
@ 2019-03-13 8:16 ` Akshay Adiga
2019-03-20 12:58 ` Michael Ellerman
0 siblings, 1 reply; 25+ messages in thread
From: Akshay Adiga @ 2019-03-13 8:16 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, npiggin
On Fri, Mar 08, 2019 at 12:16:11PM +1100, Michael Ellerman wrote:
> In order to implement KUAP (Kernel Userspace Access Protection) on
> Power9 we will be using the AMR, and therefore indirectly the
> UAMOR/AMOR.
>
> So save/restore these regs in the idle code.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> v5: Unchanged.
> v4: New.
>
> arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
Opps.. i posted a comment on the v4.
It would be good if we can make AMOR/UAMOR/AMR save-restore
code power9 only.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-08 8:26 ` Christophe Leroy
@ 2019-03-20 12:57 ` Michael Ellerman
2019-03-20 13:04 ` Christophe Leroy
0 siblings, 1 reply; 25+ messages in thread
From: Michael Ellerman @ 2019-03-20 12:57 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>
>> This patch implements a framework for Kernel Userspace Access
>> Protection.
>>
>> Then subarches will have the possibility to provide their own
>> implementation by providing setup_kuap() and
>> allow/prevent_user_access().
>>
>> Some platforms will need to know the area accessed and whether it is
>> accessed from read, write or both. Therefore source, destination and
>> size and handed over to the two functions.
>>
>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>> read/write wrappers. Drop the 32-bit code for now until we have an
>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> v5: Futex ops need read/write so use allow_user_acccess() there.
>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>> Allow subarch to override allow_read/write_from/to_user().
>
> Those little helpers that will just call allow_user_access() when
> distinct read/write handling is not performed looks overkill to me.
>
> Can't the subarch do it by itself based on the nullity of from/to ?
>
> static inline void allow_user_access(void __user *to, const void __user
> *from,
> unsigned long size)
> {
> if (to & from)
> set_kuap(0);
> else if (to)
> set_kuap(AMR_KUAP_BLOCK_READ);
> else if (from)
> set_kuap(AMR_KUAP_BLOCK_WRITE);
> }
You could implement it that way, but it reads better at the call sites
if we have:
allow_write_to_user(uaddr, sizeof(*uaddr));
vs:
allow_user_access(uaddr, NULL, sizeof(*uaddr));
So I'm inclined to keep them. It should all end up inlined and generate
the same code at the end of the day.
cheers
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR after idle
2019-03-13 8:16 ` Akshay Adiga
@ 2019-03-20 12:58 ` Michael Ellerman
0 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-20 12:58 UTC (permalink / raw)
To: Akshay Adiga; +Cc: linuxppc-dev, npiggin
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> writes:
> On Fri, Mar 08, 2019 at 12:16:11PM +1100, Michael Ellerman wrote:
>> In order to implement KUAP (Kernel Userspace Access Protection) on
>> Power9 we will be using the AMR, and therefore indirectly the
>> UAMOR/AMOR.
>>
>> So save/restore these regs in the idle code.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> v5: Unchanged.
>> v4: New.
>>
>> arch/powerpc/kernel/idle_book3s.S | 27 +++++++++++++++++++++++----
>> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> Opps.. i posted a comment on the v4.
>
> It would be good if we can make AMOR/UAMOR/AMR save-restore
> code power9 only.
Yes that would be a good optimisation.
If you can send an incremental patch against this one I'll squash it in.
If not I'll try and get it done at some point before merging.
cheers
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-20 12:57 ` Michael Ellerman
@ 2019-03-20 13:04 ` Christophe Leroy
2019-03-21 10:21 ` Christophe Leroy
2019-03-22 0:35 ` Michael Ellerman
0 siblings, 2 replies; 25+ messages in thread
From: Christophe Leroy @ 2019-03-20 13:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>> This patch implements a framework for Kernel Userspace Access
>>> Protection.
>>>
>>> Then subarches will have the possibility to provide their own
>>> implementation by providing setup_kuap() and
>>> allow/prevent_user_access().
>>>
>>> Some platforms will need to know the area accessed and whether it is
>>> accessed from read, write or both. Therefore source, destination and
>>> size and handed over to the two functions.
>>>
>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>> Allow subarch to override allow_read/write_from/to_user().
>>
>> Those little helpers that will just call allow_user_access() when
>> distinct read/write handling is not performed looks overkill to me.
>>
>> Can't the subarch do it by itself based on the nullity of from/to ?
>>
>> static inline void allow_user_access(void __user *to, const void __user
>> *from,
>> unsigned long size)
>> {
>> if (to & from)
>> set_kuap(0);
>> else if (to)
>> set_kuap(AMR_KUAP_BLOCK_READ);
>> else if (from)
>> set_kuap(AMR_KUAP_BLOCK_WRITE);
>> }
>
> You could implement it that way, but it reads better at the call sites
> if we have:
>
> allow_write_to_user(uaddr, sizeof(*uaddr));
> vs:
> allow_user_access(uaddr, NULL, sizeof(*uaddr));
>
> So I'm inclined to keep them. It should all end up inlined and generate
> the same code at the end of the day.
>
I was not suggesting to completly remove allow_write_to_user(), I fully
agree that it reads better at the call sites.
I was just thinking that allow_write_to_user() could remain generic and
call the subarch specific allow_user_access() instead of making multiple
subarch's allow_write_to_user()
But both solution are OK for me at the end.
Christophe
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-20 13:04 ` Christophe Leroy
@ 2019-03-21 10:21 ` Christophe Leroy
2019-03-22 0:35 ` Michael Ellerman
1 sibling, 0 replies; 25+ messages in thread
From: Christophe Leroy @ 2019-03-21 10:21 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: npiggin
Le 20/03/2019 à 14:04, Christophe Leroy a écrit :
>
>
> Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>
>>>> This patch implements a framework for Kernel Userspace Access
>>>> Protection.
>>>>
>>>> Then subarches will have the possibility to provide their own
>>>> implementation by providing setup_kuap() and
>>>> allow/prevent_user_access().
>>>>
>>>> Some platforms will need to know the area accessed and whether it is
>>>> accessed from read, write or both. Therefore source, destination and
>>>> size and handed over to the two functions.
>>>>
>>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>> ---
>>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>>> Allow subarch to override allow_read/write_from/to_user().
>>>
>>> Those little helpers that will just call allow_user_access() when
>>> distinct read/write handling is not performed looks overkill to me.
>>>
>>> Can't the subarch do it by itself based on the nullity of from/to ?
>>>
>>> static inline void allow_user_access(void __user *to, const void __user
>>> *from,
>>> unsigned long size)
>>> {
>>> if (to & from)
>>> set_kuap(0);
>>> else if (to)
>>> set_kuap(AMR_KUAP_BLOCK_READ);
>>> else if (from)
>>> set_kuap(AMR_KUAP_BLOCK_WRITE);
>>> }
>>
>> You could implement it that way, but it reads better at the call sites
>> if we have:
>>
>> allow_write_to_user(uaddr, sizeof(*uaddr));
>> vs:
>> allow_user_access(uaddr, NULL, sizeof(*uaddr));
>>
>> So I'm inclined to keep them. It should all end up inlined and generate
>> the same code at the end of the day.
>>
>
> I was not suggesting to completly remove allow_write_to_user(), I fully
> agree that it reads better at the call sites.
>
> I was just thinking that allow_write_to_user() could remain generic and
> call the subarch specific allow_user_access() instead of making multiple
> subarch's allow_write_to_user()
Otherwise, could we do something like the following, so that subarches
may implement it or not ?
#ifndef allow_read_from_user
static inline void allow_read_from_user(const void __user *from,
unsigned long size)
{
allow_user_access(NULL, from, size);
}
#endif
#ifndef allow_write_from_user
static inline void allow_write_to_user(void __user *to, unsigned long size)
{
allow_user_access(to, NULL, size);
}
#endif
Christophe
>
> But both solution are OK for me at the end.
>
> Christophe
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection
2019-03-20 13:04 ` Christophe Leroy
2019-03-21 10:21 ` Christophe Leroy
@ 2019-03-22 0:35 ` Michael Ellerman
1 sibling, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-03-22 0:35 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 20/03/2019 à 13:57, Michael Ellerman a écrit :
>> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>>>> From: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>
>>>> This patch implements a framework for Kernel Userspace Access
>>>> Protection.
>>>>
>>>> Then subarches will have the possibility to provide their own
>>>> implementation by providing setup_kuap() and
>>>> allow/prevent_user_access().
>>>>
>>>> Some platforms will need to know the area accessed and whether it is
>>>> accessed from read, write or both. Therefore source, destination and
>>>> size and handed over to the two functions.
>>>>
>>>> mpe: Rename to allow/prevent rather than unlock/lock, and add
>>>> read/write wrappers. Drop the 32-bit code for now until we have an
>>>> implementation for it. Add kuap to pt_regs for 64-bit as well as
>>>> 32-bit. Don't split strings, use pr_crit_ratelimited().
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>>> ---
>>>> v5: Futex ops need read/write so use allow_user_acccess() there.
>>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors.
>>>> Allow subarch to override allow_read/write_from/to_user().
>>>
>>> Those little helpers that will just call allow_user_access() when
>>> distinct read/write handling is not performed looks overkill to me.
>>>
>>> Can't the subarch do it by itself based on the nullity of from/to ?
>>>
>>> static inline void allow_user_access(void __user *to, const void __user
>>> *from,
>>> unsigned long size)
>>> {
>>> if (to & from)
>>> set_kuap(0);
>>> else if (to)
>>> set_kuap(AMR_KUAP_BLOCK_READ);
>>> else if (from)
>>> set_kuap(AMR_KUAP_BLOCK_WRITE);
>>> }
>>
>> You could implement it that way, but it reads better at the call sites
>> if we have:
>>
>> allow_write_to_user(uaddr, sizeof(*uaddr));
>> vs:
>> allow_user_access(uaddr, NULL, sizeof(*uaddr));
>>
>> So I'm inclined to keep them. It should all end up inlined and generate
>> the same code at the end of the day.
>>
>
> I was not suggesting to completly remove allow_write_to_user(), I fully
> agree that it reads better at the call sites.
>
> I was just thinking that allow_write_to_user() could remain generic and
> call the subarch specific allow_user_access() instead of making multiple
> subarch's allow_write_to_user()
Yep OK I see what you mean.
Your suggestion above should work, and involves the least amount of
ifdefs and so on.
I'll try and get time to post a v6.
cheers
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults
2019-03-08 8:53 ` Christophe Leroy
2019-03-09 12:49 ` christophe leroy
@ 2019-04-17 13:12 ` Michael Ellerman
1 sibling, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-04-17 13:12 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
>> When KUAP is enabled we have logic to detect page faults that occur
>> outside of a valid user access region and are blocked by the AMR.
>>
>> What we don't have at the moment is logic to detect a fault *within* a
>> valid user access region, that has been incorrectly blocked by AMR.
>> This is not meant to ever happen, but it can if we incorrectly
>> save/restore the AMR, or if the AMR was overwritten for some other
>> reason.
>>
>> Currently if that happens we assume it's just a regular fault that
>> will be corrected by handling the fault normally, so we just return.
>> But there is nothing the fault handling code can do to fix it, so the
>> fault just happens again and we spin forever, leading to soft lockups.
>>
>> So add some logic to detect that case and WARN() if we ever see it.
>> Arguably it should be a BUG(), but it's more polite to fail the access
>> and let the kernel continue, rather than taking down the box. There
>> should be no data integrity issue with failing the fault rather than
>> BUG'ing, as we're just going to disallow an access that should have
>> been allowed.
>>
>> To make the code a little easier to follow, unroll the condition at
>> the end of bad_kernel_fault() and comment each case, before adding the
>> call to bad_kuap_fault().
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>>
>> v5: New.
>>
>> .../powerpc/include/asm/book3s/64/kup-radix.h | 12 +++++++++
>> arch/powerpc/include/asm/kup.h | 1 +
>> arch/powerpc/mm/fault.c | 25 ++++++++++++++++---
>> 3 files changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> index 3d60b04fc3f6..8d2ddc61e92e 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
>> set_kuap(AMR_KUAP_BLOCKED);
>> }
>>
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
>> +{
>> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
>> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
>> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ)))) {
>
> Should this { go on the previous line ?
Yeah I guess.
>> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
>> + return true;
>
> Could just be
> return WARN(true, ....)
>
> Or even
> return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
> ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
> (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...);
That's not any more readable IMO.
>> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
>> index f79d4d970852..ccbd2a249575 100644
>> --- a/arch/powerpc/include/asm/kup.h
>> +++ b/arch/powerpc/include/asm/kup.h
>> @@ -28,6 +28,7 @@ static inline void prevent_user_access(void __user *to, const void __user *from,
>> unsigned long size) { }
>> static inline void allow_read_from_user(const void __user *from, unsigned long size) {}
>> static inline void allow_write_to_user(void __user *to, unsigned long size) {}
>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write) { return false; }
>> #endif /* CONFIG_PPC_KUAP */
>>
>> static inline void prevent_read_from_user(const void __user *from, unsigned long size)
>> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
>> index 463d1e9d026e..b5d3578d9f65 100644
>> --- a/arch/powerpc/mm/fault.c
>> +++ b/arch/powerpc/mm/fault.c
>> @@ -224,7 +225,7 @@ static int mm_fault_error(struct pt_regs *regs, unsigned long addr,
>>
>> /* Is this a bad kernel fault ? */
>> static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
>> - unsigned long address)
>> + unsigned long address, bool is_write)
>
> We have regs, do we need is_write in addition ?
It comes from error_code, which we also have. But I don't see any harm
passing it as we already have it calculated and sitting in a GPR.
cheers
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults
2019-03-09 12:49 ` christophe leroy
@ 2019-04-17 13:17 ` Michael Ellerman
0 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-04-17 13:17 UTC (permalink / raw)
To: christophe leroy, linuxppc-dev; +Cc: npiggin
christophe leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 09:53, Christophe Leroy a écrit :
>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
...
>>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>>> b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>>> index 3d60b04fc3f6..8d2ddc61e92e 100644
>>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>>> @@ -100,6 +100,18 @@ static inline void prevent_user_access(void
>>> __user *to, const void __user *from,
>>> set_kuap(AMR_KUAP_BLOCKED);
>>> }
>>> +static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
>>> +{
>>> + if (mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
>>> + ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
>>> + (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))))
>>> + {
>>
>> Should this { go on the previous line ?
>>
>>> + WARN(true, "Bug: %s fault blocked by AMR!", is_write ?
>>> "Write" : "Read");
>>> + return true;
>>
>> Could just be
>> return WARN(true, ....)
>>
>> Or even
>> return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
>> ((is_write && (regs->kuap & AMR_KUAP_BLOCK_WRITE)) ||
>> (!is_write && (regs->kuap & AMR_KUAP_BLOCK_READ))), ...);
>
> Could also be simplified as follows since (is_write && ...) and
> (!is_write && ...) are mutually exclusive:
>
> mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
> (regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ))
Yeah I guess that is better.
I'll do:
static inline bool bad_kuap_fault(struct pt_regs *regs, bool is_write)
{
return WARN(mmu_has_feature(MMU_FTR_RADIX_KUAP) &&
(regs->kuap & (is_write ? AMR_KUAP_BLOCK_WRITE : AMR_KUAP_BLOCK_READ)),
"Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
}
cheers
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU
2019-03-08 8:48 ` Christophe Leroy
@ 2019-04-17 13:39 ` Michael Ellerman
0 siblings, 0 replies; 25+ messages in thread
From: Michael Ellerman @ 2019-04-17 13:39 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: npiggin
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Le 08/03/2019 à 02:16, Michael Ellerman a écrit :
...
>> 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.
>> +#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.
>> +#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.
>> +.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.
>> + 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 :)
>> 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?
>> beq 4f; /* if from kernel mode */ \
>> ACCOUNT_CPU_USER_ENTRY(r13, r9, r10); \
>> SAVE_PPR(area, r9); \
>> 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.
>> 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
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-04-17 13:54 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-08 1:16 [PATCH v5 01/10] powerpc/powernv/idle: Restore IAMR after idle Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 02/10] powerpc/powernv/idle: Restore AMR/UAMOR/AMOR " Michael Ellerman
2019-03-13 8:16 ` Akshay Adiga
2019-03-20 12:58 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 03/10] powerpc: Add framework for Kernel Userspace Protection Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 04/10] powerpc: Add skeleton for Kernel Userspace Execution Prevention Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 05/10] powerpc: Add a framework for Kernel Userspace Access Protection Michael Ellerman
2019-03-08 8:26 ` Christophe Leroy
2019-03-20 12:57 ` Michael Ellerman
2019-03-20 13:04 ` Christophe Leroy
2019-03-21 10:21 ` Christophe Leroy
2019-03-22 0:35 ` Michael Ellerman
2019-03-11 9:12 ` Christophe Leroy
2019-03-08 1:16 ` [PATCH v5 06/10] powerpc/64: Setup KUP on secondary CPUs Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 07/10] powerpc/mm/radix: Use KUEP API for Radix MMU Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 08/10] powerpc/lib: Refactor __patch_instruction() to use __put_user_asm() Michael Ellerman
2019-03-08 8:32 ` Christophe Leroy
2019-03-08 1:16 ` [PATCH v5 09/10] powerpc/64s: Implement KUAP for Radix MMU Michael Ellerman
2019-03-08 8:48 ` Christophe Leroy
2019-04-17 13:39 ` Michael Ellerman
2019-03-08 1:16 ` [PATCH v5 10/10] powerpc/mm: Detect bad KUAP faults Michael Ellerman
2019-03-08 8:53 ` Christophe Leroy
2019-03-09 12:49 ` christophe leroy
2019-04-17 13:17 ` Michael Ellerman
2019-04-17 13:12 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.