* [PATCH 1/7] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_* macros
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
@ 2018-01-19 13:40 ` Julien Grall
2018-01-24 23:03 ` Stefano Stabellini
2018-01-19 13:40 ` [PATCH 2/7] xen/arm32: Add missing MIDR values for Cortex-A17 and A12 Julien Grall
` (5 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:40 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
The only difference between all the DEFINE_TRAP_ENTRY_* macros are the
interrupts (Asynchronous Abort, IRQ, FIQ) unmasked.
Rather than duplicating the code, introduce __DEFINE_TRAP_ENTRY macro
that will take the list of interrupts to unmask.
This is part of XSA-254.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/arm32/entry.S | 36 +++++++++++++-----------------------
1 file changed, 13 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 120922e64e..c6490d2847 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -111,39 +111,29 @@ abort_guest_exit_end:
skip_check:
mov pc, lr
-#define DEFINE_TRAP_ENTRY(trap) \
+/*
+ * Macro to define trap entry. The iflags corresponds to the list of
+ * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
+ */
+#define __DEFINE_TRAP_ENTRY(trap, iflags) \
ALIGN; \
trap_##trap: \
SAVE_ALL; \
- cpsie i; /* local_irq_enable */ \
- cpsie a; /* asynchronous abort enable */ \
+ cpsie iflags; \
adr lr, return_from_trap; \
mov r0, sp; \
mov r11, sp; \
bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
b do_trap_##trap
-#define DEFINE_TRAP_ENTRY_NOIRQ(trap) \
- ALIGN; \
-trap_##trap: \
- SAVE_ALL; \
- cpsie a; /* asynchronous abort enable */ \
- adr lr, return_from_trap; \
- mov r0, sp; \
- mov r11, sp; \
- bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
- b do_trap_##trap
+/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
+#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
-#define DEFINE_TRAP_ENTRY_NOABORT(trap) \
- ALIGN; \
-trap_##trap: \
- SAVE_ALL; \
- cpsie i; /* local_irq_enable */ \
- adr lr, return_from_trap; \
- mov r0, sp; \
- mov r11, sp; \
- bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
- b do_trap_##trap
+/* Trap handler which unmask Abort, keep IRQ/FIQ masked */
+#define DEFINE_TRAP_ENTRY_NOIRQ(trap) __DEFINE_TRAP_ENTRY(trap, a)
+
+/* Trap handler which unmask IRQ, keep Abort/FIQ masked */
+#define DEFINE_TRAP_ENTRY_NOABORT(trap) __DEFINE_TRAP_ENTRY(trap, i)
.align 5
GLOBAL(hyp_traps_vector)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 1/7] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_* macros
2018-01-19 13:40 ` [PATCH 1/7] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_* macros Julien Grall
@ 2018-01-24 23:03 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-24 23:03 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> The only difference between all the DEFINE_TRAP_ENTRY_* macros are the
> interrupts (Asynchronous Abort, IRQ, FIQ) unmasked.
>
> Rather than duplicating the code, introduce __DEFINE_TRAP_ENTRY macro
> that will take the list of interrupts to unmask.
>
> This is part of XSA-254.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/arch/arm/arm32/entry.S | 36 +++++++++++++-----------------------
> 1 file changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 120922e64e..c6490d2847 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -111,39 +111,29 @@ abort_guest_exit_end:
> skip_check:
> mov pc, lr
>
> -#define DEFINE_TRAP_ENTRY(trap) \
> +/*
> + * Macro to define trap entry. The iflags corresponds to the list of
> + * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
> + */
> +#define __DEFINE_TRAP_ENTRY(trap, iflags) \
> ALIGN; \
> trap_##trap: \
> SAVE_ALL; \
> - cpsie i; /* local_irq_enable */ \
> - cpsie a; /* asynchronous abort enable */ \
> + cpsie iflags; \
> adr lr, return_from_trap; \
> mov r0, sp; \
> mov r11, sp; \
> bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
> b do_trap_##trap
>
> -#define DEFINE_TRAP_ENTRY_NOIRQ(trap) \
> - ALIGN; \
> -trap_##trap: \
> - SAVE_ALL; \
> - cpsie a; /* asynchronous abort enable */ \
> - adr lr, return_from_trap; \
> - mov r0, sp; \
> - mov r11, sp; \
> - bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
> - b do_trap_##trap
> +/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
> +#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
>
> -#define DEFINE_TRAP_ENTRY_NOABORT(trap) \
> - ALIGN; \
> -trap_##trap: \
> - SAVE_ALL; \
> - cpsie i; /* local_irq_enable */ \
> - adr lr, return_from_trap; \
> - mov r0, sp; \
> - mov r11, sp; \
> - bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
> - b do_trap_##trap
> +/* Trap handler which unmask Abort, keep IRQ/FIQ masked */
> +#define DEFINE_TRAP_ENTRY_NOIRQ(trap) __DEFINE_TRAP_ENTRY(trap, a)
> +
> +/* Trap handler which unmask IRQ, keep Abort/FIQ masked */
> +#define DEFINE_TRAP_ENTRY_NOABORT(trap) __DEFINE_TRAP_ENTRY(trap, i)
>
> .align 5
> GLOBAL(hyp_traps_vector)
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 2/7] xen/arm32: Add missing MIDR values for Cortex-A17 and A12
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
2018-01-19 13:40 ` [PATCH 1/7] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_* macros Julien Grall
@ 2018-01-19 13:40 ` Julien Grall
2018-01-24 23:06 ` Stefano Stabellini
2018-01-19 13:40 ` [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry Julien Grall
` (4 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:40 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
Cortex-A17 and A12 MIDR will be used in a follow-up patch for hardening
the branch predictor.
This is part of XSA-254.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/include/asm-arm/processor.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 466da5da86..c0f79d0093 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -44,6 +44,8 @@
#define ARM_CPU_IMP_ARM 0x41
+#define ARM_CPU_PART_CORTEX_A12 0xC0D
+#define ARM_CPU_PART_CORTEX_A17 0xC0E
#define ARM_CPU_PART_CORTEX_A15 0xC0F
#define ARM_CPU_PART_CORTEX_A53 0xD03
#define ARM_CPU_PART_CORTEX_A57 0xD07
@@ -51,6 +53,8 @@
#define ARM_CPU_PART_CORTEX_A73 0xD09
#define ARM_CPU_PART_CORTEX_A75 0xD0A
+#define MIDR_CORTEX_A12 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A12)
+#define MIDR_CORTEX_A17 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A17)
#define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)
#define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
#define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 2/7] xen/arm32: Add missing MIDR values for Cortex-A17 and A12
2018-01-19 13:40 ` [PATCH 2/7] xen/arm32: Add missing MIDR values for Cortex-A17 and A12 Julien Grall
@ 2018-01-24 23:06 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-24 23:06 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> Cortex-A17 and A12 MIDR will be used in a follow-up patch for hardening
> the branch predictor.
>
> This is part of XSA-254.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/include/asm-arm/processor.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 466da5da86..c0f79d0093 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -44,6 +44,8 @@
>
> #define ARM_CPU_IMP_ARM 0x41
>
> +#define ARM_CPU_PART_CORTEX_A12 0xC0D
> +#define ARM_CPU_PART_CORTEX_A17 0xC0E
> #define ARM_CPU_PART_CORTEX_A15 0xC0F
> #define ARM_CPU_PART_CORTEX_A53 0xD03
> #define ARM_CPU_PART_CORTEX_A57 0xD07
> @@ -51,6 +53,8 @@
> #define ARM_CPU_PART_CORTEX_A73 0xD09
> #define ARM_CPU_PART_CORTEX_A75 0xD0A
>
> +#define MIDR_CORTEX_A12 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A12)
> +#define MIDR_CORTEX_A17 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A17)
> #define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A15)
> #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A53)
> #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57)
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
2018-01-19 13:40 ` [PATCH 1/7] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_* macros Julien Grall
2018-01-19 13:40 ` [PATCH 2/7] xen/arm32: Add missing MIDR values for Cortex-A17 and A12 Julien Grall
@ 2018-01-19 13:40 ` Julien Grall
2018-01-24 23:14 ` Stefano Stabellini
2018-01-19 13:41 ` [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks Julien Grall
` (3 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:40 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
At the moment, the reset vector is defined as .word 0 (e.g andeq r0, r0,
r0).
This is rather unintuitive and will result to execute the trap
undefined. Instead introduce trap helpers for reset and will generate an
error message in the unlikely case that reset will be called.
This is part of XSA-254.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/arm32/entry.S | 1 +
xen/arch/arm/arm32/traps.c | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index c6490d2847..c2fad5fe9b 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -146,6 +146,7 @@ GLOBAL(hyp_traps_vector)
b trap_irq /* 0x18 - IRQ */
b trap_fiq /* 0x1c - FIQ */
+DEFINE_TRAP_ENTRY(reset)
DEFINE_TRAP_ENTRY(undefined_instruction)
DEFINE_TRAP_ENTRY(hypervisor_call)
DEFINE_TRAP_ENTRY(prefetch_abort)
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 705255883e..4f27543dec 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -23,6 +23,11 @@
#include <asm/processor.h>
+void do_trap_reset(struct cpu_user_regs *regs)
+{
+ do_unexpected_trap("Reset", regs);
+}
+
void do_trap_undefined_instruction(struct cpu_user_regs *regs)
{
uint32_t pc = regs->pc;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry
2018-01-19 13:40 ` [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry Julien Grall
@ 2018-01-24 23:14 ` Stefano Stabellini
2018-01-25 11:24 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-24 23:14 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> At the moment, the reset vector is defined as .word 0 (e.g andeq r0, r0,
> r0).
>
> This is rather unintuitive and will result to execute the trap
> undefined. Instead introduce trap helpers for reset and will generate an
> error message in the unlikely case that reset will be called.
>
> This is part of XSA-254.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
> xen/arch/arm/arm32/entry.S | 1 +
> xen/arch/arm/arm32/traps.c | 5 +++++
> 2 files changed, 6 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index c6490d2847..c2fad5fe9b 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -146,6 +146,7 @@ GLOBAL(hyp_traps_vector)
> b trap_irq /* 0x18 - IRQ */
> b trap_fiq /* 0x1c - FIQ */
>
> +DEFINE_TRAP_ENTRY(reset)
This is OK, but shouldn't we also change the entry under
GLOBAL(hyp_traps_vector), from ".word 0" to "b trap_reset" ?
> DEFINE_TRAP_ENTRY(undefined_instruction)
> DEFINE_TRAP_ENTRY(hypervisor_call)
> DEFINE_TRAP_ENTRY(prefetch_abort)
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index 705255883e..4f27543dec 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -23,6 +23,11 @@
>
> #include <asm/processor.h>
>
> +void do_trap_reset(struct cpu_user_regs *regs)
> +{
> + do_unexpected_trap("Reset", regs);
> +}
> +
> void do_trap_undefined_instruction(struct cpu_user_regs *regs)
> {
> uint32_t pc = regs->pc;
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry
2018-01-24 23:14 ` Stefano Stabellini
@ 2018-01-25 11:24 ` Julien Grall
0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2018-01-25 11:24 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi Stefano,
On 24/01/18 23:14, Stefano Stabellini wrote:
> On Fri, 19 Jan 2018, Julien Grall wrote:
>> At the moment, the reset vector is defined as .word 0 (e.g andeq r0, r0,
>> r0).
>>
>> This is rather unintuitive and will result to execute the trap
>> undefined. Instead introduce trap helpers for reset and will generate an
>> error message in the unlikely case that reset will be called.
>>
>> This is part of XSA-254.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>> xen/arch/arm/arm32/entry.S | 1 +
>> xen/arch/arm/arm32/traps.c | 5 +++++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index c6490d2847..c2fad5fe9b 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -146,6 +146,7 @@ GLOBAL(hyp_traps_vector)
>> b trap_irq /* 0x18 - IRQ */
>> b trap_fiq /* 0x1c - FIQ */
>>
>> +DEFINE_TRAP_ENTRY(reset)
>
> This is OK, but shouldn't we also change the entry under
> GLOBAL(hyp_traps_vector), from ".word 0" to "b trap_reset" ?
That was my plan but forgot to do it :/ I will update the patch and
resend it.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
` (2 preceding siblings ...)
2018-01-19 13:40 ` [PATCH 3/7] xen/arm32: entry: Add missing trap_reset entry Julien Grall
@ 2018-01-19 13:41 ` Julien Grall
2018-01-24 23:54 ` Stefano Stabellini
2018-01-19 13:41 ` [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12 Julien Grall
` (2 subsequent siblings)
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:41 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
Aliasing attacked against CPU branch predictors can allow an attacker to
redirect speculative control flow on some CPUs and potentially divulge
information from one context to another.
This patch adds initiatial skeleton code behind a new Kconfig option
to enable implementation-specific mitigations against these attacks
for CPUs that are affected.
Most of mitigations will have to be applied when entering to the
hypervisor from the guest context.
Because the attack is against branch predictor, it is not possible to
safely use branch instruction before the mitigation is applied.
Therefore this has to be done in the vector entry before jump to the
helper handling a given exception.
However, on arm32, each vector contain a single instruction. This means
that the hardened vector tables may rely on the state of registers that
does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
Therefore hypervisor code running with guest vectors table should be
minimized and always have interrupts masked to reduce the risk to use
them.
This patch provides an infrastructure to switch vector tables before
entering to the guest and when leaving it.
Note that alternative could have been used, but older Xen (4.8 or
earlier) doesn't have support. So avoid using alternative to ease
backporting.
This is part of XSA-254.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/Kconfig | 3 +++
xen/arch/arm/arm32/entry.S | 41 ++++++++++++++++++++++++++++++++++++++++-
xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 06fd85cc77..2782ee6589 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
config ARM64_HARDEN_BRANCH_PREDICTOR
def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
+config ARM32_HARDEN_BRANCH_PREDICTOR
+ def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
+
source "common/Kconfig"
source "drivers/Kconfig"
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index c2fad5fe9b..54a1733f87 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -34,6 +34,20 @@
blne save_guest_regs
save_guest_regs:
+#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
+ /*
+ * Restore vectors table to the default as it may have been
+ * changed when returning to the guest (see
+ * return_to_hypervisor). We need to do that early (e.g before
+ * any interrupts are unmasked) because hardened vectors requires
+ * SP to be 8 bytes aligned. This does not hold when running in
+ * the hypervisor.
+ */
+ ldr r1, =hyp_traps_vector
+ mcr p15, 4, r1, c12, c0, 0
+ isb
+#endif
+
ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
str r11, [sp, #UREGS_sp]
SAVE_ONE_BANKED(SP_usr)
@@ -179,12 +193,37 @@ return_to_guest:
RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
/* Fall thru */
return_to_hypervisor:
- cpsid i
+ cpsid ai
ldr lr, [sp, #UREGS_lr]
ldr r11, [sp, #UREGS_pc]
msr ELR_hyp, r11
ldr r11, [sp, #UREGS_cpsr]
msr SPSR_hyp, r11
+#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
+ /*
+ * Hardening branch predictor may require to setup a different
+ * vector tables before returning to the guests. Those vectors
+ * may rely on the state of registers that does not hold when
+ * running in the hypervisor (e.g SP is 8 bytes aligned). So setup
+ * HVBAR very late.
+ *
+ * Default vectors table will be restored on exit (see
+ * save_guest_regs).
+ */
+ mov r9, #0 /* vector tables = NULL */
+ /*
+ * Load vector tables pointer from the per-cpu bp_harden_vecs
+ * when returning to the guest only.
+ */
+ and r11, #PSR_MODE_MASK
+ cmp r11, #PSR_MODE_HYP
+ ldrne r11, =per_cpu__bp_harden_vecs
+ mrcne p15, 4, r10, c13, c0, 2 /* r10 = per-cpu offset (HTPIDR) */
+ addne r11, r11, r10 /* r11 = offset of the vector tables */
+ ldrne r9, [r11] /* r9 = vector tables */
+ cmp r9, #0 /* Only update HVBAR when the vector */
+ mcrne p15, 4, r9, c12, c0, 0 /* tables is not NULL. */
+#endif
pop {r0-r12}
add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
clrex
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index f1ea7f3c5b..0a138fa735 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -170,6 +170,36 @@ static int enable_psci_bp_hardening(void *data)
#endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
+/* Hardening Branch predictor code for Arm32 */
+#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
+
+/*
+ * Per-CPU vector tables to use when returning to the guests. They will
+ * only be used on platform requiring to harden the branch predictor.
+ */
+DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
+
+extern char hyp_traps_vector_bp_inv[];
+
+static void __maybe_unused
+install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
+ const char *hyp_vecs, const char *desc)
+{
+ /*
+ * Enable callbacks are called on every CPU based on the
+ * capabilities. So double-check whether the CPU matches the
+ * entry.
+ */
+ if ( !entry->matches(entry) )
+ return;
+
+ printk(XENLOG_INFO "CPU%u will %s on guest exit\n",
+ smp_processor_id(), desc);
+ this_cpu(bp_harden_vecs) = hyp_vecs;
+}
+
+#endif
+
#define MIDR_RANGE(model, min, max) \
.matches = is_affected_midr_range, \
.midr_model = model, \
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-19 13:41 ` [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks Julien Grall
@ 2018-01-24 23:54 ` Stefano Stabellini
2018-01-25 11:36 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-24 23:54 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> Aliasing attacked against CPU branch predictors can allow an attacker to
> redirect speculative control flow on some CPUs and potentially divulge
> information from one context to another.
>
> This patch adds initiatial skeleton code behind a new Kconfig option
> to enable implementation-specific mitigations against these attacks
> for CPUs that are affected.
>
> Most of mitigations will have to be applied when entering to the
> hypervisor from the guest context.
>
> Because the attack is against branch predictor, it is not possible to
> safely use branch instruction before the mitigation is applied.
> Therefore this has to be done in the vector entry before jump to the
> helper handling a given exception.
>
> However, on arm32, each vector contain a single instruction. This means
> that the hardened vector tables may rely on the state of registers that
> does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
> Therefore hypervisor code running with guest vectors table should be
> minimized and always have interrupts masked to reduce the risk to use
> them.
>
> This patch provides an infrastructure to switch vector tables before
> entering to the guest and when leaving it.
>
> Note that alternative could have been used, but older Xen (4.8 or
> earlier) doesn't have support. So avoid using alternative to ease
> backporting.
>
> This is part of XSA-254.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
I only have a couple of questions. It looks good.
> ---
> xen/arch/arm/Kconfig | 3 +++
> xen/arch/arm/arm32/entry.S | 41 ++++++++++++++++++++++++++++++++++++++++-
> xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 06fd85cc77..2782ee6589 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
> config ARM64_HARDEN_BRANCH_PREDICTOR
> def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
>
> +config ARM32_HARDEN_BRANCH_PREDICTOR
> + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
> +
> source "common/Kconfig"
>
> source "drivers/Kconfig"
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index c2fad5fe9b..54a1733f87 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -34,6 +34,20 @@
> blne save_guest_regs
>
> save_guest_regs:
> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> + /*
> + * Restore vectors table to the default as it may have been
> + * changed when returning to the guest (see
> + * return_to_hypervisor). We need to do that early (e.g before
> + * any interrupts are unmasked) because hardened vectors requires
> + * SP to be 8 bytes aligned. This does not hold when running in
> + * the hypervisor.
> + */
> + ldr r1, =hyp_traps_vector
> + mcr p15, 4, r1, c12, c0, 0
> + isb
> +#endif
> +
> ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
> str r11, [sp, #UREGS_sp]
> SAVE_ONE_BANKED(SP_usr)
> @@ -179,12 +193,37 @@ return_to_guest:
> RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
> /* Fall thru */
> return_to_hypervisor:
> - cpsid i
> + cpsid ai
Why?
> ldr lr, [sp, #UREGS_lr]
> ldr r11, [sp, #UREGS_pc]
> msr ELR_hyp, r11
> ldr r11, [sp, #UREGS_cpsr]
> msr SPSR_hyp, r11
> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> + /*
> + * Hardening branch predictor may require to setup a different
> + * vector tables before returning to the guests. Those vectors
> + * may rely on the state of registers that does not hold when
> + * running in the hypervisor (e.g SP is 8 bytes aligned). So setup
> + * HVBAR very late.
> + *
> + * Default vectors table will be restored on exit (see
> + * save_guest_regs).
> + */
> + mov r9, #0 /* vector tables = NULL */
> + /*
> + * Load vector tables pointer from the per-cpu bp_harden_vecs
> + * when returning to the guest only.
> + */
> + and r11, #PSR_MODE_MASK
> + cmp r11, #PSR_MODE_HYP
> + ldrne r11, =per_cpu__bp_harden_vecs
> + mrcne p15, 4, r10, c13, c0, 2 /* r10 = per-cpu offset (HTPIDR) */
> + addne r11, r11, r10 /* r11 = offset of the vector tables */
> + ldrne r9, [r11] /* r9 = vector tables */
> + cmp r9, #0 /* Only update HVBAR when the vector */
> + mcrne p15, 4, r9, c12, c0, 0 /* tables is not NULL. */
Do we need/want an isb here? Or maybe it is not necessary because we are
about to eret?
> +#endif
> pop {r0-r12}
> add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
> clrex
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index f1ea7f3c5b..0a138fa735 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -170,6 +170,36 @@ static int enable_psci_bp_hardening(void *data)
>
> #endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
>
> +/* Hardening Branch predictor code for Arm32 */
> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> +
> +/*
> + * Per-CPU vector tables to use when returning to the guests. They will
> + * only be used on platform requiring to harden the branch predictor.
> + */
> +DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
> +
> +extern char hyp_traps_vector_bp_inv[];
> +
> +static void __maybe_unused
> +install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
> + const char *hyp_vecs, const char *desc)
> +{
> + /*
> + * Enable callbacks are called on every CPU based on the
> + * capabilities. So double-check whether the CPU matches the
> + * entry.
> + */
> + if ( !entry->matches(entry) )
> + return;
> +
> + printk(XENLOG_INFO "CPU%u will %s on guest exit\n",
> + smp_processor_id(), desc);
> + this_cpu(bp_harden_vecs) = hyp_vecs;
> +}
> +
> +#endif
> +
> #define MIDR_RANGE(model, min, max) \
> .matches = is_affected_midr_range, \
> .midr_model = model, \
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-24 23:54 ` Stefano Stabellini
@ 2018-01-25 11:36 ` Julien Grall
2018-01-25 18:45 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-25 11:36 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi Stefano,
On 24/01/18 23:54, Stefano Stabellini wrote:
> On Fri, 19 Jan 2018, Julien Grall wrote:
>> Aliasing attacked against CPU branch predictors can allow an attacker to
>> redirect speculative control flow on some CPUs and potentially divulge
>> information from one context to another.
>>
>> This patch adds initiatial skeleton code behind a new Kconfig option
>> to enable implementation-specific mitigations against these attacks
>> for CPUs that are affected.
>>
>> Most of mitigations will have to be applied when entering to the
>> hypervisor from the guest context.
>>
>> Because the attack is against branch predictor, it is not possible to
>> safely use branch instruction before the mitigation is applied.
>> Therefore this has to be done in the vector entry before jump to the
>> helper handling a given exception.
>>
>> However, on arm32, each vector contain a single instruction. This means
>> that the hardened vector tables may rely on the state of registers that
>> does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
>> Therefore hypervisor code running with guest vectors table should be
>> minimized and always have interrupts masked to reduce the risk to use
>> them.
>>
>> This patch provides an infrastructure to switch vector tables before
>> entering to the guest and when leaving it.
>>
>> Note that alternative could have been used, but older Xen (4.8 or
>> earlier) doesn't have support. So avoid using alternative to ease
>> backporting.
>>
>> This is part of XSA-254.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> I only have a couple of questions. It looks good.
>
>
>> ---
>> xen/arch/arm/Kconfig | 3 +++
>> xen/arch/arm/arm32/entry.S | 41 ++++++++++++++++++++++++++++++++++++++++-
>> xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
>> 3 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 06fd85cc77..2782ee6589 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
>> config ARM64_HARDEN_BRANCH_PREDICTOR
>> def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
>>
>> +config ARM32_HARDEN_BRANCH_PREDICTOR
>> + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
>> +
>> source "common/Kconfig"
>>
>> source "drivers/Kconfig"
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index c2fad5fe9b..54a1733f87 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -34,6 +34,20 @@
>> blne save_guest_regs
>>
>> save_guest_regs:
>> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>> + /*
>> + * Restore vectors table to the default as it may have been
>> + * changed when returning to the guest (see
>> + * return_to_hypervisor). We need to do that early (e.g before
>> + * any interrupts are unmasked) because hardened vectors requires
>> + * SP to be 8 bytes aligned. This does not hold when running in
>> + * the hypervisor.
>> + */
>> + ldr r1, =hyp_traps_vector
>> + mcr p15, 4, r1, c12, c0, 0
>> + isb
>> +#endif
>> +
>> ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
>> str r11, [sp, #UREGS_sp]
>> SAVE_ONE_BANKED(SP_usr)
>> @@ -179,12 +193,37 @@ return_to_guest:
>> RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
>> /* Fall thru */
>> return_to_hypervisor:
>> - cpsid i
>> + cpsid ai
>
> Why?
Asynchronous abort is a kind of interrupt, as we are going to switch to
the hardened vector tables you don't want an interrupt to come up.
This is because the hardened vector tables requires SP to be 8 bytes
aligned. When in the hypervisor, and particularly when restoring the
register (as below), this assumption does not hold.
So masking all interrupts (as mentioned a few time within the patch and
the commit message) will reduce the risk to use the hardened vectors. I
say reduce because you may have receive data abort (imagine an unlikely
error in the few instructions to restore state).
It is also why switching vector tables are done very early in entry path
and very late in the exit path.
>
>
>> ldr lr, [sp, #UREGS_lr]
>> ldr r11, [sp, #UREGS_pc]
>> msr ELR_hyp, r11
>> ldr r11, [sp, #UREGS_cpsr]
>> msr SPSR_hyp, r11
>> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>> + /*
>> + * Hardening branch predictor may require to setup a different
>> + * vector tables before returning to the guests. Those vectors
>> + * may rely on the state of registers that does not hold when
>> + * running in the hypervisor (e.g SP is 8 bytes aligned). So setup
>> + * HVBAR very late.
>> + *
>> + * Default vectors table will be restored on exit (see
>> + * save_guest_regs).
>> + */
>> + mov r9, #0 /* vector tables = NULL */
>> + /*
>> + * Load vector tables pointer from the per-cpu bp_harden_vecs
>> + * when returning to the guest only.
>> + */
>> + and r11, #PSR_MODE_MASK
>> + cmp r11, #PSR_MODE_HYP
>> + ldrne r11, =per_cpu__bp_harden_vecs
>> + mrcne p15, 4, r10, c13, c0, 2 /* r10 = per-cpu offset (HTPIDR) */
>> + addne r11, r11, r10 /* r11 = offset of the vector tables */
>> + ldrne r9, [r11] /* r9 = vector tables */
>> + cmp r9, #0 /* Only update HVBAR when the vector */
>> + mcrne p15, 4, r9, c12, c0, 0 /* tables is not NULL. */
>
> Do we need/want an isb here? Or maybe it is not necessary because we are
> about to eret?
The eret is a context synchronization barrier. HVBAR may not be updated
until the eret, but we don't much care as it the hardened vector tables
only matter when running in guest context.
>
>
>> +#endif
>> pop {r0-r12}
>> add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
>> clrex
>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
>> index f1ea7f3c5b..0a138fa735 100644
>> --- a/xen/arch/arm/cpuerrata.c
>> +++ b/xen/arch/arm/cpuerrata.c
>> @@ -170,6 +170,36 @@ static int enable_psci_bp_hardening(void *data)
>>
>> #endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
>>
>> +/* Hardening Branch predictor code for Arm32 */
>> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>> +
>> +/*
>> + * Per-CPU vector tables to use when returning to the guests. They will
>> + * only be used on platform requiring to harden the branch predictor.
>> + */
>> +DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
>> +
>> +extern char hyp_traps_vector_bp_inv[];
>> +
>> +static void __maybe_unused
>> +install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
>> + const char *hyp_vecs, const char *desc)
>> +{
>> + /*
>> + * Enable callbacks are called on every CPU based on the
>> + * capabilities. So double-check whether the CPU matches the
>> + * entry.
>> + */
>> + if ( !entry->matches(entry) )
>> + return;
>> +
>> + printk(XENLOG_INFO "CPU%u will %s on guest exit\n",
>> + smp_processor_id(), desc);
>> + this_cpu(bp_harden_vecs) = hyp_vecs;
>> +}
>> +
>> +#endif
>> +
>> #define MIDR_RANGE(model, min, max) \
>> .matches = is_affected_midr_range, \
>> .midr_model = model, \
>> --
>> 2.11.0
>>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-25 11:36 ` Julien Grall
@ 2018-01-25 18:45 ` Stefano Stabellini
2018-01-25 18:50 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 18:45 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, andre.przywara, xen-devel
On Thu, 25 Jan 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 24/01/18 23:54, Stefano Stabellini wrote:
> > On Fri, 19 Jan 2018, Julien Grall wrote:
> > > Aliasing attacked against CPU branch predictors can allow an attacker to
> > > redirect speculative control flow on some CPUs and potentially divulge
> > > information from one context to another.
> > >
> > > This patch adds initiatial skeleton code behind a new Kconfig option
> > > to enable implementation-specific mitigations against these attacks
> > > for CPUs that are affected.
> > >
> > > Most of mitigations will have to be applied when entering to the
> > > hypervisor from the guest context.
> > >
> > > Because the attack is against branch predictor, it is not possible to
> > > safely use branch instruction before the mitigation is applied.
> > > Therefore this has to be done in the vector entry before jump to the
> > > helper handling a given exception.
> > >
> > > However, on arm32, each vector contain a single instruction. This means
> > > that the hardened vector tables may rely on the state of registers that
> > > does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
> > > Therefore hypervisor code running with guest vectors table should be
> > > minimized and always have interrupts masked to reduce the risk to use
> > > them.
> > >
> > > This patch provides an infrastructure to switch vector tables before
> > > entering to the guest and when leaving it.
> > >
> > > Note that alternative could have been used, but older Xen (4.8 or
> > > earlier) doesn't have support. So avoid using alternative to ease
> > > backporting.
> > >
> > > This is part of XSA-254.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >
> > I only have a couple of questions. It looks good.
> >
> >
> > > ---
> > > xen/arch/arm/Kconfig | 3 +++
> > > xen/arch/arm/arm32/entry.S | 41
> > > ++++++++++++++++++++++++++++++++++++++++-
> > > xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
> > > 3 files changed, 73 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > index 06fd85cc77..2782ee6589 100644
> > > --- a/xen/arch/arm/Kconfig
> > > +++ b/xen/arch/arm/Kconfig
> > > @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
> > > config ARM64_HARDEN_BRANCH_PREDICTOR
> > > def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
> > > +config ARM32_HARDEN_BRANCH_PREDICTOR
> > > + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
> > > +
> > > source "common/Kconfig"
> > > source "drivers/Kconfig"
> > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > > index c2fad5fe9b..54a1733f87 100644
> > > --- a/xen/arch/arm/arm32/entry.S
> > > +++ b/xen/arch/arm/arm32/entry.S
> > > @@ -34,6 +34,20 @@
> > > blne save_guest_regs
> > > save_guest_regs:
> > > +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> > > + /*
> > > + * Restore vectors table to the default as it may have been
> > > + * changed when returning to the guest (see
> > > + * return_to_hypervisor). We need to do that early (e.g before
> > > + * any interrupts are unmasked) because hardened vectors requires
> > > + * SP to be 8 bytes aligned. This does not hold when running in
> > > + * the hypervisor.
> > > + */
> > > + ldr r1, =hyp_traps_vector
> > > + mcr p15, 4, r1, c12, c0, 0
> > > + isb
> > > +#endif
> > > +
> > > ldr r11, =0xffffffff /* Clobber SP which is only valid for
> > > hypervisor frames. */
> > > str r11, [sp, #UREGS_sp]
> > > SAVE_ONE_BANKED(SP_usr)
> > > @@ -179,12 +193,37 @@ return_to_guest:
> > > RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
> > > /* Fall thru */
> > > return_to_hypervisor:
> > > - cpsid i
> > > + cpsid ai
> >
> > Why?
>
> Asynchronous abort is a kind of interrupt, as we are going to switch to the
> hardened vector tables you don't want an interrupt to come up.
>
> This is because the hardened vector tables requires SP to be 8 bytes aligned.
> When in the hypervisor, and particularly when restoring the register (as
> below), this assumption does not hold.
>
> So masking all interrupts (as mentioned a few time within the patch and the
> commit message) will reduce the risk to use the hardened vectors. I say reduce
> because you may have receive data abort (imagine an unlikely error in the few
> instructions to restore state).
>
> It is also why switching vector tables are done very early in entry path and
> very late in the exit path.
All right, thanks for the explanation. Please add "and mask asynchronous
aborts" in the commit message.
> >
> >
> > > ldr lr, [sp, #UREGS_lr]
> > > ldr r11, [sp, #UREGS_pc]
> > > msr ELR_hyp, r11
> > > ldr r11, [sp, #UREGS_cpsr]
> > > msr SPSR_hyp, r11
> > > +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> > > + /*
> > > + * Hardening branch predictor may require to setup a different
> > > + * vector tables before returning to the guests. Those vectors
> > > + * may rely on the state of registers that does not hold when
> > > + * running in the hypervisor (e.g SP is 8 bytes aligned). So
> > > setup
> > > + * HVBAR very late.
> > > + *
> > > + * Default vectors table will be restored on exit (see
> > > + * save_guest_regs).
> > > + */
> > > + mov r9, #0 /* vector tables = NULL */
> > > + /*
> > > + * Load vector tables pointer from the per-cpu bp_harden_vecs
> > > + * when returning to the guest only.
> > > + */
> > > + and r11, #PSR_MODE_MASK
> > > + cmp r11, #PSR_MODE_HYP
> > > + ldrne r11, =per_cpu__bp_harden_vecs
> > > + mrcne p15, 4, r10, c13, c0, 2 /* r10 = per-cpu offset (HTPIDR)
> > > */
> > > + addne r11, r11, r10 /* r11 = offset of the vector
> > > tables */
> > > + ldrne r9, [r11] /* r9 = vector tables */
> > > + cmp r9, #0 /* Only update HVBAR when the
> > > vector */
> > > + mcrne p15, 4, r9, c12, c0, 0 /* tables is not NULL. */
> >
> > Do we need/want an isb here? Or maybe it is not necessary because we are
> > about to eret?
> The eret is a context synchronization barrier. HVBAR may not be updated until
> the eret, but we don't much care as it the hardened vector tables only matter
> when running in guest context.
OK, I understand. Please my
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >
> >
> > > +#endif
> > > pop {r0-r12}
> > > add sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */
> > > clrex
> > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> > > index f1ea7f3c5b..0a138fa735 100644
> > > --- a/xen/arch/arm/cpuerrata.c
> > > +++ b/xen/arch/arm/cpuerrata.c
> > > @@ -170,6 +170,36 @@ static int enable_psci_bp_hardening(void *data)
> > > #endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
> > > +/* Hardening Branch predictor code for Arm32 */
> > > +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> > > +
> > > +/*
> > > + * Per-CPU vector tables to use when returning to the guests. They will
> > > + * only be used on platform requiring to harden the branch predictor.
> > > + */
> > > +DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
> > > +
> > > +extern char hyp_traps_vector_bp_inv[];
> > > +
> > > +static void __maybe_unused
> > > +install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
> > > + const char *hyp_vecs, const char *desc)
> > > +{
> > > + /*
> > > + * Enable callbacks are called on every CPU based on the
> > > + * capabilities. So double-check whether the CPU matches the
> > > + * entry.
> > > + */
> > > + if ( !entry->matches(entry) )
> > > + return;
> > > +
> > > + printk(XENLOG_INFO "CPU%u will %s on guest exit\n",
> > > + smp_processor_id(), desc);
> > > + this_cpu(bp_harden_vecs) = hyp_vecs;
> > > +}
> > > +
> > > +#endif
> > > +
> > > #define MIDR_RANGE(model, min, max) \
> > > .matches = is_affected_midr_range, \
> > > .midr_model = model, \
> > > --
> > > 2.11.0
> > >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-25 18:45 ` Stefano Stabellini
@ 2018-01-25 18:50 ` Julien Grall
2018-01-25 19:37 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-25 18:50 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi,
On 25/01/18 18:45, Stefano Stabellini wrote:
> On Thu, 25 Jan 2018, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 24/01/18 23:54, Stefano Stabellini wrote:
>>> On Fri, 19 Jan 2018, Julien Grall wrote:
>>>> Aliasing attacked against CPU branch predictors can allow an attacker to
>>>> redirect speculative control flow on some CPUs and potentially divulge
>>>> information from one context to another.
>>>>
>>>> This patch adds initiatial skeleton code behind a new Kconfig option
>>>> to enable implementation-specific mitigations against these attacks
>>>> for CPUs that are affected.
>>>>
>>>> Most of mitigations will have to be applied when entering to the
>>>> hypervisor from the guest context.
>>>>
>>>> Because the attack is against branch predictor, it is not possible to
>>>> safely use branch instruction before the mitigation is applied.
>>>> Therefore this has to be done in the vector entry before jump to the
>>>> helper handling a given exception.
>>>>
>>>> However, on arm32, each vector contain a single instruction. This means
>>>> that the hardened vector tables may rely on the state of registers that
>>>> does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
>>>> Therefore hypervisor code running with guest vectors table should be
>>>> minimized and always have interrupts masked to reduce the risk to use
>>>> them.
>>>>
>>>> This patch provides an infrastructure to switch vector tables before
>>>> entering to the guest and when leaving it.
>>>>
>>>> Note that alternative could have been used, but older Xen (4.8 or
>>>> earlier) doesn't have support. So avoid using alternative to ease
>>>> backporting.
>>>>
>>>> This is part of XSA-254.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> I only have a couple of questions. It looks good.
>>>
>>>
>>>> ---
>>>> xen/arch/arm/Kconfig | 3 +++
>>>> xen/arch/arm/arm32/entry.S | 41
>>>> ++++++++++++++++++++++++++++++++++++++++-
>>>> xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
>>>> 3 files changed, 73 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>> index 06fd85cc77..2782ee6589 100644
>>>> --- a/xen/arch/arm/Kconfig
>>>> +++ b/xen/arch/arm/Kconfig
>>>> @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
>>>> config ARM64_HARDEN_BRANCH_PREDICTOR
>>>> def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
>>>> +config ARM32_HARDEN_BRANCH_PREDICTOR
>>>> + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
>>>> +
>>>> source "common/Kconfig"
>>>> source "drivers/Kconfig"
>>>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>>>> index c2fad5fe9b..54a1733f87 100644
>>>> --- a/xen/arch/arm/arm32/entry.S
>>>> +++ b/xen/arch/arm/arm32/entry.S
>>>> @@ -34,6 +34,20 @@
>>>> blne save_guest_regs
>>>> save_guest_regs:
>>>> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>>>> + /*
>>>> + * Restore vectors table to the default as it may have been
>>>> + * changed when returning to the guest (see
>>>> + * return_to_hypervisor). We need to do that early (e.g before
>>>> + * any interrupts are unmasked) because hardened vectors requires
>>>> + * SP to be 8 bytes aligned. This does not hold when running in
>>>> + * the hypervisor.
>>>> + */
>>>> + ldr r1, =hyp_traps_vector
>>>> + mcr p15, 4, r1, c12, c0, 0
>>>> + isb
>>>> +#endif
>>>> +
>>>> ldr r11, =0xffffffff /* Clobber SP which is only valid for
>>>> hypervisor frames. */
>>>> str r11, [sp, #UREGS_sp]
>>>> SAVE_ONE_BANKED(SP_usr)
>>>> @@ -179,12 +193,37 @@ return_to_guest:
>>>> RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
>>>> /* Fall thru */
>>>> return_to_hypervisor:
>>>> - cpsid i
>>>> + cpsid ai
>>>
>>> Why?
>>
>> Asynchronous abort is a kind of interrupt, as we are going to switch to the
>> hardened vector tables you don't want an interrupt to come up.
>>
>> This is because the hardened vector tables requires SP to be 8 bytes aligned.
>> When in the hypervisor, and particularly when restoring the register (as
>> below), this assumption does not hold.
>>
>> So masking all interrupts (as mentioned a few time within the patch and the
>> commit message) will reduce the risk to use the hardened vectors. I say reduce
>> because you may have receive data abort (imagine an unlikely error in the few
>> instructions to restore state).
>>
>> It is also why switching vector tables are done very early in entry path and
>> very late in the exit path.
>
> All right, thanks for the explanation. Please add "and mask asynchronous
> aborts" in the commit message.
I am not surely what you exactly suggest here. The commit message
currently contains:
"Therefore hypervisor code running with guest vectors table should be
minimized and always have interrupts masked to reduce the risk to use
them."
What are you suggesting?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-25 18:50 ` Julien Grall
@ 2018-01-25 19:37 ` Stefano Stabellini
2018-01-26 16:21 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 19:37 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, andre.przywara, xen-devel
On Thu, 25 Jan 2018, Julien Grall wrote:
> Hi,
>
> On 25/01/18 18:45, Stefano Stabellini wrote:
> > On Thu, 25 Jan 2018, Julien Grall wrote:
> > > Hi Stefano,
> > >
> > > On 24/01/18 23:54, Stefano Stabellini wrote:
> > > > On Fri, 19 Jan 2018, Julien Grall wrote:
> > > > > Aliasing attacked against CPU branch predictors can allow an attacker
> > > > > to
> > > > > redirect speculative control flow on some CPUs and potentially divulge
> > > > > information from one context to another.
> > > > >
> > > > > This patch adds initiatial skeleton code behind a new Kconfig option
> > > > > to enable implementation-specific mitigations against these attacks
> > > > > for CPUs that are affected.
> > > > >
> > > > > Most of mitigations will have to be applied when entering to the
> > > > > hypervisor from the guest context.
> > > > >
> > > > > Because the attack is against branch predictor, it is not possible to
> > > > > safely use branch instruction before the mitigation is applied.
> > > > > Therefore this has to be done in the vector entry before jump to the
> > > > > helper handling a given exception.
> > > > >
> > > > > However, on arm32, each vector contain a single instruction. This
> > > > > means
> > > > > that the hardened vector tables may rely on the state of registers
> > > > > that
> > > > > does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
> > > > > Therefore hypervisor code running with guest vectors table should be
> > > > > minimized and always have interrupts masked to reduce the risk to use
> > > > > them.
> > > > >
> > > > > This patch provides an infrastructure to switch vector tables before
> > > > > entering to the guest and when leaving it.
> > > > >
> > > > > Note that alternative could have been used, but older Xen (4.8 or
> > > > > earlier) doesn't have support. So avoid using alternative to ease
> > > > > backporting.
> > > > >
> > > > > This is part of XSA-254.
> > > > >
> > > > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > > >
> > > > I only have a couple of questions. It looks good.
> > > >
> > > >
> > > > > ---
> > > > > xen/arch/arm/Kconfig | 3 +++
> > > > > xen/arch/arm/arm32/entry.S | 41
> > > > > ++++++++++++++++++++++++++++++++++++++++-
> > > > > xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
> > > > > 3 files changed, 73 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> > > > > index 06fd85cc77..2782ee6589 100644
> > > > > --- a/xen/arch/arm/Kconfig
> > > > > +++ b/xen/arch/arm/Kconfig
> > > > > @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
> > > > > config ARM64_HARDEN_BRANCH_PREDICTOR
> > > > > def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
> > > > > +config ARM32_HARDEN_BRANCH_PREDICTOR
> > > > > + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
> > > > > +
> > > > > source "common/Kconfig"
> > > > > source "drivers/Kconfig"
> > > > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > > > > index c2fad5fe9b..54a1733f87 100644
> > > > > --- a/xen/arch/arm/arm32/entry.S
> > > > > +++ b/xen/arch/arm/arm32/entry.S
> > > > > @@ -34,6 +34,20 @@
> > > > > blne save_guest_regs
> > > > > save_guest_regs:
> > > > > +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> > > > > + /*
> > > > > + * Restore vectors table to the default as it may have been
> > > > > + * changed when returning to the guest (see
> > > > > + * return_to_hypervisor). We need to do that early (e.g
> > > > > before
> > > > > + * any interrupts are unmasked) because hardened vectors
> > > > > requires
> > > > > + * SP to be 8 bytes aligned. This does not hold when running
> > > > > in
> > > > > + * the hypervisor.
> > > > > + */
> > > > > + ldr r1, =hyp_traps_vector
> > > > > + mcr p15, 4, r1, c12, c0, 0
> > > > > + isb
> > > > > +#endif
> > > > > +
> > > > > ldr r11, =0xffffffff /* Clobber SP which is only valid for
> > > > > hypervisor frames. */
> > > > > str r11, [sp, #UREGS_sp]
> > > > > SAVE_ONE_BANKED(SP_usr)
> > > > > @@ -179,12 +193,37 @@ return_to_guest:
> > > > > RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
> > > > > /* Fall thru */
> > > > > return_to_hypervisor:
> > > > > - cpsid i
> > > > > + cpsid ai
> > > >
> > > > Why?
> > >
> > > Asynchronous abort is a kind of interrupt, as we are going to switch to
> > > the
> > > hardened vector tables you don't want an interrupt to come up.
> > >
> > > This is because the hardened vector tables requires SP to be 8 bytes
> > > aligned.
> > > When in the hypervisor, and particularly when restoring the register (as
> > > below), this assumption does not hold.
> > >
> > > So masking all interrupts (as mentioned a few time within the patch and
> > > the
> > > commit message) will reduce the risk to use the hardened vectors. I say
> > > reduce
> > > because you may have receive data abort (imagine an unlikely error in the
> > > few
> > > instructions to restore state).
> > >
> > > It is also why switching vector tables are done very early in entry path
> > > and
> > > very late in the exit path.
> >
> > All right, thanks for the explanation. Please add "and mask asynchronous
> > aborts" in the commit message.
>
> I am not surely what you exactly suggest here. The commit message currently
> contains:
>
> "Therefore hypervisor code running with guest vectors table should be
> minimized and always have interrupts masked to reduce the risk to use
> them."
>
> What are you suggesting?
"Therefore hypervisor code running with guest vectors table should be
minimized and always have interrupts and async aborts masked to reduce
the risk to use them."
Do you think that it is clearer?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-25 19:37 ` Stefano Stabellini
@ 2018-01-26 16:21 ` Julien Grall
2018-01-31 15:29 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-26 16:21 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi Stefano,
On 25/01/18 19:37, Stefano Stabellini wrote:
> On Thu, 25 Jan 2018, Julien Grall wrote:
>> Hi,
>>
>> On 25/01/18 18:45, Stefano Stabellini wrote:
>>> On Thu, 25 Jan 2018, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 24/01/18 23:54, Stefano Stabellini wrote:
>>>>> On Fri, 19 Jan 2018, Julien Grall wrote:
>>>>>> Aliasing attacked against CPU branch predictors can allow an attacker
>>>>>> to
>>>>>> redirect speculative control flow on some CPUs and potentially divulge
>>>>>> information from one context to another.
>>>>>>
>>>>>> This patch adds initiatial skeleton code behind a new Kconfig option
>>>>>> to enable implementation-specific mitigations against these attacks
>>>>>> for CPUs that are affected.
>>>>>>
>>>>>> Most of mitigations will have to be applied when entering to the
>>>>>> hypervisor from the guest context.
>>>>>>
>>>>>> Because the attack is against branch predictor, it is not possible to
>>>>>> safely use branch instruction before the mitigation is applied.
>>>>>> Therefore this has to be done in the vector entry before jump to the
>>>>>> helper handling a given exception.
>>>>>>
>>>>>> However, on arm32, each vector contain a single instruction. This
>>>>>> means
>>>>>> that the hardened vector tables may rely on the state of registers
>>>>>> that
>>>>>> does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
>>>>>> Therefore hypervisor code running with guest vectors table should be
>>>>>> minimized and always have interrupts masked to reduce the risk to use
>>>>>> them.
>>>>>>
>>>>>> This patch provides an infrastructure to switch vector tables before
>>>>>> entering to the guest and when leaving it.
>>>>>>
>>>>>> Note that alternative could have been used, but older Xen (4.8 or
>>>>>> earlier) doesn't have support. So avoid using alternative to ease
>>>>>> backporting.
>>>>>>
>>>>>> This is part of XSA-254.
>>>>>>
>>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>>>
>>>>> I only have a couple of questions. It looks good.
>>>>>
>>>>>
>>>>>> ---
>>>>>> xen/arch/arm/Kconfig | 3 +++
>>>>>> xen/arch/arm/arm32/entry.S | 41
>>>>>> ++++++++++++++++++++++++++++++++++++++++-
>>>>>> xen/arch/arm/cpuerrata.c | 30 ++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 73 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>>>>>> index 06fd85cc77..2782ee6589 100644
>>>>>> --- a/xen/arch/arm/Kconfig
>>>>>> +++ b/xen/arch/arm/Kconfig
>>>>>> @@ -191,6 +191,9 @@ config HARDEN_BRANCH_PREDICTOR
>>>>>> config ARM64_HARDEN_BRANCH_PREDICTOR
>>>>>> def_bool y if ARM_64 && HARDEN_BRANCH_PREDICTOR
>>>>>> +config ARM32_HARDEN_BRANCH_PREDICTOR
>>>>>> + def_bool y if ARM_32 && HARDEN_BRANCH_PREDICTOR
>>>>>> +
>>>>>> source "common/Kconfig"
>>>>>> source "drivers/Kconfig"
>>>>>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>>>>>> index c2fad5fe9b..54a1733f87 100644
>>>>>> --- a/xen/arch/arm/arm32/entry.S
>>>>>> +++ b/xen/arch/arm/arm32/entry.S
>>>>>> @@ -34,6 +34,20 @@
>>>>>> blne save_guest_regs
>>>>>> save_guest_regs:
>>>>>> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
>>>>>> + /*
>>>>>> + * Restore vectors table to the default as it may have been
>>>>>> + * changed when returning to the guest (see
>>>>>> + * return_to_hypervisor). We need to do that early (e.g
>>>>>> before
>>>>>> + * any interrupts are unmasked) because hardened vectors
>>>>>> requires
>>>>>> + * SP to be 8 bytes aligned. This does not hold when running
>>>>>> in
>>>>>> + * the hypervisor.
>>>>>> + */
>>>>>> + ldr r1, =hyp_traps_vector
>>>>>> + mcr p15, 4, r1, c12, c0, 0
>>>>>> + isb
>>>>>> +#endif
>>>>>> +
>>>>>> ldr r11, =0xffffffff /* Clobber SP which is only valid for
>>>>>> hypervisor frames. */
>>>>>> str r11, [sp, #UREGS_sp]
>>>>>> SAVE_ONE_BANKED(SP_usr)
>>>>>> @@ -179,12 +193,37 @@ return_to_guest:
>>>>>> RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
>>>>>> /* Fall thru */
>>>>>> return_to_hypervisor:
>>>>>> - cpsid i
>>>>>> + cpsid ai
>>>>>
>>>>> Why?
>>>>
>>>> Asynchronous abort is a kind of interrupt, as we are going to switch to
>>>> the
>>>> hardened vector tables you don't want an interrupt to come up.
>>>>
>>>> This is because the hardened vector tables requires SP to be 8 bytes
>>>> aligned.
>>>> When in the hypervisor, and particularly when restoring the register (as
>>>> below), this assumption does not hold.
>>>>
>>>> So masking all interrupts (as mentioned a few time within the patch and
>>>> the
>>>> commit message) will reduce the risk to use the hardened vectors. I say
>>>> reduce
>>>> because you may have receive data abort (imagine an unlikely error in the
>>>> few
>>>> instructions to restore state).
>>>>
>>>> It is also why switching vector tables are done very early in entry path
>>>> and
>>>> very late in the exit path.
>>>
>>> All right, thanks for the explanation. Please add "and mask asynchronous
>>> aborts" in the commit message.
>>
>> I am not surely what you exactly suggest here. The commit message currently
>> contains:
>>
>> "Therefore hypervisor code running with guest vectors table should be
>> minimized and always have interrupts masked to reduce the risk to use
>> them."
>>
>> What are you suggesting?
>
> "Therefore hypervisor code running with guest vectors table should be
> minimized and always have interrupts and async aborts masked to reduce
> the risk to use them."
>
> Do you think that it is clearer?
Well, that was covered by "interrupts". If you look at the Arm Arm, A,
I, F are considered all interrupts.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-26 16:21 ` Julien Grall
@ 2018-01-31 15:29 ` Julien Grall
2018-01-31 16:40 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-31 15:29 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
On 26/01/18 16:21, Julien Grall wrote:
>> "Therefore hypervisor code running with guest vectors table should be
>> minimized and always have interrupts and async aborts masked to reduce
>> the risk to use them."
>>
>> Do you think that it is clearer?
>
> Well, that was covered by "interrupts". If you look at the Arm Arm, A,
> I, F are considered all interrupts.
I reworked the paragraph and it is now:
"However, on arm32, each vector contain a single instruction. This means
that the hardened vector tables may rely on the state of registers that
does not hold when in the hypervisor (e.g SP is 8 bytes aligned).
Therefore hypervisor code running with guest vectors table should be
minimized and always have IRQ and SError masked to reduce the risk to
use them."
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks
2018-01-31 15:29 ` Julien Grall
@ 2018-01-31 16:40 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-31 16:40 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, andre.przywara, xen-devel
On Wed, 31 Jan 2018, Julien Grall wrote:
> On 26/01/18 16:21, Julien Grall wrote:
> > > "Therefore hypervisor code running with guest vectors table should be
> > > minimized and always have interrupts and async aborts masked to reduce
> > > the risk to use them."
> > >
> > > Do you think that it is clearer?
> >
> > Well, that was covered by "interrupts". If you look at the Arm Arm, A, I, F
> > are considered all interrupts.
>
> I reworked the paragraph and it is now:
>
> "However, on arm32, each vector contain a single instruction. This means that
> the hardened vector tables may rely on the state of registers that does not
> hold when in the hypervisor (e.g SP is 8 bytes aligned). Therefore hypervisor
> code running with guest vectors table should be
> minimized and always have IRQ and SError masked to reduce the risk to use
> them."
I think it's much better, thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
` (3 preceding siblings ...)
2018-01-19 13:41 ` [PATCH 4/7] xen/arm32: Add skeleton to harden branch predictor aliasing attacks Julien Grall
@ 2018-01-19 13:41 ` Julien Grall
2018-01-24 22:22 ` Konrad Rzeszutek Wilk
2018-01-25 1:02 ` Stefano Stabellini
2018-01-19 13:41 ` [PATCH 6/7] xen/arm32: Invalidate icache on guest exist for Cortex-A15 Julien Grall
2018-01-19 13:41 ` [PATCH 7/7] xen/arm32: entry: Document the purpose of r11 in the traps handler Julien Grall
6 siblings, 2 replies; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:41 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
In order to avoid aliasing attackes agains the branch predictor, let's
invalidate the BTB on guest exist. This is made complicated by the fact
that we cannot take a branch invalidating the BTB.
This is based on the first version posrted by Marc Zyngier on Linux-arm
mailing list (see [1]).
This is part of XSA-254.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@linaro.org>
[1] https://www.spinics.net/lists/arm-kernel/msg627032.html
---
xen/arch/arm/arm32/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
2 files changed, 74 insertions(+)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 54a1733f87..c6ec0aa399 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
b trap_irq /* 0x18 - IRQ */
b trap_fiq /* 0x1c - FIQ */
+ .align 5
+GLOBAL(hyp_traps_vector_bp_inv)
+ /*
+ * We encode the exception entry in the bottom 3 bits of
+ * SP, and we have to guarantee to be 8 bytes aligned.
+ */
+ add sp, sp, #1 /* Reset 7 */
+ add sp, sp, #1 /* Undef 6 */
+ add sp, sp, #1 /* Hypervisor Call 5 */
+ add sp, sp, #1 /* Prefetch abort 4 */
+ add sp, sp, #1 /* Data abort 3 */
+ add sp, sp, #1 /* Hypervisor 2 */
+ add sp, sp, #1 /* IRQ 1 */
+ nop /* FIQ 0 */
+
+ mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
+ isb
+
+ /*
+ * As we cannot use any temporary registers and cannot
+ * clobber SP, we can decode the exception entry using
+ * an unrolled binary search.
+ */
+ tst sp, #4
+ bne 1f
+
+ tst sp, #2
+ bne 3f
+
+ tst sp, #1
+ bic sp, sp, #0x7
+ bne trap_irq
+ b trap_fiq
+
+1:
+ tst sp, #2
+ bne 2f
+
+ tst sp, #1
+ bic sp, sp, #0x7
+ bne trap_hypervisor_call
+ b trap_prefetch_abort
+
+2:
+ tst sp, #1
+ bic sp, sp, #0x7
+ bne trap_reset
+ b trap_undefined_instruction
+
+3:
+ tst sp, #1
+ bic sp, sp, #0x7
+ bne trap_data_abort
+ b trap_guest_sync
+
DEFINE_TRAP_ENTRY(reset)
DEFINE_TRAP_ENTRY(undefined_instruction)
DEFINE_TRAP_ENTRY(hypervisor_call)
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 0a138fa735..c79e6d65d3 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -198,6 +198,13 @@ install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
this_cpu(bp_harden_vecs) = hyp_vecs;
}
+static int enable_bp_inv_hardening(void *data)
+{
+ install_bp_hardening_vecs(data, hyp_traps_vector_bp_inv,
+ "execute BPIALL");
+ return 0;
+}
+
#endif
#define MIDR_RANGE(model, min, max) \
@@ -284,6 +291,18 @@ static const struct arm_cpu_capabilities arm_errata[] = {
.enable = enable_psci_bp_hardening,
},
#endif
+#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
+ {
+ .capability = ARM_HARDEN_BRANCH_PREDICTOR,
+ MIDR_ALL_VERSIONS(MIDR_CORTEX_A12),
+ .enable = enable_bp_inv_hardening,
+ },
+ {
+ .capability = ARM_HARDEN_BRANCH_PREDICTOR,
+ MIDR_ALL_VERSIONS(MIDR_CORTEX_A17),
+ .enable = enable_bp_inv_hardening,
+ },
+#endif
{},
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-19 13:41 ` [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12 Julien Grall
@ 2018-01-24 22:22 ` Konrad Rzeszutek Wilk
2018-01-24 22:28 ` Julien Grall
2018-01-25 1:02 ` Stefano Stabellini
1 sibling, 1 reply; 29+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-01-24 22:22 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, Jan 19, 2018 at 01:41:01PM +0000, Julien Grall wrote:
> In order to avoid aliasing attackes agains the branch predictor, let's
> invalidate the BTB on guest exist. This is made complicated by the fact
> that we cannot take a branch invalidating the BTB.
>
> This is based on the first version posrted by Marc Zyngier on Linux-arm
> mailing list (see [1]).
>
> This is part of XSA-254.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> [1] https://www.spinics.net/lists/arm-kernel/msg627032.html
> ---
> xen/arch/arm/arm32/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 54a1733f87..c6ec0aa399 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
> b trap_irq /* 0x18 - IRQ */
> b trap_fiq /* 0x1c - FIQ */
>
> + .align 5
Not .align 8?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-24 22:22 ` Konrad Rzeszutek Wilk
@ 2018-01-24 22:28 ` Julien Grall
0 siblings, 0 replies; 29+ messages in thread
From: Julien Grall @ 2018-01-24 22:28 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: Stefano Stabellini, Andre Przywara, Xen-devel
On 24 January 2018 at 22:22, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Jan 19, 2018 at 01:41:01PM +0000, Julien Grall wrote:
>> In order to avoid aliasing attackes agains the branch predictor, let's
>> invalidate the BTB on guest exist. This is made complicated by the fact
>> that we cannot take a branch invalidating the BTB.
>>
>> This is based on the first version posrted by Marc Zyngier on Linux-arm
>> mailing list (see [1]).
>>
>> This is part of XSA-254.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg627032.html
>> ---
>> xen/arch/arm/arm32/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
>> 2 files changed, 74 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index 54a1733f87..c6ec0aa399 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
>> b trap_irq /* 0x18 - IRQ */
>> b trap_fiq /* 0x1c - FIQ */
>>
>> + .align 5
>
> Not .align 8?
.align 5 is following was we already have for the other vector table.
Per [1], this will make sure the base address of the table is a
multiple of 32 (one instruction per vector).
Cheers,
[1] https://ftp.gnu.org/old-gnu/Manuals/gas/html_chapter/as_7.html#SEC70
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-19 13:41 ` [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12 Julien Grall
2018-01-24 22:22 ` Konrad Rzeszutek Wilk
@ 2018-01-25 1:02 ` Stefano Stabellini
2018-01-25 11:50 ` Julien Grall
1 sibling, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 1:02 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> In order to avoid aliasing attackes agains the branch predictor, let's
> invalidate the BTB on guest exist. This is made complicated by the fact
> that we cannot take a branch invalidating the BTB.
>
> This is based on the first version posrted by Marc Zyngier on Linux-arm
> mailing list (see [1]).
>
> This is part of XSA-254.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> [1] https://www.spinics.net/lists/arm-kernel/msg627032.html
> ---
> xen/arch/arm/arm32/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index 54a1733f87..c6ec0aa399 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
> b trap_irq /* 0x18 - IRQ */
> b trap_fiq /* 0x1c - FIQ */
>
> + .align 5
> +GLOBAL(hyp_traps_vector_bp_inv)
> + /*
> + * We encode the exception entry in the bottom 3 bits of
> + * SP, and we have to guarantee to be 8 bytes aligned.
> + */
> + add sp, sp, #1 /* Reset 7 */
> + add sp, sp, #1 /* Undef 6 */
> + add sp, sp, #1 /* Hypervisor Call 5 */
> + add sp, sp, #1 /* Prefetch abort 4 */
> + add sp, sp, #1 /* Data abort 3 */
> + add sp, sp, #1 /* Hypervisor 2 */
> + add sp, sp, #1 /* IRQ 1 */
> + nop /* FIQ 0 */
Clever! Things that you don't read every day :-)
> + mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
> + isb
> +
> + /*
> + * As we cannot use any temporary registers and cannot
> + * clobber SP, we can decode the exception entry using
> + * an unrolled binary search.
> + */
> + tst sp, #4
> + bne 1f
> +
> + tst sp, #2
> + bne 3f
> +
> + tst sp, #1
> + bic sp, sp, #0x7
> + bne trap_irq
> + b trap_fiq
I might be confused, but this is the case where sp == 0x7, right?
Shouldn't we have b trap_reset here?
Similarly the branch just above corresponds to 0x6, which should be
bne trap_undefined_instruction.
What am I getting wrong?
> +1:
> + tst sp, #2
> + bne 2f
> +
> + tst sp, #1
> + bic sp, sp, #0x7
> + bne trap_hypervisor_call
> + b trap_prefetch_abort
> +
> +2:
> + tst sp, #1
> + bic sp, sp, #0x7
> + bne trap_reset
> + b trap_undefined_instruction
> +
> +3:
> + tst sp, #1
> + bic sp, sp, #0x7
> + bne trap_data_abort
> + b trap_guest_sync
> +
> DEFINE_TRAP_ENTRY(reset)
> DEFINE_TRAP_ENTRY(undefined_instruction)
> DEFINE_TRAP_ENTRY(hypervisor_call)
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 0a138fa735..c79e6d65d3 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -198,6 +198,13 @@ install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
> this_cpu(bp_harden_vecs) = hyp_vecs;
> }
>
> +static int enable_bp_inv_hardening(void *data)
> +{
> + install_bp_hardening_vecs(data, hyp_traps_vector_bp_inv,
> + "execute BPIALL");
> + return 0;
> +}
> +
> #endif
>
> #define MIDR_RANGE(model, min, max) \
> @@ -284,6 +291,18 @@ static const struct arm_cpu_capabilities arm_errata[] = {
> .enable = enable_psci_bp_hardening,
> },
> #endif
> +#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> + {
> + .capability = ARM_HARDEN_BRANCH_PREDICTOR,
> + MIDR_ALL_VERSIONS(MIDR_CORTEX_A12),
> + .enable = enable_bp_inv_hardening,
> + },
> + {
> + .capability = ARM_HARDEN_BRANCH_PREDICTOR,
> + MIDR_ALL_VERSIONS(MIDR_CORTEX_A17),
> + .enable = enable_bp_inv_hardening,
> + },
> +#endif
> {},
> };
>
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-25 1:02 ` Stefano Stabellini
@ 2018-01-25 11:50 ` Julien Grall
2018-01-25 19:35 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-25 11:50 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi Stefano,
On 25/01/18 01:02, Stefano Stabellini wrote:
> On Fri, 19 Jan 2018, Julien Grall wrote:
>> In order to avoid aliasing attackes agains the branch predictor, let's
>> invalidate the BTB on guest exist. This is made complicated by the fact
>> that we cannot take a branch invalidating the BTB.
>>
>> This is based on the first version posrted by Marc Zyngier on Linux-arm
>> mailing list (see [1]).
>>
>> This is part of XSA-254.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg627032.html
>> ---
>> xen/arch/arm/arm32/entry.S | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
>> 2 files changed, 74 insertions(+)
>>
>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
>> index 54a1733f87..c6ec0aa399 100644
>> --- a/xen/arch/arm/arm32/entry.S
>> +++ b/xen/arch/arm/arm32/entry.S
>> @@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
>> b trap_irq /* 0x18 - IRQ */
>> b trap_fiq /* 0x1c - FIQ */
>>
>> + .align 5
>> +GLOBAL(hyp_traps_vector_bp_inv)
>> + /*
>> + * We encode the exception entry in the bottom 3 bits of
>> + * SP, and we have to guarantee to be 8 bytes aligned.
>> + */
>> + add sp, sp, #1 /* Reset 7 */
>> + add sp, sp, #1 /* Undef 6 */
>> + add sp, sp, #1 /* Hypervisor Call 5 */
>> + add sp, sp, #1 /* Prefetch abort 4 */
>> + add sp, sp, #1 /* Data abort 3 */
>> + add sp, sp, #1 /* Hypervisor 2 */
>> + add sp, sp, #1 /* IRQ 1 */
>> + nop /* FIQ 0 */
>
> Clever! Things that you don't read every day :-)
Thanks Marc for the idea :).
>
>
>> + mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
>> + isb
>> +
>> + /*
>> + * As we cannot use any temporary registers and cannot
>> + * clobber SP, we can decode the exception entry using
>> + * an unrolled binary search.
>> + */
>> + tst sp, #4
>> + bne 1f
>> +
>> + tst sp, #2
>> + bne 3f
>> +
>> + tst sp, #1
>> + bic sp, sp, #0x7
>> + bne trap_irq
>> + b trap_fiq
>
> I might be confused, but this is the case where sp == 0x7, right?
> Shouldn't we have b trap_reset here?
>
> Similarly the branch just above corresponds to 0x6, which should be
> bne trap_undefined_instruction.
>
> What am I getting wrong?
The tst instruction performs a bitwise AND on a register value (here
sp). The result will be used to update the condition flags.
So with tst sp, #4 the result will either be 0x100 or 0x000. The former
will clear Z flag while the latter set Z flag.
This means that bne will branch only when bit 2 is set. So the only way
to end here is because the first 3-bit are equal to 0x00X. This
corresponds to IRQ/FIQ vector.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-25 11:50 ` Julien Grall
@ 2018-01-25 19:35 ` Stefano Stabellini
2018-01-31 15:31 ` Julien Grall
0 siblings, 1 reply; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 19:35 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, andre.przywara, xen-devel
On Thu, 25 Jan 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 25/01/18 01:02, Stefano Stabellini wrote:
> > On Fri, 19 Jan 2018, Julien Grall wrote:
> > > In order to avoid aliasing attackes agains the branch predictor, let's
> > > invalidate the BTB on guest exist. This is made complicated by the fact
> > > that we cannot take a branch invalidating the BTB.
> > >
> > > This is based on the first version posrted by Marc Zyngier on Linux-arm
> > > mailing list (see [1]).
> > >
> > > This is part of XSA-254.
> > >
> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > >
> > > [1] https://www.spinics.net/lists/arm-kernel/msg627032.html
> > > ---
> > > xen/arch/arm/arm32/entry.S | 55
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > xen/arch/arm/cpuerrata.c | 19 ++++++++++++++++
> > > 2 files changed, 74 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> > > index 54a1733f87..c6ec0aa399 100644
> > > --- a/xen/arch/arm/arm32/entry.S
> > > +++ b/xen/arch/arm/arm32/entry.S
> > > @@ -160,6 +160,61 @@ GLOBAL(hyp_traps_vector)
> > > b trap_irq /* 0x18 - IRQ */
> > > b trap_fiq /* 0x1c - FIQ */
> > > + .align 5
> > > +GLOBAL(hyp_traps_vector_bp_inv)
> > > + /*
> > > + * We encode the exception entry in the bottom 3 bits of
> > > + * SP, and we have to guarantee to be 8 bytes aligned.
> > > + */
> > > + add sp, sp, #1 /* Reset 7 */
> > > + add sp, sp, #1 /* Undef 6 */
> > > + add sp, sp, #1 /* Hypervisor Call 5 */
> > > + add sp, sp, #1 /* Prefetch abort 4 */
> > > + add sp, sp, #1 /* Data abort 3 */
> > > + add sp, sp, #1 /* Hypervisor 2 */
> > > + add sp, sp, #1 /* IRQ 1 */
> > > + nop /* FIQ 0 */
> >
> > Clever! Things that you don't read every day :-)
>
> Thanks Marc for the idea :).
>
> >
> >
> > > + mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
> > > + isb
> > > +
> > > + /*
> > > + * As we cannot use any temporary registers and cannot
> > > + * clobber SP, we can decode the exception entry using
> > > + * an unrolled binary search.
> > > + */
> > > + tst sp, #4
> > > + bne 1f
> > > +
> > > + tst sp, #2
> > > + bne 3f
> > > +
> > > + tst sp, #1
> > > + bic sp, sp, #0x7
> > > + bne trap_irq
> > > + b trap_fiq
> >
> > I might be confused, but this is the case where sp == 0x7, right?
> > Shouldn't we have b trap_reset here?
> >
> > Similarly the branch just above corresponds to 0x6, which should be
> > bne trap_undefined_instruction.
> >
> > What am I getting wrong?
>
> The tst instruction performs a bitwise AND on a register value (here sp). The
> result will be used to update the condition flags.
>
> So with tst sp, #4 the result will either be 0x100 or 0x000. The former will
> clear Z flag while the latter set Z flag.
>
> This means that bne will branch only when bit 2 is set. So the only way to end
> here is because the first 3-bit are equal to 0x00X. This corresponds to
> IRQ/FIQ vector.
I got it the other way around, thanks for the explanation :-)
Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-25 19:35 ` Stefano Stabellini
@ 2018-01-31 15:31 ` Julien Grall
2018-01-31 16:40 ` Stefano Stabellini
0 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-31 15:31 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: andre.przywara, xen-devel
Hi Stefano,
On 25/01/18 19:35, Stefano Stabellini wrote:
>> This means that bne will branch only when bit 2 is set. So the only way to end
>> here is because the first 3-bit are equal to 0x00X. This corresponds to
>> IRQ/FIQ vector.
>
> I got it the other way around, thanks for the explanation :-)
>
> Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
Did you mean Reviewed-by or Acked-by?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12
2018-01-31 15:31 ` Julien Grall
@ 2018-01-31 16:40 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-31 16:40 UTC (permalink / raw)
To: Julien Grall; +Cc: Stefano Stabellini, andre.przywara, xen-devel
On Wed, 31 Jan 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 25/01/18 19:35, Stefano Stabellini wrote:
> > > This means that bne will branch only when bit 2 is set. So the only way to
> > > end
> > > here is because the first 3-bit are equal to 0x00X. This corresponds to
> > > IRQ/FIQ vector.
> >
> > I got it the other way around, thanks for the explanation :-)
> >
> > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org>
>
> Did you mean Reviewed-by or Acked-by?
LOL! I meant Reviewed-by.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 6/7] xen/arm32: Invalidate icache on guest exist for Cortex-A15
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
` (4 preceding siblings ...)
2018-01-19 13:41 ` [PATCH 5/7] xen/arm32: Invalidate BTB on guest exit for Cortex A17 and 12 Julien Grall
@ 2018-01-19 13:41 ` Julien Grall
2018-01-25 1:08 ` Stefano Stabellini
2018-01-19 13:41 ` [PATCH 7/7] xen/arm32: entry: Document the purpose of r11 in the traps handler Julien Grall
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:41 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
In order to avoid aliasing attacks against the branch predictor on
Cortex A-15, let's invalidate the BTB on guest exit, which can only be
done by invalidating the icache (with ACTLR[0] being set).
We use the same hack as for A12/A17 to perform the vector decoding.
This is based on Linux patch from the kpti branch in [1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/arm32/entry.S | 21 +++++++++++++++++++++
xen/arch/arm/cpuerrata.c | 13 +++++++++++++
2 files changed, 34 insertions(+)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index c6ec0aa399..c529592d20 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -161,6 +161,26 @@ GLOBAL(hyp_traps_vector)
b trap_fiq /* 0x1c - FIQ */
.align 5
+GLOBAL(hyp_traps_vector_ic_inv)
+ /*
+ * We encode the exception entry in the bottom 3 bits of
+ * SP, and we have to guarantee to be 8 bytes aligned.
+ */
+ add sp, sp, #1 /* Reset 7 */
+ add sp, sp, #1 /* Undef 6 */
+ add sp, sp, #1 /* Hypervisor call 5 */
+ add sp, sp, #1 /* Prefetch abort 4 */
+ add sp, sp, #1 /* Data abort 3 */
+ add sp, sp, #1 /* Hypervisor 2 */
+ add sp, sp, #1 /* IRQ 1 */
+ nop /* FIQ 0 */
+
+ mcr p15, 0, r0, c7, c5, 0 /* ICIALLU */
+ isb
+
+ b decode_vectors
+
+ .align 5
GLOBAL(hyp_traps_vector_bp_inv)
/*
* We encode the exception entry in the bottom 3 bits of
@@ -178,6 +198,7 @@ GLOBAL(hyp_traps_vector_bp_inv)
mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
isb
+decode_vectors:
/*
* As we cannot use any temporary registers and cannot
* clobber SP, we can decode the exception entry using
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index c79e6d65d3..9c7458ef06 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -180,6 +180,7 @@ static int enable_psci_bp_hardening(void *data)
DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
extern char hyp_traps_vector_bp_inv[];
+extern char hyp_traps_vector_ic_inv[];
static void __maybe_unused
install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
@@ -205,6 +206,13 @@ static int enable_bp_inv_hardening(void *data)
return 0;
}
+static int enable_ic_inv_hardening(void *data)
+{
+ install_bp_hardening_vecs(data, hyp_traps_vector_ic_inv,
+ "execute ICIALLU");
+ return 0;
+}
+
#endif
#define MIDR_RANGE(model, min, max) \
@@ -302,6 +310,11 @@ static const struct arm_cpu_capabilities arm_errata[] = {
MIDR_ALL_VERSIONS(MIDR_CORTEX_A17),
.enable = enable_bp_inv_hardening,
},
+ {
+ .capability = ARM_HARDEN_BRANCH_PREDICTOR,
+ MIDR_ALL_VERSIONS(MIDR_CORTEX_A15),
+ .enable = enable_ic_inv_hardening,
+ },
#endif
{},
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 6/7] xen/arm32: Invalidate icache on guest exist for Cortex-A15
2018-01-19 13:41 ` [PATCH 6/7] xen/arm32: Invalidate icache on guest exist for Cortex-A15 Julien Grall
@ 2018-01-25 1:08 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 1:08 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> In order to avoid aliasing attacks against the branch predictor on
> Cortex A-15, let's invalidate the BTB on guest exit, which can only be
> done by invalidating the icache (with ACTLR[0] being set).
>
> We use the same hack as for A12/A17 to perform the vector decoding.
>
> This is based on Linux patch from the kpti branch in [1].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/arch/arm/arm32/entry.S | 21 +++++++++++++++++++++
> xen/arch/arm/cpuerrata.c | 13 +++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index c6ec0aa399..c529592d20 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -161,6 +161,26 @@ GLOBAL(hyp_traps_vector)
> b trap_fiq /* 0x1c - FIQ */
>
> .align 5
> +GLOBAL(hyp_traps_vector_ic_inv)
> + /*
> + * We encode the exception entry in the bottom 3 bits of
> + * SP, and we have to guarantee to be 8 bytes aligned.
> + */
> + add sp, sp, #1 /* Reset 7 */
> + add sp, sp, #1 /* Undef 6 */
> + add sp, sp, #1 /* Hypervisor call 5 */
> + add sp, sp, #1 /* Prefetch abort 4 */
> + add sp, sp, #1 /* Data abort 3 */
> + add sp, sp, #1 /* Hypervisor 2 */
> + add sp, sp, #1 /* IRQ 1 */
> + nop /* FIQ 0 */
> +
> + mcr p15, 0, r0, c7, c5, 0 /* ICIALLU */
> + isb
> +
> + b decode_vectors
> +
> + .align 5
> GLOBAL(hyp_traps_vector_bp_inv)
> /*
> * We encode the exception entry in the bottom 3 bits of
> @@ -178,6 +198,7 @@ GLOBAL(hyp_traps_vector_bp_inv)
> mcr p15, 0, r0, c7, c5, 6 /* BPIALL */
> isb
>
> +decode_vectors:
> /*
> * As we cannot use any temporary registers and cannot
> * clobber SP, we can decode the exception entry using
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index c79e6d65d3..9c7458ef06 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -180,6 +180,7 @@ static int enable_psci_bp_hardening(void *data)
> DEFINE_PER_CPU_READ_MOSTLY(const char *, bp_harden_vecs);
>
> extern char hyp_traps_vector_bp_inv[];
> +extern char hyp_traps_vector_ic_inv[];
>
> static void __maybe_unused
> install_bp_hardening_vecs(const struct arm_cpu_capabilities *entry,
> @@ -205,6 +206,13 @@ static int enable_bp_inv_hardening(void *data)
> return 0;
> }
>
> +static int enable_ic_inv_hardening(void *data)
> +{
> + install_bp_hardening_vecs(data, hyp_traps_vector_ic_inv,
> + "execute ICIALLU");
> + return 0;
> +}
> +
> #endif
>
> #define MIDR_RANGE(model, min, max) \
> @@ -302,6 +310,11 @@ static const struct arm_cpu_capabilities arm_errata[] = {
> MIDR_ALL_VERSIONS(MIDR_CORTEX_A17),
> .enable = enable_bp_inv_hardening,
> },
> + {
> + .capability = ARM_HARDEN_BRANCH_PREDICTOR,
> + MIDR_ALL_VERSIONS(MIDR_CORTEX_A15),
> + .enable = enable_ic_inv_hardening,
> + },
> #endif
> {},
> };
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 7/7] xen/arm32: entry: Document the purpose of r11 in the traps handler
2018-01-19 13:40 [PATCH 0/7] xen/arm32: Branch predictor hardening (XSA-254 variant 2) Julien Grall
` (5 preceding siblings ...)
2018-01-19 13:41 ` [PATCH 6/7] xen/arm32: Invalidate icache on guest exist for Cortex-A15 Julien Grall
@ 2018-01-19 13:41 ` Julien Grall
2018-01-25 1:08 ` Stefano Stabellini
6 siblings, 1 reply; 29+ messages in thread
From: Julien Grall @ 2018-01-19 13:41 UTC (permalink / raw)
To: xen-devel; +Cc: sstabellini, andre.przywara
It took me a bit of time to understand why __DEFINE_TRAP_ENTRY is
storing the original stack pointer in r11. It is working in pair with
return_traps_entry where sp will be restored from r11.
This is fine because per the AAPCS r11 must be preserved by the
subroutine. So in return_from_trap, r11 will still contain the original
stack pointer.
Add some documentation in the code to point the 2 sides to each other.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
xen/arch/arm/arm32/entry.S | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index c529592d20..7f323de484 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -136,6 +136,10 @@ trap_##trap: \
cpsie iflags; \
adr lr, return_from_trap; \
mov r0, sp; \
+ /* \
+ * Save the stack pointer in r11. It will be restored after the \
+ * trap has been handled (see return_from_trap). \
+ */ \
mov r11, sp; \
bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
b do_trap_##trap
@@ -246,6 +250,10 @@ DEFINE_TRAP_ENTRY_NOIRQ(fiq)
DEFINE_TRAP_ENTRY_NOABORT(data_abort)
return_from_trap:
+ /*
+ * Restore the stack pointer from r11. It was saved on exception
+ * entry (see __DEFINE_TRAP_ENTRY).
+ */
mov sp, r11
ENTRY(return_to_new_vcpu32)
ldr r11, [sp, #UREGS_cpsr]
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH 7/7] xen/arm32: entry: Document the purpose of r11 in the traps handler
2018-01-19 13:41 ` [PATCH 7/7] xen/arm32: entry: Document the purpose of r11 in the traps handler Julien Grall
@ 2018-01-25 1:08 ` Stefano Stabellini
0 siblings, 0 replies; 29+ messages in thread
From: Stefano Stabellini @ 2018-01-25 1:08 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, andre.przywara, xen-devel
On Fri, 19 Jan 2018, Julien Grall wrote:
> It took me a bit of time to understand why __DEFINE_TRAP_ENTRY is
> storing the original stack pointer in r11. It is working in pair with
> return_traps_entry where sp will be restored from r11.
>
> This is fine because per the AAPCS r11 must be preserved by the
> subroutine. So in return_from_trap, r11 will still contain the original
> stack pointer.
>
> Add some documentation in the code to point the 2 sides to each other.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/arch/arm/arm32/entry.S | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index c529592d20..7f323de484 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -136,6 +136,10 @@ trap_##trap: \
> cpsie iflags; \
> adr lr, return_from_trap; \
> mov r0, sp; \
> + /* \
> + * Save the stack pointer in r11. It will be restored after the \
> + * trap has been handled (see return_from_trap). \
> + */ \
> mov r11, sp; \
> bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
> b do_trap_##trap
> @@ -246,6 +250,10 @@ DEFINE_TRAP_ENTRY_NOIRQ(fiq)
> DEFINE_TRAP_ENTRY_NOABORT(data_abort)
>
> return_from_trap:
> + /*
> + * Restore the stack pointer from r11. It was saved on exception
> + * entry (see __DEFINE_TRAP_ENTRY).
> + */
> mov sp, r11
> ENTRY(return_to_new_vcpu32)
> ldr r11, [sp, #UREGS_cpsr]
> --
> 2.11.0
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 29+ messages in thread