| Message ID | f3af9f2b-ddc2-f0e2-3c80-6e93f9839861@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 27 Jan 2018 14:23:57 -0800 Florian Fainelli <f.fainelli@gmail.com> wrote: > On 01/25/2018 07:21 AM, Marc Zyngier wrote: > > In order to avoid aliasing attacks against the branch predictor, > > Cortex-A15 require to invalidate the BTB when switching > > from one user context to another. The only way to do so on this > > CPU is to perform an ICIALLU, having set ACTLR[0] to 1 from secure > > mode. > > Even though this is a platform design mistake, let's say your Linux > kernel boots in secure supervisor mode, we could have code > that tries to set ACTLR[0] as early as possible, since the writes are > ignored if executing from non-secure mode. If Linux is booted normally > either PL1 or PL2 non-secure we could still check ACTLR[0]. > > My concern is that without doing this, we may have a hard time catching > improper firmware as well as having bogus bug reports. This is > completely RFC though since: > > - I could not quite figure out yet why update ca15_actlr_status from > assembly is not reflected in the C code despite using PC relative loads One potential problem is that you're writing this with the MMU off, and reading it with the MMU on. Unless you've added CMOs to make this visible, it will not be reliable. Also, I'm not sure how this works with MCPM. > - this is done in __ca15mp_setup and __b15mp_setup because we know by > then that we have either of these two CPUs but we could presumably move > this check under a Kconfig option and earlier where all erratas are checked Thanks, M.
On 01/28/2018 03:55 AM, Marc Zyngier wrote: > On Sat, 27 Jan 2018 14:23:57 -0800 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> On 01/25/2018 07:21 AM, Marc Zyngier wrote: >>> In order to avoid aliasing attacks against the branch predictor, >>> Cortex-A15 require to invalidate the BTB when switching >>> from one user context to another. The only way to do so on this >>> CPU is to perform an ICIALLU, having set ACTLR[0] to 1 from secure >>> mode. >> >> Even though this is a platform design mistake, let's say your Linux >> kernel boots in secure supervisor mode, we could have code >> that tries to set ACTLR[0] as early as possible, since the writes are >> ignored if executing from non-secure mode. If Linux is booted normally >> either PL1 or PL2 non-secure we could still check ACTLR[0]. >> >> My concern is that without doing this, we may have a hard time catching >> improper firmware as well as having bogus bug reports. This is >> completely RFC though since: >> >> - I could not quite figure out yet why update ca15_actlr_status from >> assembly is not reflected in the C code despite using PC relative loads > > One potential problem is that you're writing this with the MMU off, and > reading it with the MMU on. Unless you've added CMOs to make this > visible, it will not be reliable. I did not, let's change that. > > Also, I'm not sure how this works with MCPM. I just started looking at MCPM, do you know roughly at what point do non-MCPM and MCPM entry points converge? If we moved the check for ACTLR[0] there, would that be too late. > >> - this is done in __ca15mp_setup and __b15mp_setup because we know by >> then that we have either of these two CPUs but we could presumably move >> this check under a Kconfig option and earlier where all erratas are checked > > Thanks, > > M. >
On 29/01/18 18:05, Florian Fainelli wrote: > On 01/28/2018 03:55 AM, Marc Zyngier wrote: >> On Sat, 27 Jan 2018 14:23:57 -0800 >> Florian Fainelli <f.fainelli@gmail.com> wrote: >> >>> On 01/25/2018 07:21 AM, Marc Zyngier wrote: >>>> In order to avoid aliasing attacks against the branch predictor, >>>> Cortex-A15 require to invalidate the BTB when switching >>>> from one user context to another. The only way to do so on this >>>> CPU is to perform an ICIALLU, having set ACTLR[0] to 1 from secure >>>> mode. >>> >>> Even though this is a platform design mistake, let's say your Linux >>> kernel boots in secure supervisor mode, we could have code >>> that tries to set ACTLR[0] as early as possible, since the writes are >>> ignored if executing from non-secure mode. If Linux is booted normally >>> either PL1 or PL2 non-secure we could still check ACTLR[0]. >>> >>> My concern is that without doing this, we may have a hard time catching >>> improper firmware as well as having bogus bug reports. This is >>> completely RFC though since: >>> >>> - I could not quite figure out yet why update ca15_actlr_status from >>> assembly is not reflected in the C code despite using PC relative loads >> >> One potential problem is that you're writing this with the MMU off, and >> reading it with the MMU on. Unless you've added CMOs to make this >> visible, it will not be reliable. > > I did not, let's change that. > >> >> Also, I'm not sure how this works with MCPM. > > I just started looking at MCPM, do you know roughly at what point do > non-MCPM and MCPM entry points converge? If we moved the check for > ACTLR[0] there, would that be too late. I have no idea. You'll have to reverse-engineer it (MCPM makes me feel slightly sick). M.
diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h index f2c36acf9886..e46a79c74d07 100644 --- a/arch/arm/include/asm/smp_plat.h +++ b/arch/arm/include/asm/smp_plat.h @@ -117,4 +117,6 @@ static inline int platform_can_hotplug_cpu(unsigned int cpu) } #endif +extern void ca15_actlr_check(void); + #endif diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index fc40a2b40595..7519d948f5a4 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -1061,6 +1061,44 @@ void __init hyp_mode_check(void) #endif } +#ifdef CONFIG_CPU_V7 +extern unsigned char cpu_ca15_actlr_status; + +void __init ca15_actlr_check(void) +{ + pr_info("%s: reading from 0x%08x\n", __func__, (unsigned int)&cpu_ca15_actlr_status); + /* Check the status of ACTLR[0], here are the possible states: + * + * 0b00: Firmware has set ACTLR[0], we are golden + * 0b01: Firmware did not set ACTLR[0], kernel ran in secure mode + * and so we could set ACTLR[0], we are okay, but you should + * reconsider your boot flow to have the kernel run in non + * secure mode + * 0b10: The firmware did not set ACTLR[0], and the kernel could not + * set it either, this is really bad, you are suspectibe to + * Spectre variant 2 (CVE-2017-5715). + */ + switch (cpu_ca15_actlr_status) { + case 0: + pr_info("CPU: ACTLR[0] set by firmware\n"); + break; + case 1: + pr_warn("CPU: ACTLR[0] set by kernel, CPU running in SVC secure mode!\n"); + break; + case 2: + pr_crit("CPU: Firmware did not set ACTLR[0], vulnerable to CVE-2017-5715 variant 2\n"); + break; + default: + pr_err("Invalid value: 0x%02x\n", cpu_ca15_actlr_status); + break; + } +} +#else +void __init ca15_actlr_check(void) +{ +} +#endif + void __init setup_arch(char **cmdline_p) { const struct machine_desc *mdesc; @@ -1140,8 +1178,10 @@ void __init setup_arch(char **cmdline_p) } #endif - if (!is_smp()) + if (!is_smp()) { hyp_mode_check(); + ca15_actlr_check(); + } reserve_crashkernel(); diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index b4fbf00ee4ad..1b01a6b4e287 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -429,6 +429,7 @@ void __init smp_cpus_done(unsigned int max_cpus) (bogosum / (5000/HZ)) % 100); hyp_mode_check(); + ca15_actlr_check(); } void __init smp_prepare_boot_cpu(void) diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S index 513e327bf509..d09c2579c8a6 100644 --- a/arch/arm/mm/proc-v7.S +++ b/arch/arm/mm/proc-v7.S @@ -267,6 +267,13 @@ ENDPROC(cpu_pj4b_do_resume) #endif +#ifdef CONFIG_ARM_LPAE + /* LPAE requires an additional page for the PGD */ +#define PMD_ORDER 3 +#else +#define PMD_ORDER 2 +#endif + /* * __v7_setup * @@ -288,14 +295,26 @@ __v7_ca5mp_setup: __v7_ca9mp_setup: __v7_cr7mp_setup: mov r10, #(1 << 0) @ Cache/TLB ops broadcasting - b 1f __v7_ca7mp_setup: __v7_ca12mp_setup: + b 1f __v7_ca15mp_setup: __v7_b15mp_setup: -__v7_ca17mp_setup: + /* Spectre variant 2, ACTLR[0] check */ + ldr r11, =cpu_ca15_actlr_status mov r10, #0 -1: adr r0, __v7_setup_stack_ptr +3: mrc p15, 0, r0, c1, c0, 1 @ Read ACTLR + tst r0, #(1 << 0) @ ACTRL[0] was set, good + bne 2f + add r10, r10, #1 @ Remember this was not set + strb r10, [r11] + orr r0, r0, #(1 << 0) @ Try to set ACTLR[0] now + mcr p15, 0, r0, c1, c0, 1 + cmp r10, #1 @ First time we tried setting it + beq 3b @ we need to check the value again +__v7_ca17mp_setup: +1: mov r10, #0 +2: adr r0, __v7_setup_stack_ptr ldr r12, [r0] add r12, r12, r0 @ the local stack stmia r12, {r1-r6, lr} @ v7_invalidate_l1 touches r0-r6 @@ -559,6 +578,11 @@ ENDPROC(__v7_setup) __v7_setup_stack: .space 4 * 7 @ 7 registers + .section ".data" + .globl cpu_ca15_actlr_status +cpu_ca15_actlr_status: + .space 1, 0 + __INITDATA @ define struct processor (see <asm/proc-fns.h> and proc-macros.S) @@ -571,7 +595,6 @@ __v7_setup_stack: #ifdef CONFIG_CPU_PJ4B define_processor_functions pj4b, dabort=v7_early_abort, pabort=v7_pabort, suspend=1 #endif - .section ".rodata" string cpu_arch_name, "armv7"