| Message ID | 20181031135713.2873-10-dave.long@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | V4.9 backport of 32-bit arm spectre patches | expand |
On Wed, 31 Oct 2018 13:56:58 +0000, David Long <dave.long@linaro.org> wrote: > > From: Russell King <rmk+kernel@armlinux.org.uk> > > Commit 10115105cb3aa17b5da1cb726ae8dd5f6854bd93 upstream. > > Add firmware based hardening for cores that require more complex > handling in firmware. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > Boot-tested-by: Tony Lindgren <tony@atomide.com> > Reviewed-by: Tony Lindgren <tony@atomide.com> > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: David A. Long <dave.long@linaro.org> > --- > arch/arm/mm/proc-v7-bugs.c | 60 ++++++++++++++++++++++++++++++++++++++ > arch/arm/mm/proc-v7.S | 21 +++++++++++++ > 2 files changed, 81 insertions(+) > [...] > diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S > index 2d2e5ae85816..8fde9edb4a48 100644 > --- a/arch/arm/mm/proc-v7.S > +++ b/arch/arm/mm/proc-v7.S > @@ -9,6 +9,7 @@ > * > * This is the "shell" of the ARMv7 processor support. > */ > +#include <linux/arm-smccc.h> > #include <linux/init.h> > #include <linux/linkage.h> > #include <asm/assembler.h> > @@ -88,6 +89,26 @@ ENTRY(cpu_v7_dcache_clean_area) > ret lr > ENDPROC(cpu_v7_dcache_clean_area) > > +#ifdef CONFIG_ARM_PSCI > + .arch_extension sec > +ENTRY(cpu_v7_smc_switch_mm) > + stmfd sp!, {r0 - r3} > + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > + smc #0 > + ldmfd sp!, {r0 - r3} > + b cpu_v7_switch_mm > +ENDPROC(cpu_v7_smc_switch_mm) > + .arch_extension virt > +ENTRY(cpu_v7_hvc_switch_mm) > + stmfd sp!, {r0 - r3} > + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > + hvc #0 > + ldmfd sp!, {r0 - r3} > + b cpu_v7_switch_mm > +ENDPROC(cpu_v7_smc_switch_mm) As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. Please keep this series on hold until this is fixed in mainline and you can cherry-pick the corresponding patch. Thanks, M. [1] https://patchwork.kernel.org/patch/10475033/ -- Jazz is not dead, it just smell funny.
On Tue, Nov 06, 2018 at 10:40:33AM +0000, Marc Zyngier wrote: > On Wed, 31 Oct 2018 13:56:58 +0000, > David Long <dave.long@linaro.org> wrote: > > > > From: Russell King <rmk+kernel@armlinux.org.uk> > > > > Commit 10115105cb3aa17b5da1cb726ae8dd5f6854bd93 upstream. > > > > Add firmware based hardening for cores that require more complex > > handling in firmware. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Boot-tested-by: Tony Lindgren <tony@atomide.com> > > Reviewed-by: Tony Lindgren <tony@atomide.com> > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > Signed-off-by: David A. Long <dave.long@linaro.org> > > --- > > arch/arm/mm/proc-v7-bugs.c | 60 ++++++++++++++++++++++++++++++++++++++ > > arch/arm/mm/proc-v7.S | 21 +++++++++++++ > > 2 files changed, 81 insertions(+) > > > > [...] > > > diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S > > index 2d2e5ae85816..8fde9edb4a48 100644 > > --- a/arch/arm/mm/proc-v7.S > > +++ b/arch/arm/mm/proc-v7.S > > @@ -9,6 +9,7 @@ > > * > > * This is the "shell" of the ARMv7 processor support. > > */ > > +#include <linux/arm-smccc.h> > > #include <linux/init.h> > > #include <linux/linkage.h> > > #include <asm/assembler.h> > > @@ -88,6 +89,26 @@ ENTRY(cpu_v7_dcache_clean_area) > > ret lr > > ENDPROC(cpu_v7_dcache_clean_area) > > > > +#ifdef CONFIG_ARM_PSCI > > + .arch_extension sec > > +ENTRY(cpu_v7_smc_switch_mm) > > + stmfd sp!, {r0 - r3} > > + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > > + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > > + smc #0 > > + ldmfd sp!, {r0 - r3} > > + b cpu_v7_switch_mm > > +ENDPROC(cpu_v7_smc_switch_mm) > > + .arch_extension virt > > +ENTRY(cpu_v7_hvc_switch_mm) > > + stmfd sp!, {r0 - r3} > > + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > > + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > > + hvc #0 > > + ldmfd sp!, {r0 - r3} > > + b cpu_v7_switch_mm > > +ENDPROC(cpu_v7_smc_switch_mm) > > As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. > Please keep this series on hold until this is fixed in mainline and > you can cherry-pick the corresponding patch. You have to wonder at the effectiveness of the autobooters if stuff like this is not caught. There's way too many configuration combinations and firmwares for individuals to be able to test every code path, we need autobooters to have sufficient diversity (and to pick up on failures better) to be able to exercise these in an automated fashion and report decent, reliable results. It's taken from May to November to find this, which is _way_ too long a timeframe. The Thumb annotations for functions are always going to be very troublesome as there's no automated way to validate them except by actually exercising the code. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
On Tue, Nov 06, 2018 at 10:55:00AM +0000, Russell King - ARM Linux wrote: > On Tue, Nov 06, 2018 at 10:40:33AM +0000, Marc Zyngier wrote: > > As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. > > Please keep this series on hold until this is fixed in mainline and > > you can cherry-pick the corresponding patch. > You have to wonder at the effectiveness of the autobooters if stuff > like this is not caught. There's way too many configuration > combinations and firmwares for individuals to be able to test every > code path, we need autobooters to have sufficient diversity (and to > pick up on failures better) to be able to exercise these in an > automated fashion and report decent, reliable results. Right, and it depends on what people are willing to contribute hardware wise. However in the case of Thumb it's just a config option so we should probably ensure that there's at least one config that's at least getting booted, we could put something in the kernel source but I'm thinking that the easiest thing would be to teach at least KernelCI to just add a multi_v7+THUMB2 build (and then there's userspace too!). I'll try look into that after Plumbers, I've got some other stuff queued up there anyway. > It's taken from May to November to find this, which is _way_ too long > a timeframe. > The Thumb annotations for functions are always going to be very > troublesome as there's no automated way to validate them except by > actually exercising the code. There's some work going on on adding runtime testing which will help a bit there as it'll improve coverage but it's never going to exercise everything.
On 11/6/18 5:40 AM, Marc Zyngier wrote: > On Wed, 31 Oct 2018 13:56:58 +0000, > David Long <dave.long@linaro.org> wrote: >> >> From: Russell King <rmk+kernel@armlinux.org.uk> >> >> Commit 10115105cb3aa17b5da1cb726ae8dd5f6854bd93 upstream. >> >> Add firmware based hardening for cores that require more complex >> handling in firmware. >> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> Boot-tested-by: Tony Lindgren <tony@atomide.com> >> Reviewed-by: Tony Lindgren <tony@atomide.com> >> Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> >> Signed-off-by: David A. Long <dave.long@linaro.org> >> --- >> arch/arm/mm/proc-v7-bugs.c | 60 ++++++++++++++++++++++++++++++++++++++ >> arch/arm/mm/proc-v7.S | 21 +++++++++++++ >> 2 files changed, 81 insertions(+) >> > > [...] > >> diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S >> index 2d2e5ae85816..8fde9edb4a48 100644 >> --- a/arch/arm/mm/proc-v7.S >> +++ b/arch/arm/mm/proc-v7.S >> @@ -9,6 +9,7 @@ >> * >> * This is the "shell" of the ARMv7 processor support. >> */ >> +#include <linux/arm-smccc.h> >> #include <linux/init.h> >> #include <linux/linkage.h> >> #include <asm/assembler.h> >> @@ -88,6 +89,26 @@ ENTRY(cpu_v7_dcache_clean_area) >> ret lr >> ENDPROC(cpu_v7_dcache_clean_area) >> >> +#ifdef CONFIG_ARM_PSCI >> + .arch_extension sec >> +ENTRY(cpu_v7_smc_switch_mm) >> + stmfd sp!, {r0 - r3} >> + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 >> + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 >> + smc #0 >> + ldmfd sp!, {r0 - r3} >> + b cpu_v7_switch_mm >> +ENDPROC(cpu_v7_smc_switch_mm) >> + .arch_extension virt >> +ENTRY(cpu_v7_hvc_switch_mm) >> + stmfd sp!, {r0 - r3} >> + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 >> + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 >> + hvc #0 >> + ldmfd sp!, {r0 - r3} >> + b cpu_v7_switch_mm >> +ENDPROC(cpu_v7_smc_switch_mm) > > As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. > Please keep this series on hold until this is fixed in mainline and > you can cherry-pick the corresponding patch. > > Thanks, > > M. > > [1] https://patchwork.kernel.org/patch/10475033/ > Note that it looks like this problem is now in v4.14 stable too. -dl
On Tue, Nov 06, 2018 at 11:20:19AM -0500, David Long wrote: > On 11/6/18 5:40 AM, Marc Zyngier wrote: > >On Wed, 31 Oct 2018 13:56:58 +0000, > >David Long <dave.long@linaro.org> wrote: > >> > >>From: Russell King <rmk+kernel@armlinux.org.uk> > >> > >>Commit 10115105cb3aa17b5da1cb726ae8dd5f6854bd93 upstream. > >> > >>Add firmware based hardening for cores that require more complex > >>handling in firmware. > >> > >>Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > >>Boot-tested-by: Tony Lindgren <tony@atomide.com> > >>Reviewed-by: Tony Lindgren <tony@atomide.com> > >>Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > >>Signed-off-by: David A. Long <dave.long@linaro.org> > >>--- > >> arch/arm/mm/proc-v7-bugs.c | 60 ++++++++++++++++++++++++++++++++++++++ > >> arch/arm/mm/proc-v7.S | 21 +++++++++++++ > >> 2 files changed, 81 insertions(+) > >> > > > >[...] > > > >>diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S > >>index 2d2e5ae85816..8fde9edb4a48 100644 > >>--- a/arch/arm/mm/proc-v7.S > >>+++ b/arch/arm/mm/proc-v7.S > >>@@ -9,6 +9,7 @@ > >> * > >> * This is the "shell" of the ARMv7 processor support. > >> */ > >>+#include <linux/arm-smccc.h> > >> #include <linux/init.h> > >> #include <linux/linkage.h> > >> #include <asm/assembler.h> > >>@@ -88,6 +89,26 @@ ENTRY(cpu_v7_dcache_clean_area) > >> ret lr > >> ENDPROC(cpu_v7_dcache_clean_area) > >>+#ifdef CONFIG_ARM_PSCI > >>+ .arch_extension sec > >>+ENTRY(cpu_v7_smc_switch_mm) > >>+ stmfd sp!, {r0 - r3} > >>+ movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > >>+ movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > >>+ smc #0 > >>+ ldmfd sp!, {r0 - r3} > >>+ b cpu_v7_switch_mm > >>+ENDPROC(cpu_v7_smc_switch_mm) > >>+ .arch_extension virt > >>+ENTRY(cpu_v7_hvc_switch_mm) > >>+ stmfd sp!, {r0 - r3} > >>+ movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 > >>+ movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 > >>+ hvc #0 > >>+ ldmfd sp!, {r0 - r3} > >>+ b cpu_v7_switch_mm > >>+ENDPROC(cpu_v7_smc_switch_mm) > > > >As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. > >Please keep this series on hold until this is fixed in mainline and > >you can cherry-pick the corresponding patch. > > > >Thanks, > > > > M. > > > >[1] https://patchwork.kernel.org/patch/10475033/ > > > > Note that it looks like this problem is now in v4.14 stable too. The good news is that Linus has just pulled the fix into mainline, so we can now poke Greg to pick it up for all stable kernels - but, as a result, we're going to get into a bit of a mess because it's going to require careful management of which stable kernels, and getting it applied by indirect reference along with _these_ patches. I'm not sure if we've just made things easier or harder. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
On Tue, Nov 06, 2018 at 04:19:32PM +0000, Mark Brown wrote: > On Tue, Nov 06, 2018 at 10:55:00AM +0000, Russell King - ARM Linux wrote: > > On Tue, Nov 06, 2018 at 10:40:33AM +0000, Marc Zyngier wrote: > > > > As pointed out by Ard a while ago [1], this breaks Thumb-2 kernels. > > > Please keep this series on hold until this is fixed in mainline and > > > you can cherry-pick the corresponding patch. > > > You have to wonder at the effectiveness of the autobooters if stuff > > like this is not caught. There's way too many configuration > > combinations and firmwares for individuals to be able to test every > > code path, we need autobooters to have sufficient diversity (and to > > pick up on failures better) to be able to exercise these in an > > automated fashion and report decent, reliable results. > > Right, and it depends on what people are willing to contribute hardware > wise. However in the case of Thumb it's just a config option so we > should probably ensure that there's at least one config that's at least > getting booted, we could put something in the kernel source but I'm > thinking that the easiest thing would be to teach at least KernelCI to > just add a multi_v7+THUMB2 build (and then there's userspace too!). > I'll try look into that after Plumbers, I've got some other stuff queued > up there anyway. With any missing ENDPROC(), the only way to currently detect it is by trying to run the code path - building alone does not flag any warnings or errors. That's because the assembler has no idea whether what is being assembled is code or data, and the purpose of ENDPROC() is to mark it as code for the rest of the toolchain, so that it can apply the bit 0 "fixup" for Thumb2 code. So, in this case, the only way the error is detectable is to have a platform where we boot a kernel which makes use of the HVC fixup path. If that's too much to ask, then we're just going to have to accept that Thumb2 kernels are going to be more fragile than ARM kernels because there's no way to be certain that we have the correct annotations everywhere - so we're going to have to rely on users reporting these bugs _after_ the changes have hit mainline. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
On Tue, Nov 06, 2018 at 04:30:44PM +0000, Russell King - ARM Linux wrote: > With any missing ENDPROC(), the only way to currently detect it is by > trying to run the code path - building alone does not flag any warnings > or errors. That's because the assembler has no idea whether what is > being assembled is code or data, and the purpose of ENDPROC() is to > mark it as code for the rest of the toolchain, so that it can apply the > bit 0 "fixup" for Thumb2 code. > So, in this case, the only way the error is detectable is to have a > platform where we boot a kernel which makes use of the HVC fixup path. > If that's too much to ask, then we're just going to have to accept that > Thumb2 kernels are going to be more fragile than ARM kernels because > there's no way to be certain that we have the correct annotations > everywhere - so we're going to have to rely on users reporting these > bugs _after_ the changes have hit mainline. Oh, totally - but currently none of the automated stuff is even trying to boot it so if we can do literally anything we'll be better off in terms of coverage even though it doesn't address the fundemental fragility.
diff --git a/arch/arm/mm/proc-v7-bugs.c b/arch/arm/mm/proc-v7-bugs.c index 85a2e3d6263c..da25a38e1897 100644 --- a/arch/arm/mm/proc-v7-bugs.c +++ b/arch/arm/mm/proc-v7-bugs.c @@ -1,14 +1,20 @@ // SPDX-License-Identifier: GPL-2.0 +#include <linux/arm-smccc.h> #include <linux/kernel.h> +#include <linux/psci.h> #include <linux/smp.h> #include <asm/cp15.h> #include <asm/cputype.h> +#include <asm/proc-fns.h> #include <asm/system_misc.h> #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR DEFINE_PER_CPU(harden_branch_predictor_fn_t, harden_branch_predictor_fn); +extern void cpu_v7_smc_switch_mm(phys_addr_t pgd_phys, struct mm_struct *mm); +extern void cpu_v7_hvc_switch_mm(phys_addr_t pgd_phys, struct mm_struct *mm); + static void harden_branch_predictor_bpiall(void) { write_sysreg(0, BPIALL); @@ -19,6 +25,16 @@ static void harden_branch_predictor_iciallu(void) write_sysreg(0, ICIALLU); } +static void __maybe_unused call_smc_arch_workaround_1(void) +{ + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_1, NULL); +} + +static void __maybe_unused call_hvc_arch_workaround_1(void) +{ + arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_WORKAROUND_1, NULL); +} + static void cpu_v7_spectre_init(void) { const char *spectre_v2_method = NULL; @@ -45,7 +61,51 @@ static void cpu_v7_spectre_init(void) harden_branch_predictor_iciallu; spectre_v2_method = "ICIALLU"; break; + +#ifdef CONFIG_ARM_PSCI + default: + /* Other ARM CPUs require no workaround */ + if (read_cpuid_implementor() == ARM_CPU_IMP_ARM) + break; + /* fallthrough */ + /* Cortex A57/A72 require firmware workaround */ + case ARM_CPU_PART_CORTEX_A57: + case ARM_CPU_PART_CORTEX_A72: { + struct arm_smccc_res res; + + if (psci_ops.smccc_version == SMCCC_VERSION_1_0) + break; + + switch (psci_ops.conduit) { + case PSCI_CONDUIT_HVC: + arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, + ARM_SMCCC_ARCH_WORKAROUND_1, &res); + if ((int)res.a0 != 0) + break; + per_cpu(harden_branch_predictor_fn, cpu) = + call_hvc_arch_workaround_1; + processor.switch_mm = cpu_v7_hvc_switch_mm; + spectre_v2_method = "hypervisor"; + break; + + case PSCI_CONDUIT_SMC: + arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID, + ARM_SMCCC_ARCH_WORKAROUND_1, &res); + if ((int)res.a0 != 0) + break; + per_cpu(harden_branch_predictor_fn, cpu) = + call_smc_arch_workaround_1; + processor.switch_mm = cpu_v7_smc_switch_mm; + spectre_v2_method = "firmware"; + break; + + default: + break; + } } +#endif + } + if (spectre_v2_method) pr_info("CPU%u: Spectre v2: using %s workaround\n", smp_processor_id(), spectre_v2_method); diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S index 2d2e5ae85816..8fde9edb4a48 100644 --- a/arch/arm/mm/proc-v7.S +++ b/arch/arm/mm/proc-v7.S @@ -9,6 +9,7 @@ * * This is the "shell" of the ARMv7 processor support. */ +#include <linux/arm-smccc.h> #include <linux/init.h> #include <linux/linkage.h> #include <asm/assembler.h> @@ -88,6 +89,26 @@ ENTRY(cpu_v7_dcache_clean_area) ret lr ENDPROC(cpu_v7_dcache_clean_area) +#ifdef CONFIG_ARM_PSCI + .arch_extension sec +ENTRY(cpu_v7_smc_switch_mm) + stmfd sp!, {r0 - r3} + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 + smc #0 + ldmfd sp!, {r0 - r3} + b cpu_v7_switch_mm +ENDPROC(cpu_v7_smc_switch_mm) + .arch_extension virt +ENTRY(cpu_v7_hvc_switch_mm) + stmfd sp!, {r0 - r3} + movw r0, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1 + movt r0, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1 + hvc #0 + ldmfd sp!, {r0 - r3} + b cpu_v7_switch_mm +ENDPROC(cpu_v7_smc_switch_mm) +#endif ENTRY(cpu_v7_iciallu_switch_mm) mov r3, #0 mcr p15, 0, r3, c7, c5, 0 @ ICIALLU