* [PATCH 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction @ 2019-09-03 4:47 Michael Neuling 2019-09-03 4:47 ` [PATCH 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts Michael Neuling ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Michael Neuling @ 2019-09-03 4:47 UTC (permalink / raw) To: mpe; +Cc: linuxppc-dev, cyrilbur, Gustavo Romero From: Gustavo Romero <gromero@linux.ibm.com> When we take an FP unavailable exception in a transaction we have to account for the hardware FP TM checkpointed registers being incorrect. In this case for this process we know the current and checkpointed FP registers must be the same (since FP wasn't used inside the transaction) hence in the thread_struct we copy the current FP registers to the checkpointed ones. This copy is done in tm_reclaim_thread(). We use thread->ckpt_regs.msr to determine if FP was on when in userspace. thread->ckpt_regs.msr represents the state of the MSR when exiting userspace. This is setup by check_if_tm_restore_required(). Unfortunatley there is an optimisation in giveup_all() which returns early if tsk->thread.regs->msr (via local variable `usermsr`) has FP=VEC=VSX=SPE=0. This optimisation means that check_if_tm_restore_required() is not called and hence thread->ckpt_regs.msr is not updated and will contain an old value. This can happen if due to load_fp=255 we start a userspace process with MSR FP=1 and then we are context switched out. In this case thread->ckpt_regs.msr will contain FP=1. If that same process is then context switched in and load_fp overflows, MSR will have FP=0. If that process now enters a transaction and does an FP instruction, the FP unavailable will not update thread->ckpt_regs.msr (the bug) and MSR FP=1 will be retained in thread->ckpt_regs.msr. tm_reclaim_thread() will then not perform the required memcpy and the checkpointed FP regs in the thread struct will contain the wrong values. The code path for this happening is: Userspace: Kernel Start userspace with MSR FP/VEC/VSX/SPE=0 TM=1 < ----- ... tbegin bne fp instruction FP unavailable ---- > fp_unavailable_tm() tm_reclaim_current() tm_reclaim_thread() giveup_all() return early since FP/VMX/VSX=0 /* ckpt MSR not updated (Incorrect) */ tm_reclaim() /* thread_struct ckpt FP regs contain junk (OK) */ /* Sees ckpt MSR FP=1 (Incorrect) */ no memcpy() performed /* thread_struct ckpt FP regs not fixed (Incorrect) */ tm_recheckpoint() /* Put junk in hardware checkpoint FP regs */ .... < ----- Return to userspace with MSR TM=1 FP=1 with junk in the FP TM checkpoint TM rollback reads FP junk This is a data integrity problem for the current process as the FP registers are corrupted. It's also a security problem as the FP registers from one process may be leaked to another. This patch moves up check_if_tm_restore_required() in giveup_all() to ensure thread->ckpt_regs.msr is updated correctly. A simple testcase to replicate this will be posted to tools/testing/selftests/powerpc/tm/tm-poison.c Similarly for VMX. This fixes CVE-2019-15030. Fixes: f48e91e87e67 ("powerpc/tm: Fix FP and VMX register corruption") Cc: stable@vger.kernel.org # 4.12+ Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 8fc4de0d22..437b57068c 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -497,13 +497,14 @@ void giveup_all(struct task_struct *tsk) if (!tsk->thread.regs) return; + check_if_tm_restore_required(tsk); + usermsr = tsk->thread.regs->msr; if ((usermsr & msr_all_available) == 0) return; msr_check_and_set(msr_all_available); - check_if_tm_restore_required(tsk); WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC))); -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts 2019-09-03 4:47 [PATCH 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Neuling @ 2019-09-03 4:47 ` Michael Neuling 2019-09-03 4:47 ` [PATCH 3/3] powerpc/tm: Add tm-poison test Michael Neuling 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero 2 siblings, 0 replies; 11+ messages in thread From: Michael Neuling @ 2019-09-03 4:47 UTC (permalink / raw) To: mpe; +Cc: linuxppc-dev, cyrilbur, Gustavo Romero From: Gustavo Romero <gromero@linux.ibm.com> When in userspace and MSR FP=0 the hardware FP state is unrelated to the current process. This is extended for transactions where if tbegin is run with FP=0, the hardware checkpoint FP state will also be unrelated to the current process. Due to this, we need to ensure this hardware checkpoint is updated with the correct state before we enable FP for this process. Unfortunately we get this wrong when returning to a process from a hardware interrupt. A process that starts a transaction with FP=0 can take an interrupt. When the kernel returns back to that process, we change to FP=1 but with hardware checkpoint FP state not updated. If this transaction is then rolled back, the FP registers now contain the wrong state. The process looks like this: Userspace: Kernel Start userspace with MSR FP=0 TM=1 < ----- ... tbegin bne Hardware interrupt ---- > <do_IRQ...> .... ret_from_except restore_math() /* sees FP=0 */ restore_fp() tm_active_with_fp() /* sees FP=1 (Incorrect) */ load_fp_state() FP = 0 -> 1 < ----- Return to userspace with MSR TM=1 FP=1 with junk in the FP TM checkpoint TM rollback reads FP junk When returning from the hardware exception, tm_active_with_fp() is incorrectly making restore_fp() call load_fp_state() which is setting FP=1. The fix is to remove tm_active_with_fp(). tm_active_with_fp() is attempting to handle the case where FP state has been changed inside a transaction. In this case the checkpointed and transactional FP state is different and hence we must restore the FP state (ie. we can't do lazy FP restore inside a transaction that's used FP). It's safe to remove tm_active_with_fp() as this case is handled by restore_tm_state(). restore_tm_state() detects if FP has been using inside a transaction and will set load_fp and call restore_math() to ensure the FP state (checkpoint and transaction) is restored. This is a data integrity problem for the current process as the FP registers are corrupted. It's also a security problem as the FP registers from one process may be leaked to another. Similarly for VMX. A simple testcase to replicate this will be posted to tools/testing/selftests/powerpc/tm/tm-poison.c This fixes CVE-2019-15031. Fixes: a7771176b439 ("powerpc: Don't enable FP/Altivec if not checkpointed") Cc: stable@vger.kernel.org # 4.15+ Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/process.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 437b57068c..7a84c9f177 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -101,21 +101,8 @@ static void check_if_tm_restore_required(struct task_struct *tsk) } } -static bool tm_active_with_fp(struct task_struct *tsk) -{ - return MSR_TM_ACTIVE(tsk->thread.regs->msr) && - (tsk->thread.ckpt_regs.msr & MSR_FP); -} - -static bool tm_active_with_altivec(struct task_struct *tsk) -{ - return MSR_TM_ACTIVE(tsk->thread.regs->msr) && - (tsk->thread.ckpt_regs.msr & MSR_VEC); -} #else static inline void check_if_tm_restore_required(struct task_struct *tsk) { } -static inline bool tm_active_with_fp(struct task_struct *tsk) { return false; } -static inline bool tm_active_with_altivec(struct task_struct *tsk) { return false; } #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ bool strict_msr_control; @@ -252,7 +239,7 @@ EXPORT_SYMBOL(enable_kernel_fp); static int restore_fp(struct task_struct *tsk) { - if (tsk->thread.load_fp || tm_active_with_fp(tsk)) { + if (tsk->thread.load_fp) { load_fp_state(¤t->thread.fp_state); current->thread.load_fp++; return 1; @@ -334,8 +321,7 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); static int restore_altivec(struct task_struct *tsk) { - if (cpu_has_feature(CPU_FTR_ALTIVEC) && - (tsk->thread.load_vec || tm_active_with_altivec(tsk))) { + if (cpu_has_feature(CPU_FTR_ALTIVEC) && (tsk->thread.load_vec)) { load_vr_state(&tsk->thread.vr_state); tsk->thread.used_vr = 1; tsk->thread.load_vec++; -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] powerpc/tm: Add tm-poison test 2019-09-03 4:47 [PATCH 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Neuling 2019-09-03 4:47 ` [PATCH 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts Michael Neuling @ 2019-09-03 4:47 ` Michael Neuling 2019-09-03 11:46 ` Michael Ellerman 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero 2 siblings, 1 reply; 11+ messages in thread From: Michael Neuling @ 2019-09-03 4:47 UTC (permalink / raw) To: mpe; +Cc: linuxppc-dev, cyrilbur, Gustavo Romero From: Gustavo Romero <gromero@linux.ibm.com> Add TM selftest to check if FP or VEC register values from one process can leak into another process when both run on the same CPU. This tests for CVE-2019-15030 and CVE-2019-15031. Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- tools/testing/selftests/powerpc/tm/.gitignore | 1 + tools/testing/selftests/powerpc/tm/Makefile | 2 +- .../testing/selftests/powerpc/tm/tm-poison.c | 180 ++++++++++++++++++ 3 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/powerpc/tm/tm-poison.c diff --git a/tools/testing/selftests/powerpc/tm/.gitignore b/tools/testing/selftests/powerpc/tm/.gitignore index 951fe855f7..98f2708d86 100644 --- a/tools/testing/selftests/powerpc/tm/.gitignore +++ b/tools/testing/selftests/powerpc/tm/.gitignore @@ -17,3 +17,4 @@ tm-vmx-unavail tm-unavailable tm-trap tm-sigreturn +tm-poison diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile index c0734ed0ef..b15a1a325b 100644 --- a/tools/testing/selftests/powerpc/tm/Makefile +++ b/tools/testing/selftests/powerpc/tm/Makefile @@ -5,7 +5,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-signal-context-chk-fpu TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-stack \ tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap \ $(SIGNAL_CONTEXT_CHK_TESTS) tm-sigreturn tm-signal-sigreturn-nt \ - tm-signal-context-force-tm + tm-signal-context-force-tm tm-poison top_srcdir = ../../../../.. include ../../lib.mk diff --git a/tools/testing/selftests/powerpc/tm/tm-poison.c b/tools/testing/selftests/powerpc/tm/tm-poison.c new file mode 100644 index 0000000000..0113da7a8d --- /dev/null +++ b/tools/testing/selftests/powerpc/tm/tm-poison.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2019, Gustavo Romero, Michael Neuling, IBM Corp. + * + * This test will spawn two processes. Both will be attached to the same + * CPU (CPU 0). The child will be in a loop writing to FP register f31 and + * VMX/VEC/Altivec register vr31 a known value, called poison, calling + * sched_yield syscall after to allow the parent to switch on the CPU. + * Parent will set f31 and vr31 to 1 and in a loop will check if f31 and + * vr31 remain 1 as expected until a given timeout (2m). If the issue is + * present child's poison will leak into parent's f31 or vr31 registers, + * otherwise, poison will never leak into parent's f31 and vr31 registers. + * + * This test for CVE-2019-15030 and CVE-2019-15031. + */ + +#define _GNU_SOURCE +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <inttypes.h> +#include <sched.h> +#include <sys/types.h> +#include <signal.h> + +#include "tm.h" + +int tm_poison_test(void) +{ + int pid; + cpu_set_t cpuset; + uint64_t poison = 0xdeadbeefc0dec0fe; + uint64_t unknown = 0; + bool fail_fp = false; + bool fail_vr = false; + + SKIP_IF(!have_htm()); + + /* Attach both Child and Parent to CPU 0 */ + CPU_ZERO(&cpuset); + CPU_SET(0, &cpuset); + sched_setaffinity(0, sizeof(cpuset), &cpuset); + + pid = fork(); + if (!pid) { + /** + * child + */ + while (1) { + sched_yield(); + asm ( + "mtvsrd 31, %[poison];" // f31 = poison + "mtvsrd 63, %[poison];" // vr31 = poison + + : : [poison] "r" (poison) : ); + } + } + + /** + * parent + */ + asm ( + /* + * Set r3, r4, and f31 to known value 1 before entering + * in transaction. They won't be written after that. + */ + " li 3, 0x1 ;" + " li 4, 0x1 ;" + " mtvsrd 31, 4 ;" + + /* + * The Time Base (TB) is a 64-bit counter register that is + * independent of the CPU clock and which is incremented + * at a frequency of 512000000 Hz, so every 1.953125ns. + * So it's necessary 120s/0.000000001953125s = 61440000000 + * increments to get a 2 minutes timeout. Below we set that + * value in r5 and then use r6 to track initial TB value, + * updating TB values in r7 at every iteration and comparing it + * to r6. When r7 (current) - r6 (initial) > 61440000000 we bail + * out since for sure we spent already 2 minutes in the loop. + * SPR 268 is the TB register. + */ + " lis 5, 14 ;" + " ori 5, 5, 19996 ;" + " sldi 5, 5, 16 ;" // r5 = 61440000000 + + " mfspr 6, 268 ;" // r6 (TB initial) + "1: mfspr 7, 268 ;" // r7 (TB current) + " subf 7, 6, 7 ;" // r7 - r6 > 61440000000 ? + " cmpd 7, 5 ;" + " bgt 3f ;" // yes, exit + + /* + * Main loop to check f31 + */ + " tbegin. ;" // no, try again + " beq 1b ;" // restart if no timeout + " mfvsrd 3, 31 ;" // read f31 + " cmpd 3, 4 ;" // f31 == 1 ? + " bne 2f ;" // broken :-( + " tabort. 3 ;" // try another transaction + "2: tend. ;" // commit transaction + "3: mr %[unknown], 3 ;" // record r3 + + : [unknown] "=r" (unknown) + : + : "cr0", "r3", "r4", "r5", "r6", "r7", "vs31" + + ); + + /* + * On leak 'unknown' will contain 'poison' value from child, + * otherwise (no leak) 'unknown' will contain the same value + * as r3 before entering in transactional mode, i.e. 0x1. + */ + fail_fp = unknown != 0x1; + if (fail_fp) + printf("Unknown value %#lx leaked into f31!\n", unknown); + else + printf("Good, no poison or leaked value into FP registers\n"); + + asm ( + /* + * Set r3, r4, and vr31 to known value 1 before entering + * in transaction. They won't be written after that. + */ + " li 3, 0x1 ;" + " li 4, 0x1 ;" + " mtvsrd 63, 4 ;" + + " lis 5, 14 ;" + " ori 5, 5, 19996 ;" + " sldi 5, 5, 16 ;" // r5 = 61440000000 + + " mfspr 6, 268 ;" // r6 (TB initial) + "1: mfspr 7, 268 ;" // r7 (TB current) + " subf 7, 6, 7 ;" // r7 - r6 > 61440000000 ? + " cmpd 7, 5 ;" + " bgt 3f ;" // yes, exit + + /* + * Main loop to check vr31 + */ + " tbegin. ;" // no, try again + " beq 1b ;" // restart if no timeout + " mfvsrd 3, 63 ;" // read vr31 + " cmpd 3, 4 ;" // vr31 == 1 ? + " bne 2f ;" // broken :-( + " tabort. 3 ;" // try another transaction + "2: tend. ;" // commit transaction + "3: mr %[unknown], 3 ;" // record r3 + + : [unknown] "=r" (unknown) + : + : "cr0", "r3", "r4", "r5", "r6", "r7", "vs63" + + ); + + /* + * On leak 'unknown' will contain 'poison' value from child, + * otherwise (no leak) 'unknown' will contain the same value + * as r3 before entering in transactional mode, i.e. 0x1. + */ + fail_vr = unknown != 0x1; + if (fail_vr) + printf("Unknown value %#lx leaked into vr31!\n", unknown); + else + printf("Good, no poison or leaked value into VEC registers\n"); + + kill(pid, SIGKILL); + + return (fail_fp | fail_vr); +} + +int main(int argc, char *argv[]) +{ + /* Test completes in about 4m */ + test_harness_set_timeout(250); + return test_harness(tm_poison_test, "tm_poison_test"); +} -- 2.21.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] powerpc/tm: Add tm-poison test 2019-09-03 4:47 ` [PATCH 3/3] powerpc/tm: Add tm-poison test Michael Neuling @ 2019-09-03 11:46 ` Michael Ellerman 2019-09-04 4:59 ` Gustavo Romero 0 siblings, 1 reply; 11+ messages in thread From: Michael Ellerman @ 2019-09-03 11:46 UTC (permalink / raw) To: Michael Neuling; +Cc: linuxppc-dev, cyrilbur, Gustavo Romero Michael Neuling <mikey@neuling.org> writes: > From: Gustavo Romero <gromero@linux.ibm.com> > > Add TM selftest to check if FP or VEC register values from one process > can leak into another process when both run on the same CPU. > > This tests for CVE-2019-15030 and CVE-2019-15031. > > Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> > Signed-off-by: Michael Neuling <mikey@neuling.org> > --- > tools/testing/selftests/powerpc/tm/.gitignore | 1 + > tools/testing/selftests/powerpc/tm/Makefile | 2 +- > .../testing/selftests/powerpc/tm/tm-poison.c | 180 ++++++++++++++++++ > 3 files changed, 182 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/powerpc/tm/tm-poison.c This doesn't build on 64-bit big endian: tm-poison.c: In function 'tm_poison_test': tm-poison.c:118:10: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=] printf("Unknown value %#lx leaked into f31!\n", unknown); ^ tm-poison.c:166:10: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=] printf("Unknown value %#lx leaked into vr31!\n", unknown); ^ cheers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] powerpc/tm: Add tm-poison test 2019-09-03 11:46 ` Michael Ellerman @ 2019-09-04 4:59 ` Gustavo Romero 0 siblings, 0 replies; 11+ messages in thread From: Gustavo Romero @ 2019-09-04 4:59 UTC (permalink / raw) To: Michael Ellerman; +Cc: linuxppc-dev, Gustavo Romero Hi Michael, On 09/03/2019 08:46 AM, Michael Ellerman wrote: > Michael Neuling <mikey@neuling.org> writes: >> From: Gustavo Romero <gromero@linux.ibm.com> >> >> Add TM selftest to check if FP or VEC register values from one process >> can leak into another process when both run on the same CPU. >> >> This tests for CVE-2019-15030 and CVE-2019-15031. >> >> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> >> Signed-off-by: Michael Neuling <mikey@neuling.org> >> --- >> tools/testing/selftests/powerpc/tm/.gitignore | 1 + >> tools/testing/selftests/powerpc/tm/Makefile | 2 +- >> .../testing/selftests/powerpc/tm/tm-poison.c | 180 ++++++++++++++++++ >> 3 files changed, 182 insertions(+), 1 deletion(-) >> create mode 100644 tools/testing/selftests/powerpc/tm/tm-poison.c > > This doesn't build on 64-bit big endian: > > tm-poison.c: In function 'tm_poison_test': > tm-poison.c:118:10: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=] > printf("Unknown value %#lx leaked into f31!\n", unknown); > ^ > tm-poison.c:166:10: error: format '%lx' expects argument of type 'long unsigned int', but argument 2 has type 'uint64_t {aka long long unsigned int}' [-Werror=format=] > printf("Unknown value %#lx leaked into vr31!\n", unknown); > ^ Sorry. I sent a v2 addressing it. Now I tested the fix against Travis CI. Thank you. Cheers, Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction 2019-09-03 4:47 [PATCH 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Neuling 2019-09-03 4:47 ` [PATCH 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts Michael Neuling 2019-09-03 4:47 ` [PATCH 3/3] powerpc/tm: Add tm-poison test Michael Neuling @ 2019-09-04 4:55 ` gromero 2019-09-04 4:55 ` [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts gromero ` (2 more replies) 2 siblings, 3 replies; 11+ messages in thread From: gromero @ 2019-09-04 4:55 UTC (permalink / raw) To: linuxppc-dev, mikey; +Cc: cyrilbur, gromero From: Gustavo Romero <gromero@linux.ibm.com> When we take an FP unavailable exception in a transaction we have to account for the hardware FP TM checkpointed registers being incorrect. In this case for this process we know the current and checkpointed FP registers must be the same (since FP wasn't used inside the transaction) hence in the thread_struct we copy the current FP registers to the checkpointed ones. This copy is done in tm_reclaim_thread(). We use thread->ckpt_regs.msr to determine if FP was on when in userspace. thread->ckpt_regs.msr represents the state of the MSR when exiting userspace. This is setup by check_if_tm_restore_required(). Unfortunatley there is an optimisation in giveup_all() which returns early if tsk->thread.regs->msr (via local variable `usermsr`) has FP=VEC=VSX=SPE=0. This optimisation means that check_if_tm_restore_required() is not called and hence thread->ckpt_regs.msr is not updated and will contain an old value. This can happen if due to load_fp=255 we start a userspace process with MSR FP=1 and then we are context switched out. In this case thread->ckpt_regs.msr will contain FP=1. If that same process is then context switched in and load_fp overflows, MSR will have FP=0. If that process now enters a transaction and does an FP instruction, the FP unavailable will not update thread->ckpt_regs.msr (the bug) and MSR FP=1 will be retained in thread->ckpt_regs.msr. tm_reclaim_thread() will then not perform the required memcpy and the checkpointed FP regs in the thread struct will contain the wrong values. The code path for this happening is: Userspace: Kernel Start userspace with MSR FP/VEC/VSX/SPE=0 TM=1 < ----- ... tbegin bne fp instruction FP unavailable ---- > fp_unavailable_tm() tm_reclaim_current() tm_reclaim_thread() giveup_all() return early since FP/VMX/VSX=0 /* ckpt MSR not updated (Incorrect) */ tm_reclaim() /* thread_struct ckpt FP regs contain junk (OK) */ /* Sees ckpt MSR FP=1 (Incorrect) */ no memcpy() performed /* thread_struct ckpt FP regs not fixed (Incorrect) */ tm_recheckpoint() /* Put junk in hardware checkpoint FP regs */ .... < ----- Return to userspace with MSR TM=1 FP=1 with junk in the FP TM checkpoint TM rollback reads FP junk This is a data integrity problem for the current process as the FP registers are corrupted. It's also a security problem as the FP registers from one process may be leaked to another. This patch moves up check_if_tm_restore_required() in giveup_all() to ensure thread->ckpt_regs.msr is updated correctly. A simple testcase to replicate this will be posted to tools/testing/selftests/powerpc/tm/tm-poison.c Similarly for VMX. This fixes CVE-2019-15030. Fixes: f48e91e87e67 ("powerpc/tm: Fix FP and VMX register corruption") Cc: stable@vger.kernel.org # 4.12+ Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 8fc4de0d22b4..437b57068cf8 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -497,13 +497,14 @@ void giveup_all(struct task_struct *tsk) if (!tsk->thread.regs) return; + check_if_tm_restore_required(tsk); + usermsr = tsk->thread.regs->msr; if ((usermsr & msr_all_available) == 0) return; msr_check_and_set(msr_all_available); - check_if_tm_restore_required(tsk); WARN_ON((usermsr & MSR_VSX) && !((usermsr & MSR_FP) && (usermsr & MSR_VEC))); -- 2.17.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero @ 2019-09-04 4:55 ` gromero 2019-09-05 12:05 ` Michael Ellerman 2019-09-04 4:55 ` [PATCH v2 3/3] powerpc/tm: Add tm-poison test gromero 2019-09-05 12:05 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Ellerman 2 siblings, 1 reply; 11+ messages in thread From: gromero @ 2019-09-04 4:55 UTC (permalink / raw) To: linuxppc-dev, mikey; +Cc: cyrilbur, gromero From: Gustavo Romero <gromero@linux.ibm.com> When in userspace and MSR FP=0 the hardware FP state is unrelated to the current process. This is extended for transactions where if tbegin is run with FP=0, the hardware checkpoint FP state will also be unrelated to the current process. Due to this, we need to ensure this hardware checkpoint is updated with the correct state before we enable FP for this process. Unfortunately we get this wrong when returning to a process from a hardware interrupt. A process that starts a transaction with FP=0 can take an interrupt. When the kernel returns back to that process, we change to FP=1 but with hardware checkpoint FP state not updated. If this transaction is then rolled back, the FP registers now contain the wrong state. The process looks like this: Userspace: Kernel Start userspace with MSR FP=0 TM=1 < ----- ... tbegin bne Hardware interrupt ---- > <do_IRQ...> .... ret_from_except restore_math() /* sees FP=0 */ restore_fp() tm_active_with_fp() /* sees FP=1 (Incorrect) */ load_fp_state() FP = 0 -> 1 < ----- Return to userspace with MSR TM=1 FP=1 with junk in the FP TM checkpoint TM rollback reads FP junk When returning from the hardware exception, tm_active_with_fp() is incorrectly making restore_fp() call load_fp_state() which is setting FP=1. The fix is to remove tm_active_with_fp(). tm_active_with_fp() is attempting to handle the case where FP state has been changed inside a transaction. In this case the checkpointed and transactional FP state is different and hence we must restore the FP state (ie. we can't do lazy FP restore inside a transaction that's used FP). It's safe to remove tm_active_with_fp() as this case is handled by restore_tm_state(). restore_tm_state() detects if FP has been using inside a transaction and will set load_fp and call restore_math() to ensure the FP state (checkpoint and transaction) is restored. This is a data integrity problem for the current process as the FP registers are corrupted. It's also a security problem as the FP registers from one process may be leaked to another. Similarly for VMX. A simple testcase to replicate this will be posted to tools/testing/selftests/powerpc/tm/tm-poison.c This fixes CVE-2019-15031. Fixes: a7771176b439 ("powerpc: Don't enable FP/Altivec if not checkpointed") Cc: stable@vger.kernel.org # 4.15+ Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- arch/powerpc/kernel/process.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 437b57068cf8..7a84c9f1778e 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -101,21 +101,8 @@ static void check_if_tm_restore_required(struct task_struct *tsk) } } -static bool tm_active_with_fp(struct task_struct *tsk) -{ - return MSR_TM_ACTIVE(tsk->thread.regs->msr) && - (tsk->thread.ckpt_regs.msr & MSR_FP); -} - -static bool tm_active_with_altivec(struct task_struct *tsk) -{ - return MSR_TM_ACTIVE(tsk->thread.regs->msr) && - (tsk->thread.ckpt_regs.msr & MSR_VEC); -} #else static inline void check_if_tm_restore_required(struct task_struct *tsk) { } -static inline bool tm_active_with_fp(struct task_struct *tsk) { return false; } -static inline bool tm_active_with_altivec(struct task_struct *tsk) { return false; } #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ bool strict_msr_control; @@ -252,7 +239,7 @@ EXPORT_SYMBOL(enable_kernel_fp); static int restore_fp(struct task_struct *tsk) { - if (tsk->thread.load_fp || tm_active_with_fp(tsk)) { + if (tsk->thread.load_fp) { load_fp_state(¤t->thread.fp_state); current->thread.load_fp++; return 1; @@ -334,8 +321,7 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread); static int restore_altivec(struct task_struct *tsk) { - if (cpu_has_feature(CPU_FTR_ALTIVEC) && - (tsk->thread.load_vec || tm_active_with_altivec(tsk))) { + if (cpu_has_feature(CPU_FTR_ALTIVEC) && (tsk->thread.load_vec)) { load_vr_state(&tsk->thread.vr_state); tsk->thread.used_vr = 1; tsk->thread.load_vec++; -- 2.17.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts 2019-09-04 4:55 ` [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts gromero @ 2019-09-05 12:05 ` Michael Ellerman 0 siblings, 0 replies; 11+ messages in thread From: Michael Ellerman @ 2019-09-05 12:05 UTC (permalink / raw) To: gromero, linuxppc-dev, mikey; +Cc: cyrilbur, gromero On Wed, 2019-09-04 at 04:55:28 UTC, gromero wrote: > From: Gustavo Romero <gromero@linux.ibm.com> > > When in userspace and MSR FP=0 the hardware FP state is unrelated to > the current process. This is extended for transactions where if tbegin > is run with FP=0, the hardware checkpoint FP state will also be > unrelated to the current process. Due to this, we need to ensure this > hardware checkpoint is updated with the correct state before we enable > FP for this process. ... > > This fixes CVE-2019-15031. > > Fixes: a7771176b439 ("powerpc: Don't enable FP/Altivec if not checkpointed") > Cc: stable@vger.kernel.org # 4.15+ > Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> > Signed-off-by: Michael Neuling <mikey@neuling.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/a8318c13e79badb92bc6640704a64cc022a6eb97 cheers ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] powerpc/tm: Add tm-poison test 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero 2019-09-04 4:55 ` [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts gromero @ 2019-09-04 4:55 ` gromero 2019-09-25 11:05 ` Michael Ellerman 2019-09-05 12:05 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Ellerman 2 siblings, 1 reply; 11+ messages in thread From: gromero @ 2019-09-04 4:55 UTC (permalink / raw) To: linuxppc-dev, mikey; +Cc: cyrilbur, gromero From: Gustavo Romero <gromero@linux.ibm.com> Add TM selftest to check if FP or VEC register values from one process can leak into another process when both run on the same CPU. Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> Signed-off-by: Michael Neuling <mikey@neuling.org> --- tools/testing/selftests/powerpc/tm/.gitignore | 1 + tools/testing/selftests/powerpc/tm/Makefile | 2 +- .../testing/selftests/powerpc/tm/tm-poison.c | 179 ++++++++++++++++++ 3 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/powerpc/tm/tm-poison.c diff --git a/tools/testing/selftests/powerpc/tm/.gitignore b/tools/testing/selftests/powerpc/tm/.gitignore index 951fe855f7cd..98f2708d86cc 100644 --- a/tools/testing/selftests/powerpc/tm/.gitignore +++ b/tools/testing/selftests/powerpc/tm/.gitignore @@ -17,3 +17,4 @@ tm-vmx-unavail tm-unavailable tm-trap tm-sigreturn +tm-poison diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile index c0734ed0ef56..b15a1a325bd0 100644 --- a/tools/testing/selftests/powerpc/tm/Makefile +++ b/tools/testing/selftests/powerpc/tm/Makefile @@ -5,7 +5,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-signal-context-chk-fpu TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-stack \ tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap \ $(SIGNAL_CONTEXT_CHK_TESTS) tm-sigreturn tm-signal-sigreturn-nt \ - tm-signal-context-force-tm + tm-signal-context-force-tm tm-poison top_srcdir = ../../../../.. include ../../lib.mk diff --git a/tools/testing/selftests/powerpc/tm/tm-poison.c b/tools/testing/selftests/powerpc/tm/tm-poison.c new file mode 100644 index 000000000000..977558497c16 --- /dev/null +++ b/tools/testing/selftests/powerpc/tm/tm-poison.c @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2019, Gustavo Romero, Michael Neuling, IBM Corp. + * + * This test will spawn two processes. Both will be attached to the same + * CPU (CPU 0). The child will be in a loop writing to FP register f31 and + * VMX/VEC/Altivec register vr31 a known value, called poison, calling + * sched_yield syscall after to allow the parent to switch on the CPU. + * Parent will set f31 and vr31 to 1 and in a loop will check if f31 and + * vr31 remain 1 as expected until a given timeout (2m). If the issue is + * present child's poison will leak into parent's f31 or vr31 registers, + * otherwise, poison will never leak into parent's f31 and vr31 registers. + */ + +#define _GNU_SOURCE +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <inttypes.h> +#include <sched.h> +#include <sys/types.h> +#include <signal.h> +#include <inttypes.h> + +#include "tm.h" + +int tm_poison_test(void) +{ + int pid; + cpu_set_t cpuset; + uint64_t poison = 0xdeadbeefc0dec0fe; + uint64_t unknown = 0; + bool fail_fp = false; + bool fail_vr = false; + + SKIP_IF(!have_htm()); + + /* Attach both Child and Parent to CPU 0 */ + CPU_ZERO(&cpuset); + CPU_SET(0, &cpuset); + sched_setaffinity(0, sizeof(cpuset), &cpuset); + + pid = fork(); + if (!pid) { + /** + * child + */ + while (1) { + sched_yield(); + asm ( + "mtvsrd 31, %[poison];" // f31 = poison + "mtvsrd 63, %[poison];" // vr31 = poison + + : : [poison] "r" (poison) : ); + } + } + + /** + * parent + */ + asm ( + /* + * Set r3, r4, and f31 to known value 1 before entering + * in transaction. They won't be written after that. + */ + " li 3, 0x1 ;" + " li 4, 0x1 ;" + " mtvsrd 31, 4 ;" + + /* + * The Time Base (TB) is a 64-bit counter register that is + * independent of the CPU clock and which is incremented + * at a frequency of 512000000 Hz, so every 1.953125ns. + * So it's necessary 120s/0.000000001953125s = 61440000000 + * increments to get a 2 minutes timeout. Below we set that + * value in r5 and then use r6 to track initial TB value, + * updating TB values in r7 at every iteration and comparing it + * to r6. When r7 (current) - r6 (initial) > 61440000000 we bail + * out since for sure we spent already 2 minutes in the loop. + * SPR 268 is the TB register. + */ + " lis 5, 14 ;" + " ori 5, 5, 19996 ;" + " sldi 5, 5, 16 ;" // r5 = 61440000000 + + " mfspr 6, 268 ;" // r6 (TB initial) + "1: mfspr 7, 268 ;" // r7 (TB current) + " subf 7, 6, 7 ;" // r7 - r6 > 61440000000 ? + " cmpd 7, 5 ;" + " bgt 3f ;" // yes, exit + + /* + * Main loop to check f31 + */ + " tbegin. ;" // no, try again + " beq 1b ;" // restart if no timeout + " mfvsrd 3, 31 ;" // read f31 + " cmpd 3, 4 ;" // f31 == 1 ? + " bne 2f ;" // broken :-( + " tabort. 3 ;" // try another transaction + "2: tend. ;" // commit transaction + "3: mr %[unknown], 3 ;" // record r3 + + : [unknown] "=r" (unknown) + : + : "cr0", "r3", "r4", "r5", "r6", "r7", "vs31" + + ); + + /* + * On leak 'unknown' will contain 'poison' value from child, + * otherwise (no leak) 'unknown' will contain the same value + * as r3 before entering in transactional mode, i.e. 0x1. + */ + fail_fp = unknown != 0x1; + if (fail_fp) + printf("Unknown value %#"PRIx64" leaked into f31!\n", unknown); + else + printf("Good, no poison or leaked value into FP registers\n"); + + asm ( + /* + * Set r3, r4, and vr31 to known value 1 before entering + * in transaction. They won't be written after that. + */ + " li 3, 0x1 ;" + " li 4, 0x1 ;" + " mtvsrd 63, 4 ;" + + " lis 5, 14 ;" + " ori 5, 5, 19996 ;" + " sldi 5, 5, 16 ;" // r5 = 61440000000 + + " mfspr 6, 268 ;" // r6 (TB initial) + "1: mfspr 7, 268 ;" // r7 (TB current) + " subf 7, 6, 7 ;" // r7 - r6 > 61440000000 ? + " cmpd 7, 5 ;" + " bgt 3f ;" // yes, exit + + /* + * Main loop to check vr31 + */ + " tbegin. ;" // no, try again + " beq 1b ;" // restart if no timeout + " mfvsrd 3, 63 ;" // read vr31 + " cmpd 3, 4 ;" // vr31 == 1 ? + " bne 2f ;" // broken :-( + " tabort. 3 ;" // try another transaction + "2: tend. ;" // commit transaction + "3: mr %[unknown], 3 ;" // record r3 + + : [unknown] "=r" (unknown) + : + : "cr0", "r3", "r4", "r5", "r6", "r7", "vs63" + + ); + + /* + * On leak 'unknown' will contain 'poison' value from child, + * otherwise (no leak) 'unknown' will contain the same value + * as r3 before entering in transactional mode, i.e. 0x1. + */ + fail_vr = unknown != 0x1; + if (fail_vr) + printf("Unknown value %#"PRIx64" leaked into vr31!\n", unknown); + else + printf("Good, no poison or leaked value into VEC registers\n"); + + kill(pid, SIGKILL); + + return (fail_fp | fail_vr); +} + +int main(int argc, char *argv[]) +{ + /* Test completes in about 4m */ + test_harness_set_timeout(250); + return test_harness(tm_poison_test, "tm_poison_test"); +} -- 2.17.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] powerpc/tm: Add tm-poison test 2019-09-04 4:55 ` [PATCH v2 3/3] powerpc/tm: Add tm-poison test gromero @ 2019-09-25 11:05 ` Michael Ellerman 0 siblings, 0 replies; 11+ messages in thread From: Michael Ellerman @ 2019-09-25 11:05 UTC (permalink / raw) To: gromero, linuxppc-dev, mikey; +Cc: cyrilbur, gromero On Wed, 2019-09-04 at 04:55:29 UTC, gromero wrote: > From: Gustavo Romero <gromero@linux.ibm.com> > > Add TM selftest to check if FP or VEC register values from one process > can leak into another process when both run on the same CPU. > > Signed-off-by: Gustavo Romero <gromero@linux.ibm.com> > Signed-off-by: Michael Neuling <mikey@neuling.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/a003365cab64b0f7988ac3ccb1da895ce0bece5e cheers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero 2019-09-04 4:55 ` [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts gromero 2019-09-04 4:55 ` [PATCH v2 3/3] powerpc/tm: Add tm-poison test gromero @ 2019-09-05 12:05 ` Michael Ellerman 2 siblings, 0 replies; 11+ messages in thread From: Michael Ellerman @ 2019-09-05 12:05 UTC (permalink / raw) To: gromero, linuxppc-dev, mikey; +Cc: cyrilbur, gromero On Wed, 2019-09-04 at 04:55:27 UTC, gromero wrote: > From: Gustavo Romero <gromero@linux.ibm.com> > > When we take an FP unavailable exception in a transaction we have to > account for the hardware FP TM checkpointed registers being > incorrect. In this case for this process we know the current and > checkpointed FP registers must be the same (since FP wasn't used > inside the transaction) hence in the thread_struct we copy the current > FP registers to the checkpointed ones. ... > > This fixes CVE-2019-15030. > > Fixes: f48e91e87e67 ("powerpc/tm: Fix FP and VMX register corruption") > Cc: stable@vger.kernel.org # 4.12+ > Signed-off-by: Gustavo Romero <gromero@linux.vnet.ibm.com> > Signed-off-by: Michael Neuling <mikey@neuling.org> Applied to powerpc fixes, thanks. https://git.kernel.org/powerpc/c/8205d5d98ef7f155de211f5e2eb6ca03d95a5a60 cheers ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-25 11:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-03 4:47 [PATCH 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Neuling 2019-09-03 4:47 ` [PATCH 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts Michael Neuling 2019-09-03 4:47 ` [PATCH 3/3] powerpc/tm: Add tm-poison test Michael Neuling 2019-09-03 11:46 ` Michael Ellerman 2019-09-04 4:59 ` Gustavo Romero 2019-09-04 4:55 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction gromero 2019-09-04 4:55 ` [PATCH v2 2/3] powerpc/tm: Fix restoring FP/VMX facility incorrectly on interrupts gromero 2019-09-05 12:05 ` Michael Ellerman 2019-09-04 4:55 ` [PATCH v2 3/3] powerpc/tm: Add tm-poison test gromero 2019-09-25 11:05 ` Michael Ellerman 2019-09-05 12:05 ` [PATCH v2 1/3] powerpc/tm: Fix FP/VMX unavailable exceptions inside a transaction Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).