Re: Spectre defence for armv7

2018-01-14 Thread Mark Kettenis
> Date: Sat, 13 Jan 2018 23:08:55 +
> From: Dimitris Papastamos 
> 
> > +   case CPU_ID_CORTEX_A15:
> > +   case CPU_ID_CORTEX_A57:
> > +   case CPU_ID_CORTEX_A72:
> > +   /*
> > +* Vulnerable; BPIALL is "not effective" so must use
> > +* ICIALLU and hope the firmware set the magic bit in
> > +* the ACTLR that actually forces a BTB flush.
> > +*/
> > +   ci->ci_flush_bp = cortex_a15_flush_bp;
> > +   break;
> > +   }
> 
> Unfortunately there is an A57 erratum (833069) that when "Disabling
> MMU Translation with CPUACTLR_EL1 "Enable Invalidates of BTB" bit set
> can cause Invalidate by PA or VA to fail".  The only safe way to
> trigger BTB invalidation on A57 is to disable and enable the MMU.

Thanks for the heads-up.  I remember reading that erratum thinking
"that sucks", and then forgot about it.  I'll leave Cortex-A57 out for
now and will revisit this once I have the PSCI call implemented.
Running OpenBSD/armv7 on Cortex-A57 is largely academic anyway.



Re: Spectre defence for armv7

2018-01-13 Thread Dimitris Papastamos
> + case CPU_ID_CORTEX_A15:
> + case CPU_ID_CORTEX_A57:
> + case CPU_ID_CORTEX_A72:
> + /*
> +  * Vulnerable; BPIALL is "not effective" so must use
> +  * ICIALLU and hope the firmware set the magic bit in
> +  * the ACTLR that actually forces a BTB flush.
> +  */
> + ci->ci_flush_bp = cortex_a15_flush_bp;
> + break;
> + }

Unfortunately there is an A57 erratum (833069) that when "Disabling
MMU Translation with CPUACTLR_EL1 "Enable Invalidates of BTB" bit set
can cause Invalidate by PA or VA to fail".  The only safe way to
trigger BTB invalidation on A57 is to disable and enable the MMU.



Spectre defence for armv7

2018-01-13 Thread Mark Kettenis
The diff below improves our resiliency against "variant 2".  Like on
x86 the defence is based on flushing the branch predictor cache at the
appropriate points.

It turns out we are already in pretty good shape as we are already
flushing on context switches.  I believe we're forced to do that
because we don't use ASIDs.  The other place that matters seems to be
when trapping because userland tried to access the kernel part of the
address space in an attempt to "train" the branch predictor.  I don't
pretend to fully understand the details but it makes some sense to me.

Now flushing the branch predictor cache is slightly tricky.  In
principle the architecture defines an instruction to do the flush.
But it is "ineffective" on Cortex-A15 (and Cortex-A57 and Cortex-A72
which are based on the same design and could in principle be run in
32-bit mode).  The following link gives a good summary:

https://github.com/ARM-software/arm-trusted-firmware/wiki/ARM-Trusted-Firmware-Security-Advisory-TFV-6

and seems to be consistent with what they did in the Linux kernel:

https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=kpti

After some thinking I decided that the default should probably be to
use BPIALL as that is the architected instruction and most likely to
work correctly on future processors as the Cortex-A15/A57/A72 design
seems to have been phased out in favour of a newer design.

I deliberately moved the hook into "struct cpuinfo" because big.LITTLE
designs often pair vulnerable and non-vulnerable cores.

ok?


Index: arch/arm/arm/cpu.c
===
RCS file: /cvs/src/sys/arch/arm/arm/cpu.c,v
retrieving revision 1.43
diff -u -p -r1.43 cpu.c
--- arch/arm/arm/cpu.c  29 Dec 2017 14:45:15 -  1.43
+++ arch/arm/arm/cpu.c  13 Jan 2018 22:24:08 -
@@ -257,6 +257,42 @@ identify_arm_cpu(struct device *dv, stru
 
printf("\n");
 
+   /*
+* Some ARM processors are vulnerable to branch target
+* injection attacks.
+*/
+   switch (cpuid & CPU_ID_CORTEX_MASK) {
+   case CPU_ID_CORTEX_A5:
+   case CPU_ID_CORTEX_A7:
+   case CPU_ID_CORTEX_A32:
+   case CPU_ID_CORTEX_A35:
+   case CPU_ID_CORTEX_A53:
+   case CPU_ID_CORTEX_A55:
+   /* Not vulnerable; no need to flush. */
+   ci->ci_flush_bp = cpufunc_nullop;
+   break;
+   case CPU_ID_CORTEX_A8:
+   case CPU_ID_CORTEX_A9:
+   case CPU_ID_CORTEX_A12:
+   case CPU_ID_CORTEX_A17:
+   case CPU_ID_CORTEX_A73:
+   case CPU_ID_CORTEX_A75:
+   default:
+   /* Vulnerable; flush BP cache. */
+   ci->ci_flush_bp = armv7_flush_bp;
+   break;
+   case CPU_ID_CORTEX_A15:
+   case CPU_ID_CORTEX_A57:
+   case CPU_ID_CORTEX_A72:
+   /*
+* Vulnerable; BPIALL is "not effective" so must use
+* ICIALLU and hope the firmware set the magic bit in
+* the ACTLR that actually forces a BTB flush.
+*/
+   ci->ci_flush_bp = cortex_a15_flush_bp;
+   break;
+   }
+
/* Print cache info. */
if (arm_picache_line_size == 0 && arm_pdcache_line_size == 0)
goto skip_pcache;
Index: arch/arm/arm/cpufunc_asm_armv7.S
===
RCS file: /cvs/src/sys/arch/arm/arm/cpufunc_asm_armv7.S,v
retrieving revision 1.14
diff -u -p -r1.14 cpufunc_asm_armv7.S
--- arch/arm/arm/cpufunc_asm_armv7.S15 Aug 2016 21:08:56 -  1.14
+++ arch/arm/arm/cpufunc_asm_armv7.S13 Jan 2018 22:24:08 -
@@ -209,6 +209,17 @@ ENTRY(armv7_dcache_inv_range)
 
 
 /*
+ * BTB functions.
+ */
+ENTRY(armv7_flush_bp)
+   mcr CP15_BPIALL
+   mov pc, lr
+
+ENTRY(cortex_a15_flush_bp)
+   mcr CP15_ICIALLU/* Heavy hammer; BPIALL is a no-op */
+   mov pc, lr
+
+/*
  * Context switch.
  *
  * These is the CPU-specific parts of the context switcher cpu_switch()
Index: arch/arm/arm/fault.c
===
RCS file: /cvs/src/sys/arch/arm/arm/fault.c,v
retrieving revision 1.30
diff -u -p -r1.30 fault.c
--- arch/arm/arm/fault.c8 Sep 2017 05:36:51 -   1.30
+++ arch/arm/arm/fault.c13 Jan 2018 22:24:08 -
@@ -209,6 +209,15 @@ data_abort_handler(trapframe_t *tf)
goto out;
}
 
+   va = trunc_page((vaddr_t)far);
+
+   /*
+* Flush BP cache on processors that are vulnerable to branch
+* target injection attacks if access is outside user space.
+*/
+   if (va < VM_MIN_ADDRESS || va >= VM_MAX_ADDRESS)
+   curcpu()->ci_flush_bp();
+
/*
 * At this point, we're dealing with one of the following data aborts:
 *
@@ -255,8 +264,6 @@ data_abort_handler(trapframe_t *tf)
"Program Counter\n");