1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
|
From 098fe877967870ffda2dfd9629a5fd272f6aacdc Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Fri, 11 Oct 2019 17:49:28 +0100
Subject: [PATCH 3/4] xen/arm32: Don't blindly unmask interrupts on trap
without a change of level
Exception vectors will unmask interrupts regardless the state of them in
the interrupted context.
One of the consequences is IRQ will be unmasked when receiving an
undefined instruction exception (used by WARN*) from the hypervisor.
This could result to unexpected behavior such as deadlock (if a lock was
shared with interrupts).
In a nutshell, interrupts should only be unmasked when it is safe to do.
Xen only unmask IRQ and Abort interrupts, so the logic can stay simple.
As vectors exceptions may be shared between guest and hypervisor, we now
need to have a different policy for the interrupts.
On exception from hypervisor, each vector will select the list of
interrupts to inherit from the interrupted context. Any interrupts not
listed will be kept masked.
On exception from the guest, the Abort and IRQ will be unmasked
depending on the exact vector.
The interrupts will be kept unmasked when the vector cannot used by
either guest or hypervisor.
Note that each vector is not anymore preceded by ALIGN. This is fine
because the alignment is already bigger than what we need.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/arm32/entry.S | 138 +++++++++++++++++++++++++++++++++++----------
1 file changed, 109 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 150cbc0b4b..ec90cca093 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -4,6 +4,17 @@
#include <asm/alternative.h>
#include <public/xen.h>
+/*
+ * Short-hands to defined the interrupts (A, I, F)
+ *
+ * _ means the interrupt state will not change
+ * X means the state of interrupt X will change
+ *
+ * To be used with msr cpsr_* only
+ */
+#define IFLAGS_AIF PSR_ABT_MASK | PSR_IRQ_MASK | PSR_FIQ_MASK
+#define IFLAGS_A_F PSR_ABT_MASK | PSR_FIQ_MASK
+
#define SAVE_ONE_BANKED(reg) mrs r11, reg; str r11, [sp, #UREGS_##reg]
#define RESTORE_ONE_BANKED(reg) ldr r11, [sp, #UREGS_##reg]; msr reg, r11
@@ -106,10 +117,18 @@ skip_check:
mov pc, lr
/*
- * Macro to define trap entry. The iflags corresponds to the list of
- * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
+ * Macro to define a trap entry.
+ *
+ * @guest_iflags: Optional list of interrupts to unmask when
+ * entering from guest context. As this is used with cpsie,
+ * the letter (a, i, f) should be used.
+ *
+ * @hyp_iflags: Optional list of interrupts to inherit when
+ * entering from hypervisor context. Any interrupts not
+ * listed will be kept unchanged. As this is used with cpsr_*,
+ * IFLAGS_* short-hands should be used.
*/
- .macro vector trap, iflags
+ .macro vector trap, guest_iflags=n, hyp_iflags=0
/* Save registers in the stack */
sub sp, #(UREGS_SP_usr - UREGS_sp) /* SP, LR, SPSR, PC */
push {r0-r12} /* Save R0-R12 */
@@ -127,12 +146,39 @@ skip_check:
mrs r11, SPSR_hyp
str r11, [sp, #UREGS_cpsr]
- and r11, #PSR_MODE_MASK
- cmp r11, #PSR_MODE_HYP
- blne save_guest_regs
+ /*
+ * We need to distinguish whether we came from guest or
+ * hypervisor context.
+ */
+ and r0, r11, #PSR_MODE_MASK
+ cmp r0, #PSR_MODE_HYP
+
+ bne 1f
+ /*
+ * Trap from the hypervisor
+ *
+ * Inherit the state of the interrupts from the hypervisor
+ * context. For that we need to use SPSR (stored in r11) and
+ * modify CPSR accordingly.
+ *
+ * CPSR = (CPSR & ~hyp_iflags) | (SPSR & hyp_iflags)
+ */
+ mrs r10, cpsr
+ bic r10, r10, #\hyp_iflags
+ and r11, r11, #\hyp_iflags
+ orr r10, r10, r11
+ msr cpsr_cx, r10
+ b 2f
+
+1:
+ /* Trap from the guest */
+ bl save_guest_regs
+ .if \guest_iflags != n
+ cpsie \guest_iflags
+ .endif
+2:
/* We are ready to handle the trap, setup the registers and jump. */
- cpsie \iflags
adr lr, return_from_trap
mov r0, sp
/*
@@ -144,20 +190,6 @@ skip_check:
b do_trap_\trap
.endm
-#define __DEFINE_TRAP_ENTRY(trap, iflags) \
- ALIGN; \
-trap_##trap: \
- vector trap, iflags
-
-/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
-#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
-
-/* 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)
b trap_reset /* 0x00 - Reset */
@@ -228,14 +260,62 @@ decode_vectors:
#endif /* CONFIG_HARDEN_BRANCH_PREDICTOR */
-DEFINE_TRAP_ENTRY(reset)
-DEFINE_TRAP_ENTRY(undefined_instruction)
-DEFINE_TRAP_ENTRY(hypervisor_call)
-DEFINE_TRAP_ENTRY(prefetch_abort)
-DEFINE_TRAP_ENTRY(guest_sync)
-DEFINE_TRAP_ENTRY_NOIRQ(irq)
-DEFINE_TRAP_ENTRY_NOIRQ(fiq)
-DEFINE_TRAP_ENTRY_NOABORT(data_abort)
+/* Vector not used by the Hypervisor. */
+trap_reset:
+ vector reset
+
+/*
+ * Vector only used by the Hypervisor.
+ *
+ * While the exception can be executed with all the interrupts (e.g.
+ * IRQ) unmasked, the interrupted context may have purposefully masked
+ * some of them. So we want to inherit the state from the interrupted
+ * context.
+ */
+trap_undefined_instruction:
+ vector undefined_instruction, hyp_iflags=IFLAGS_AIF
+
+/* We should never reach this trap */
+trap_hypervisor_call:
+ vector hypervisor_call
+
+/*
+ * Vector only used by the hypervisor.
+ *
+ * While the exception can be executed with all the interrupts (e.g.
+ * IRQ) unmasked, the interrupted context may have purposefully masked
+ * some of them. So we want to inherit the state from the interrupted
+ * context.
+ */
+trap_prefetch_abort:
+ vector prefetch_abort, hyp_iflags=IFLAGS_AIF
+
+/*
+ * Vector only used by the hypervisor.
+ *
+ * Data Abort should be rare and most likely fatal. It is best to not
+ * unmask any interrupts to limit the amount of code that can run before
+ * the Data Abort is treated.
+ */
+trap_data_abort:
+ vector data_abort
+
+/* Vector only used by the guest. We can unmask Abort/IRQ. */
+trap_guest_sync:
+ vector guest_sync, guest_iflags=ai
+
+
+/* Vector used by the hypervisor and the guest. */
+trap_irq:
+ vector irq, guest_iflags=a, hyp_iflags=IFLAGS_A_F
+
+/*
+ * Vector used by the hypervisor and the guest.
+ *
+ * FIQ are not meant to happen, so we don't unmask any interrupts.
+ */
+trap_fiq:
+ vector fiq
return_from_trap:
/*
--
2.11.0
|
