* [PATCH 1/6] powerpc/64/kuap: move kuap checks out of MSR[RI]=0 regions of exit code
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-04-29 6:56 ` [PATCH 2/6] powerpc/64s/kuap: kuap_restore missing isync Nicholas Piggin
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Any kind of WARN causes a program check that will crash with
unrecoverable exception if it occurs when RI is clear.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/syscall_64.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index 8f7e268f3294..a37c7717424f 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -35,6 +35,8 @@ notrace long system_call_exception(long r3, long r4, long r5,
BUG_ON(!FULL_REGS(regs));
BUG_ON(regs->softe != IRQS_ENABLED);
+ kuap_check_amr();
+
account_cpu_user_entry();
#ifdef CONFIG_PPC_SPLPAR
@@ -47,8 +49,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
}
#endif
- kuap_check_amr();
-
/*
* This is not required for the syscall exit path, but makes the
* stack frame look nicer. If this was initialised in the first stack
@@ -142,6 +142,8 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
unsigned long ti_flags;
unsigned long ret = 0;
+ kuap_check_amr();
+
regs->result = r3;
/* Check whether the syscall is issued inside a restartable sequence */
@@ -218,8 +220,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
local_paca->tm_scratch = regs->msr;
#endif
- kuap_check_amr();
-
account_cpu_user_exit();
return ret;
@@ -242,6 +242,8 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
BUG_ON(!FULL_REGS(regs));
BUG_ON(regs->softe != IRQS_ENABLED);
+ kuap_check_amr();
+
local_irq_save(flags);
again:
@@ -298,8 +300,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
local_paca->tm_scratch = regs->msr;
#endif
- kuap_check_amr();
-
account_cpu_user_exit();
return ret;
@@ -319,6 +319,8 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
BUG_ON(regs->msr & MSR_PR);
BUG_ON(!FULL_REGS(regs));
+ kuap_check_amr();
+
if (unlikely(*ti_flagsp & _TIF_EMULATE_STACK_STORE)) {
clear_bits(_TIF_EMULATE_STACK_STORE, ti_flagsp);
ret = 1;
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/6] powerpc/64s/kuap: kuap_restore missing isync
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
2020-04-29 6:56 ` [PATCH 1/6] powerpc/64/kuap: move kuap checks out of MSR[RI]=0 regions of exit code Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-04-29 6:56 ` [PATCH 3/6] powerpc/64/kuap: interrupt exit conditionally restore AMR Nicholas Piggin
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Writing the AMR register is documented to require context synchronizing
operations before and after, for it to take effect as expected. The kuap
restore at interrupt exit time deliberately avoids the isync after the
AMR update because it only needs to take effect after the context
synchronizing rfid that soon follows. Add a comment for this.
The missing isync before the update doesn't have an obvious
justification, and seems it could theorietically allow a rogue user
access to leak past the AMR update. Add isyncs for these.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/kup-radix.h | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 3bcef989a35d..8dc5f292b806 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -16,7 +16,9 @@
#ifdef CONFIG_PPC_KUAP
BEGIN_MMU_FTR_SECTION_NESTED(67)
ld \gpr, STACK_REGS_KUAP(r1)
+ isync
mtspr SPRN_AMR, \gpr
+ /* No isync required, see kuap_restore_amr() */
END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
#endif
.endm
@@ -62,8 +64,15 @@
static inline void kuap_restore_amr(struct pt_regs *regs)
{
- if (mmu_has_feature(MMU_FTR_RADIX_KUAP))
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ isync();
mtspr(SPRN_AMR, regs->kuap);
+ /*
+ * No isync required here because we are about to rfi
+ * back to previous context before any user accesses
+ * would be made, which is a CSI.
+ */
+ }
}
static inline void kuap_check_amr(void)
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/6] powerpc/64/kuap: interrupt exit conditionally restore AMR
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
2020-04-29 6:56 ` [PATCH 1/6] powerpc/64/kuap: move kuap checks out of MSR[RI]=0 regions of exit code Nicholas Piggin
2020-04-29 6:56 ` [PATCH 2/6] powerpc/64s/kuap: kuap_restore missing isync Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-04-29 6:56 ` [PATCH 4/6] powerpc/64s/kuap: restore AMR in system reset exception Nicholas Piggin
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
The AMR update is made conditional on AMR actually changing, which
should be the less common case on most workloads (though kernel page
faults on uaccess could be frequent, this doesn't significantly slow
down that case).
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
.../powerpc/include/asm/book3s/64/kup-radix.h | 36 ++++++++++++++-----
arch/powerpc/kernel/syscall_64.c | 14 +++++---
2 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index 8dc5f292b806..ec8970958a26 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -62,19 +62,32 @@
#include <asm/mmu.h>
#include <asm/ptrace.h>
-static inline void kuap_restore_amr(struct pt_regs *regs)
+static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
{
if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
- isync();
- mtspr(SPRN_AMR, regs->kuap);
- /*
- * No isync required here because we are about to rfi
- * back to previous context before any user accesses
- * would be made, which is a CSI.
- */
+ if (unlikely(regs->kuap != amr)) {
+ isync();
+ mtspr(SPRN_AMR, regs->kuap);
+ /*
+ * No isync required here because we are about to rfi
+ * back to previous context before any user accesses
+ * would be made, which is a CSI.
+ */
+ }
}
}
+static inline unsigned long kuap_get_and_check_amr(void)
+{
+ if (mmu_has_feature(MMU_FTR_RADIX_KUAP)) {
+ unsigned long amr = mfspr(SPRN_AMR);
+ if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG)) /* kuap_check_amr() */
+ WARN_ON_ONCE(amr != AMR_KUAP_BLOCKED);
+ return amr;
+ }
+ return 0;
+}
+
static inline void kuap_check_amr(void)
{
if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_RADIX_KUAP))
@@ -151,13 +164,18 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
"Bug: %s fault blocked by AMR!", is_write ? "Write" : "Read");
}
#else /* CONFIG_PPC_KUAP */
-static inline void kuap_restore_amr(struct pt_regs *regs)
+static inline void kuap_restore_amr(struct pt_regs *regs, unsigned long amr)
{
}
static inline void kuap_check_amr(void)
{
}
+
+static inline unsigned long kuap_get_and_check_amr(void)
+{
+ return 0;
+}
#endif /* CONFIG_PPC_KUAP */
#endif /* __ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
index a37c7717424f..edeab10c6888 100644
--- a/arch/powerpc/kernel/syscall_64.c
+++ b/arch/powerpc/kernel/syscall_64.c
@@ -242,6 +242,10 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
BUG_ON(!FULL_REGS(regs));
BUG_ON(regs->softe != IRQS_ENABLED);
+ /*
+ * We don't need to restore AMR on the way back to userspace for KUAP.
+ * AMR can only have been unlocked if we interrupted the kernel.
+ */
kuap_check_amr();
local_irq_save(flags);
@@ -313,13 +317,14 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
unsigned long *ti_flagsp = ¤t_thread_info()->flags;
unsigned long flags;
unsigned long ret = 0;
+ unsigned long amr;
if (IS_ENABLED(CONFIG_PPC_BOOK3S) && unlikely(!(regs->msr & MSR_RI)))
unrecoverable_exception(regs);
BUG_ON(regs->msr & MSR_PR);
BUG_ON(!FULL_REGS(regs));
- kuap_check_amr();
+ amr = kuap_get_and_check_amr();
if (unlikely(*ti_flagsp & _TIF_EMULATE_STACK_STORE)) {
clear_bits(_TIF_EMULATE_STACK_STORE, ti_flagsp);
@@ -367,10 +372,11 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
#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.
+ * Don't want to mfspr(SPRN_AMR) here, because this comes after
+ * mtmsr, which would cause RAW stalls. Hence, we take the AMR value
+ * from the check above.
*/
- kuap_restore_amr(regs);
+ kuap_restore_amr(regs, amr);
return ret;
}
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 4/6] powerpc/64s/kuap: restore AMR in system reset exception
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
` (2 preceding siblings ...)
2020-04-29 6:56 ` [PATCH 3/6] powerpc/64/kuap: interrupt exit conditionally restore AMR Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-04-29 6:56 ` [PATCH 5/6] powerpc/64s/kuap: restore AMR in fast_interrupt_return Nicholas Piggin
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
system reset interrupt handler locks AMR and exits with
PTION_RESTORE_REGS without restoring AMR. Similarly to the soft-NMI
ler, it needs to restore.
ed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/exceptions-64s.S | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index 728ccb0f560c..b0ad930cbae5 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -971,6 +971,7 @@ EXC_COMMON_BEGIN(system_reset_common)
ld r10,SOFTE(r1)
stb r10,PACAIRQSOFTMASK(r13)
+ kuap_restore_amr r10
EXCEPTION_RESTORE_REGS
RFI_TO_USER_OR_KERNEL
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 5/6] powerpc/64s/kuap: restore AMR in fast_interrupt_return
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
` (3 preceding siblings ...)
2020-04-29 6:56 ` [PATCH 4/6] powerpc/64s/kuap: restore AMR in system reset exception Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-04-29 6:56 ` [PATCH 6/6] powerpc/64s/kuap: conditionally restore AMR in kuap_restore_amr asm Nicholas Piggin
2020-06-09 5:54 ` [PATCH 0/6] assorted kuap fixes (try again) Michael Ellerman
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Interrupts that use fast_interrupt_return actually do lock AMR, but they
have been ones which tend to come from userspace (or kernel bugs) in
radix mode. With kuap on hash, segment interrupts are taken in kernel
often, which quickly breaks due to the missing restore.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/entry_64.S | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 9a1e5d636dea..b3c9f15089b6 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -472,15 +472,17 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
#ifdef CONFIG_PPC_BOOK3S
/*
* If MSR EE/RI was never enabled, IRQs not reconciled, NVGPRs not
- * touched, AMR not set, no exit work created, then this can be used.
+ * touched, no exit work created, then this can be used.
*/
.balign IFETCH_ALIGN_BYTES
.globl fast_interrupt_return
fast_interrupt_return:
_ASM_NOKPROBE_SYMBOL(fast_interrupt_return)
+ kuap_check_amr r3, r4
ld r4,_MSR(r1)
andi. r0,r4,MSR_PR
bne .Lfast_user_interrupt_return
+ kuap_restore_amr r3
andi. r0,r4,MSR_RI
li r3,0 /* 0 return value, no EMULATE_STACK_STORE */
bne+ .Lfast_kernel_interrupt_return
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 6/6] powerpc/64s/kuap: conditionally restore AMR in kuap_restore_amr asm
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
` (4 preceding siblings ...)
2020-04-29 6:56 ` [PATCH 5/6] powerpc/64s/kuap: restore AMR in fast_interrupt_return Nicholas Piggin
@ 2020-04-29 6:56 ` Nicholas Piggin
2020-06-09 5:54 ` [PATCH 0/6] assorted kuap fixes (try again) Michael Ellerman
6 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2020-04-29 6:56 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Similar to the C code change, make the AMR restore conditional on
whether the register has changed.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/book3s/64/kup-radix.h | 10 +++++++---
arch/powerpc/kernel/entry_64.S | 8 ++++----
arch/powerpc/kernel/exceptions-64s.S | 4 ++--
3 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index ec8970958a26..e82df54f5681 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -12,13 +12,17 @@
#ifdef __ASSEMBLY__
-.macro kuap_restore_amr gpr
+.macro kuap_restore_amr gpr1, gpr2
#ifdef CONFIG_PPC_KUAP
BEGIN_MMU_FTR_SECTION_NESTED(67)
- ld \gpr, STACK_REGS_KUAP(r1)
+ mfspr \gpr1, SPRN_AMR
+ ld \gpr2, STACK_REGS_KUAP(r1)
+ cmpd \gpr1, \gpr2
+ beq 998f
isync
- mtspr SPRN_AMR, \gpr
+ mtspr SPRN_AMR, \gpr2
/* No isync required, see kuap_restore_amr() */
+998:
END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_RADIX_KUAP, 67)
#endif
.endm
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index b3c9f15089b6..9d49338e0c85 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -479,11 +479,11 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
fast_interrupt_return:
_ASM_NOKPROBE_SYMBOL(fast_interrupt_return)
kuap_check_amr r3, r4
- ld r4,_MSR(r1)
- andi. r0,r4,MSR_PR
+ ld r5,_MSR(r1)
+ andi. r0,r5,MSR_PR
bne .Lfast_user_interrupt_return
- kuap_restore_amr r3
- andi. r0,r4,MSR_RI
+ kuap_restore_amr r3, r4
+ andi. r0,r5,MSR_RI
li r3,0 /* 0 return value, no EMULATE_STACK_STORE */
bne+ .Lfast_kernel_interrupt_return
addi r3,r1,STACK_FRAME_OVERHEAD
diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index b0ad930cbae5..ef4a90212664 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -971,7 +971,7 @@ EXC_COMMON_BEGIN(system_reset_common)
ld r10,SOFTE(r1)
stb r10,PACAIRQSOFTMASK(r13)
- kuap_restore_amr r10
+ kuap_restore_amr r9, r10
EXCEPTION_RESTORE_REGS
RFI_TO_USER_OR_KERNEL
@@ -2757,7 +2757,7 @@ EXC_COMMON_BEGIN(soft_nmi_common)
ld r10,SOFTE(r1)
stb r10,PACAIRQSOFTMASK(r13)
- kuap_restore_amr r10
+ kuap_restore_amr r9, r10
EXCEPTION_RESTORE_REGS hsrr=0
RFI_TO_KERNEL
--
2.23.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/6] assorted kuap fixes (try again)
2020-04-29 6:56 [PATCH 0/6] assorted kuap fixes (try again) Nicholas Piggin
` (5 preceding siblings ...)
2020-04-29 6:56 ` [PATCH 6/6] powerpc/64s/kuap: conditionally restore AMR in kuap_restore_amr asm Nicholas Piggin
@ 2020-06-09 5:54 ` Michael Ellerman
6 siblings, 0 replies; 8+ messages in thread
From: Michael Ellerman @ 2020-06-09 5:54 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
On Wed, 29 Apr 2020 16:56:48 +1000, Nicholas Piggin wrote:
> Well the last series was a disaster, I'll try again sending the
> patches with proper subject and changelogs written.
>
> Nicholas Piggin (6):
> powerpc/64/kuap: move kuap checks out of MSR[RI]=0 regions of exit
> code
> powerpc/64s/kuap: kuap_restore missing isync
> powerpc/64/kuap: interrupt exit conditionally restore AMR
> powerpc/64s/kuap: restore AMR in system reset exception
> powerpc/64s/kuap: restore AMR in fast_interrupt_return
> powerpc/64s/kuap: conditionally restore AMR in kuap_restore_amr asm
>
> [...]
Patches 2, 3 and 6 applied to powerpc/next.
[2/6] powerpc/64s/kuap: Add missing isync to KUAP restore paths
https://git.kernel.org/powerpc/c/cb2b53cbffe3c388cd676b63f34e54ceb2643ae2
[3/6] powerpc/64/kuap: Conditionally restore AMR in interrupt exit
https://git.kernel.org/powerpc/c/579940bb451c2dd33396d2d56ce6ef5d92154b3b
[6/6] powerpc/64s/kuap: Conditionally restore AMR in kuap_restore_amr asm
https://git.kernel.org/powerpc/c/d4539074b0e9c5fa6508e8c33aaf51abc8ff6e91
cheers
^ permalink raw reply [flat|nested] 8+ messages in thread