[prev in list] [next in list] [prev in thread] [next in thread]
List: oss-security
Subject: [oss-security] Xen Security Advisory 303 v4 (CVE-2019-18422) - ARM: Interrupts are unconditionally u
From: Xen.org security team <security () xen ! org>
Date: 2019-10-31 12:30:44
Message-ID: E1iQ9bA-0004NO-AA () xenbits ! xenproject ! org
[Download RAW message or body]
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Xen Security Advisory CVE-2019-18422 / XSA-303
version 4
ARM: Interrupts are unconditionally unmasked in exception handlers
UPDATES IN VERSION 4
====================
Fix typoes in the series and add more reviewed-by tag.
Public release.
ISSUE DESCRIPTION
=================
When an exception occurs on an ARM system which is handled without
changing processor level, some interrupts are unconditionally enabled
during exception entry. So exceptions which occur when interrupts are
masked will effectively unmask the interrupts.
IMPACT
======
A malicious guest might contrive to arrange for critical Xen code to
run with interrupts erroneously enabled. This could lead to data
corruption, denial of service, or possibly even privilege escalation.
However a precise attack technique has not been identified.
VULNERABLE SYSTEMS
==================
All ARM systems are vulnerable. x86 systems are not vulnerable.
On some platforms this issue could possibly be exploited by an
unprivileged userspace attacker.
CREDITS
=======
This issue was discovered by Julian Grall of Arm.
RESOLUTION
==========
Applying the appropriate attached patch resolves this issue.
xsa303/*.patch xen-unstable .. Xen 4.9
xsa303-4.8/*.patch Xen 4.8
$ sha256sum xsa303* xsa303*/*
66b3eb28cfa633999da7480a37cd919293eb87aa730e7bc58b12c47bcdb0c9c0 xsa303.meta
7769eee9b876cdb7dde2ec664d34a5067f9b639d5c543ee89ff2eda818f04cab \
xsa303-4.8/0001-Revert-xen-arm32-entry-Consolidate-DEFINE_TRAP_ENTRY.patch \
f1337aa8c4b38f4ab61e7206c7bd8f5c782583947d9b9e1e8c6f139db73ca2cb \
xsa303-4.8/0002-xen-arm32-entry-Consolidate-DEFINE_TRAP_ENTRY-macros.patch \
160ea6acfba85faf1cbb670b0a3873f025c0dab388f73018a22a61104e1a5fe1 \
xsa303-4.8/0003-xen-arm32-entry-Fold-the-macro-SAVE_ALL-in-the-macro.patch \
2cc1e3282263f03c6b9c6e05039f84173b8dbc893a2cd88f80ce2275ff7478d8 \
xsa303-4.8/0004-xen-arm32-Don-t-blindly-unmask-interrupts-on-trap-wi.patch \
63c4a90c45ae28032e0149353cafd495cce5caa8c84ad022d21b8078710e996d \
xsa303-4.8/0005-xen-arm64-Don-t-blindly-unmask-interrupts-on-trap-wi.patch \
4da48a29aaad85a410021952b2b3cb4dae14365c688e724ed7fc80feea1334df \
xsa303/0001-xen-arm32-entry-Split-__DEFINE_ENTRY_TRAP-in-two.patch \
99773cbfb6f0df5f0c83477c9dcd39127cb361213455bd2cb1f6bcfe4566d5a2 \
xsa303/0002-xen-arm32-entry-Fold-the-macro-SAVE_ALL-in-the-macro.patch \
9e8241c311aa8da7fcb1da09b9d8b5a55c26a10f02355e37e97d1e7a3b6db7be \
xsa303/0003-xen-arm32-Don-t-blindly-unmask-interrupts-on-trap-wi.patch \
4c9bc0d0b27eff06f65f1a679263ffbcc8aa4c65117840284dc115ae49e7966d \
xsa303/0004-xen-arm64-Don-t-blindly-unmask-interrupts-on-trap-wi.patch $
DEPLOYMENT DURING EMBARGO
=========================
Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.
But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).
Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.
(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable. This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)
For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
iQFABAEBCAAqFiEEI+MiLBRfRHX6gGCng/4UyVfoK9kFAl26014MHHBncEB4ZW4u
b3JnAAoJEIP+FMlX6CvZ79IIALZ04OSkaCmDXeNhb7mXPjqyNPGY8bqXwD1TQd51
W7yLo+DM/cnkj+u3UR96Mkma3eAj8sJLKeuQGRMScyyfNCj/b0pY0M1h6XRi5NLN
zV6EWk7rR/87ID4Z82nwAq4lhsTgfgglH4I39oKZzFflHQtmij4DKuf/5K9g+6qT
8lc70ylgBep3Q3e73qJ1aLEvBYVnhs0lxY8QJDHOIS9GWY6/kqSVoWzK1dUtJDhD
vB/MPBtG1WxJETrCjC1Fat6lmfErjqiqX/tunFVFASFPL4aOTSVA7Oo9IJYX9XSY
6f3le7BYj8xJUp7A0z2vv0YBQvOQ/bsvs4ONMRpRswwDA+Q=
=eraI
-----END PGP SIGNATURE-----
["xsa303.meta" (application/octet-stream)]
["xsa303-4.8/0001-Revert-xen-arm32-entry-Consolidate-DEFINE_TRAP_ENTRY.patch" (application/octet-stream)]
From 3c295a1ea51bdcd31704e960a8847ef41abb8a60 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Thu, 17 Oct 2019 14:25:05 +0100
Subject: [PATCH 1/5] Revert "xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY_*
macros"
This reverts commit 6082e3ba8941b3d10c3cb73f445759c19e89afc9.
The patch was meant to only consolidate the code but it also re-enables
Abort interrupt. Follow-up patch will introduce a different way to
consolidate the code.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/arm32/entry.S | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index b66705ff50..3e320dc8ac 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -116,15 +116,11 @@ abort_guest_exit_end:
mov pc, lr
-/*
- * 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) \
+#define DEFINE_TRAP_ENTRY(trap) \
ALIGN; \
trap_##trap: \
SAVE_ALL; \
- cpsie iflags; \
+ cpsie i; /* local_irq_enable */ \
adr lr, return_from_trap; \
mov r0, sp; \
/* \
@@ -135,11 +131,15 @@ trap_##trap: \
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)
-
-/* Trap handler which unmask Abort, keep IRQ/FIQ masked */
-#define DEFINE_TRAP_ENTRY_NOIRQ(trap) __DEFINE_TRAP_ENTRY(trap, a)
+#define DEFINE_TRAP_ENTRY_NOIRQ(trap) \
+ ALIGN; \
+trap_##trap: \
+ SAVE_ALL; \
+ 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
.align 5
GLOBAL(hyp_traps_vector)
--
2.11.0
["xsa303-4.8/0002-xen-arm32-entry-Consolidate-DEFINE_TRAP_ENTRY-macros.patch" (application/octet-stream)]
From f9879f4ed198ca03a03bf69152f0158be6580252 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:07:53 +0100
Subject: [PATCH 2/5] xen/arm32: entry: Consolidate DEFINE_TRAP_ENTRY*() macros
The only difference between the two macros DEFINE_TRAP_ENTRY() and
DEFINE_TRAP_ENTRY_NOIRQ() is the list of interrupts to be unmasked.
While the macros are fairly small today, it will be necessary to add the
same code twice in follow-up patches.
To prevent too much duplication, a new assembly macros is introduced to
generate the body of a trap.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/arm32/entry.S | 41 ++++++++++++++++++++++++-----------------
1 file changed, 24 insertions(+), 17 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 3e320dc8ac..f6ba5a2082 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -116,30 +116,37 @@ abort_guest_exit_end:
mov pc, lr
+
+ /*
+ * Macro to define a trap entry. The iflags is optional and
+ * corresponds to alist of interrupts (Asynchronous Abort, IRQ, FIQ)
+ * to unmask.
+ */
+ .macro vector trap, iflags=n
+ SAVE_ALL
+ .if \iflags != n
+ cpsie \iflags
+ .endif
+ 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
+ .endm
+
#define DEFINE_TRAP_ENTRY(trap) \
ALIGN; \
trap_##trap: \
- SAVE_ALL; \
- cpsie i; /* local_irq_enable */ \
- 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
+ vector trap, iflags=i \
#define DEFINE_TRAP_ENTRY_NOIRQ(trap) \
ALIGN; \
trap_##trap: \
- SAVE_ALL; \
- 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
+ vector trap
.align 5
GLOBAL(hyp_traps_vector)
--
2.11.0
["xsa303-4.8/0003-xen-arm32-entry-Fold-the-macro-SAVE_ALL-in-the-macro.patch" (application/octet-stream)]
From 8db4a93dc6e2ac66bad0dac254e49789a8cd9fd2 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:15:48 +0100
Subject: [PATCH 3/5] xen/arm32: entry: Fold the macro SAVE_ALL in the macro
vector
Follow-up rework will require the macro vector to distinguish between
a trap from a guest vs while in the hypervisor.
The macro SAVE_ALL already has code to distinguish between the two and
it is only called by the vector macro. So fold the former into the
latter. This will help to avoid duplicating the check.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/arm32/entry.S | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index f6ba5a2082..c2d0ec2273 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -12,27 +12,6 @@
#define RESTORE_BANKED(mode) \
RESTORE_ONE_BANKED(SP_##mode) ; RESTORE_ONE_BANKED(LR_##mode) ; RESTORE_ONE_BANKED(SPSR_##mode)
-#define SAVE_ALL \
- sub sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */ \
- push {r0-r12}; /* Save R0-R12 */ \
- \
- mrs r11, ELR_hyp; /* ELR_hyp is return address. */\
- str r11, [sp, #UREGS_pc]; \
- \
- str lr, [sp, #UREGS_lr]; \
- \
- add r11, sp, #UREGS_kernel_sizeof+4; \
- str r11, [sp, #UREGS_sp]; \
- \
- mrc CP32(r11, HSR); /* Save exception syndrome */ \
- str r11, [sp, #UREGS_hsr]; \
- \
- mrs r11, SPSR_hyp; \
- str r11, [sp, #UREGS_cpsr]; \
- and r11, #PSR_MODE_MASK; \
- cmp r11, #PSR_MODE_HYP; \
- blne save_guest_regs
-
save_guest_regs:
#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
/*
@@ -51,7 +30,7 @@ save_guest_regs:
ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
str r11, [sp, #UREGS_sp]
SAVE_ONE_BANKED(SP_usr)
- /* LR_usr is the same physical register as lr and is saved in SAVE_ALL */
+ /* LR_usr is the same physical register as lr and is saved by the caller */
SAVE_BANKED(svc)
SAVE_BANKED(abt)
SAVE_BANKED(und)
@@ -123,7 +102,28 @@ abort_guest_exit_end:
* to unmask.
*/
.macro vector trap, iflags=n
- SAVE_ALL
+ /* Save registers in the stack */
+ sub sp, #(UREGS_SP_usr - UREGS_sp) /* SP, LR, SPSR, PC */
+ push {r0-r12} /* Save R0-R12 */
+ mrs r11, ELR_hyp /* ELR_hyp is return address */
+ str r11, [sp, #UREGS_pc]
+
+ str lr, [sp, #UREGS_lr]
+
+ add r11, sp, #(UREGS_kernel_sizeof + 4)
+
+ str r11, [sp, #UREGS_sp]
+
+ mrc CP32(r11, HSR) /* Save exception syndrome */
+ str r11, [sp, #UREGS_hsr]
+
+ mrs r11, SPSR_hyp
+ str r11, [sp, #UREGS_cpsr]
+ and r11, #PSR_MODE_MASK
+ cmp r11, #PSR_MODE_HYP
+ blne save_guest_regs
+
+ /* We are ready to handle the trap, setup the registers and jump. */
.if \iflags != n
cpsie \iflags
.endif
--
2.11.0
["xsa303-4.8/0004-xen-arm32-Don-t-blindly-unmask-interrupts-on-trap-wi.patch" (application/octet-stream)]
From 8b66a1104d920c478f0c35e6360154e06025f060 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 4/5] 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 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 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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/arm32/entry.S | 136 +++++++++++++++++++++++++++++++++++----------
1 file changed, 107 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index c2d0ec2273..f1f3b558dd 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -3,6 +3,16 @@
#include <asm/regs.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__I_ PSR_IRQ_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
@@ -95,13 +105,19 @@ abort_guest_exit_end:
mov pc, lr
-
/*
- * Macro to define a trap entry. The iflags is optional and
- * corresponds to alist 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=n
+ .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 */
@@ -119,14 +135,40 @@ abort_guest_exit_end:
mrs r11, SPSR_hyp
str r11, [sp, #UREGS_cpsr]
- and r11, #PSR_MODE_MASK
- cmp r11, #PSR_MODE_HYP
- blne save_guest_regs
- /* We are ready to handle the trap, setup the registers and jump. */
- .if \iflags != n
- cpsie \iflags
+
+ /*
+ * 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. */
adr lr, return_from_trap
mov r0, sp
/*
@@ -138,16 +180,6 @@ abort_guest_exit_end:
b do_trap_\trap
.endm
-#define DEFINE_TRAP_ENTRY(trap) \
- ALIGN; \
-trap_##trap: \
- vector trap, iflags=i \
-
-#define DEFINE_TRAP_ENTRY_NOIRQ(trap) \
- ALIGN; \
-trap_##trap: \
- vector trap
-
.align 5
GLOBAL(hyp_traps_vector)
b trap_reset /* 0x00 - Reset */
@@ -218,14 +250,60 @@ decode_vectors:
#endif /* CONFIG_HARDEN_BRANCH_PREDICTOR */
-DEFINE_TRAP_ENTRY(reset)
-DEFINE_TRAP_ENTRY(undefined_instruction)
-DEFINE_TRAP_ENTRY(supervisor_call)
-DEFINE_TRAP_ENTRY(prefetch_abort)
-DEFINE_TRAP_ENTRY(data_abort)
-DEFINE_TRAP_ENTRY(guest_sync)
-DEFINE_TRAP_ENTRY_NOIRQ(irq)
-DEFINE_TRAP_ENTRY_NOIRQ(fiq)
+/* Vector not used by the Hypervisor. */
+trap_reset:
+ vector reset
+
+/*
+ * Vector only used by the Hypervisor.
+ *
+ * While the exception can be executed with IRQ unmasked, the interrupted
+ * context may have purposefully masked it. So we want to inherit the state
+ * from the interrupted context.
+ */
+trap_undefined_instruction:
+ vector undefined_instruction, hyp_iflags=IFLAGS__I_
+
+/* We should never reach this trap */
+trap_supervisor_call:
+ vector supervisor_call
+
+/*
+ * Vector only used by the hypervisor.
+ *
+ * While the exception can be executed with IRQ unmasked, the interrupted
+ * context may have purposefully masked it. So we want to inherit the state
+ * from the interrupted context.
+ */
+trap_prefetch_abort:
+ vector prefetch_abort, hyp_iflags=IFLAGS__I_
+
+/*
+ * 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 IRQ. */
+trap_guest_sync:
+ vector guest_sync, guest_iflags=i
+
+
+/* Vector used by the hypervisor and the guest. */
+trap_irq:
+ vector irq
+
+/*
+ * 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
["xsa303-4.8/0005-xen-arm64-Don-t-blindly-unmask-interrupts-on-trap-wi.patch" (application/octet-stream)]
From aa342e4af814eab107b11f0b7abca0457b616fbc Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Mon, 7 Oct 2019 18:10:56 +0100
Subject: [PATCH 5/5] xen/arm64: Don't blindly unmask interrupts on trap
without a change of level
Some of the traps without a change of the level (i.e. hypervisor ->
hypervisor) will unmask interrupts regardless the state of them in the
interrupted context.
One of the consequences is IRQ will be unmasked when receiving a
synchronous exception (used by WARN*()). 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 interrupts, so the logic can stay simple:
- hyp_error: All the interrupts are now kept masked. SError should
be pretty rare and if ever happen then we most likely want to
avoid any other interrupts to be generated. The potential main
"caller" is during virtual SError synchronization on the exit
path from the guest (see check_pending_vserror).
- hyp_sync: The IRQ state is inherited from the interrupted context.
- hyp_irq: All interrupts are now kept masked.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/arm64/entry.S | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 0e7ddde9ed..7fd55300db 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -177,6 +177,14 @@ hyp_error_invalid:
entry hyp=1
invalid BAD_ERROR
+/*
+ * SError received while running in the hypervisor mode.
+ *
+ * Technically, we could unmask the IRQ if it were unmasked in the
+ * interrupted context. However, this require to check the PSTATE. For
+ * simplicity, as SError should be rare and potentially fatal,
+ * all interrupts are kept masked.
+ */
hyp_error:
/*
* Only two possibilities:
@@ -186,7 +194,6 @@ hyp_error:
* 2) Or we come from anywhere else, and that's a bug: we panic.
*/
entry hyp=1
- msr daifclr, #2
/*
* The ELR_EL2 may be modified by an interrupt, so we have to use the
@@ -214,14 +221,28 @@ hyp_error:
bl do_trap_guest_error
exit hyp=1
-/* Traps taken in Current EL with SP_ELx */
+/*
+ * Synchronous exception received while running in the hypervisor mode.
+ *
+ * While the exception can be executed with IRQ unmasked, the interrupt
+ * context may have purposefully masked it. So we want to inherit the
+ * state from the interrupted context.
+ */
hyp_sync:
entry hyp=1
- msr daifclr, #2
+
+ /* Inherit IRQ state and keep the other interrupts masked. */
+ mrs x0, SPSR_el2
+ and x0, x0, #PSR_IRQ_MASK
+ mov x1, #(PSR_DBG_MASK | PSR_ABT_MASK | PSR_FIQ_MASK)
+ orr x0, x0, x1
+ msr daif, x0
+
mov x0, sp
bl do_trap_hyp_sync
exit hyp=1
+/* IRQ received while running in the hypervisor mode. */
hyp_irq:
entry hyp=1
mov x0, sp
--
2.11.0
["xsa303/0001-xen-arm32-entry-Split-__DEFINE_ENTRY_TRAP-in-two.patch" (application/octet-stream)]
From c8cb33fa64c9ccbfa2a494a9dad2e0a763c09176 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:07:53 +0100
Subject: [PATCH 1/4] xen/arm32: entry: Split __DEFINE_ENTRY_TRAP in two
The preprocessing macro __DEFINE_ENTRY_TRAP is used to generate trap
entry function. While the macro is fairly small today, follow-up patches
will increase the size signicantly.
In general, assembly macros are more readable as they allow you to name
parameters and avoid '\'. So the actual implementation of the trap is
now switched to an assembly macro.
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 | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 0b4cd19abd..4a762e04f1 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -126,24 +126,28 @@ abort_guest_exit_end:
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 trap entry. The iflags corresponds to the list of
+ * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
+ */
+ .macro vector trap, iflags
+ SAVE_ALL
+ 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
+ .endm
+
#define __DEFINE_TRAP_ENTRY(trap, iflags) \
ALIGN; \
trap_##trap: \
- SAVE_ALL; \
- 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
+ vector trap, iflags
/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
--
2.11.0
["xsa303/0002-xen-arm32-entry-Fold-the-macro-SAVE_ALL-in-the-macro.patch" (application/octet-stream)]
From be7379207c83fa74f8a6c22a8ea213f02714776f Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:15:48 +0100
Subject: [PATCH 2/4] xen/arm32: entry: Fold the macro SAVE_ALL in the macro
vector
Follow-up rework will require the macro vector to distinguish between
a trap from a guest vs while in the hypervisor.
The macro SAVE_ALL already has code to distinguish between the two and
it is only called by the vector macro. So fold the former into the
latter. This will help to avoid duplicating the check.
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 | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 4a762e04f1..150cbc0b4b 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -13,27 +13,6 @@
#define RESTORE_BANKED(mode) \
RESTORE_ONE_BANKED(SP_##mode) ; RESTORE_ONE_BANKED(LR_##mode) ; RESTORE_ONE_BANKED(SPSR_##mode)
-#define SAVE_ALL \
- sub sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */ \
- push {r0-r12}; /* Save R0-R12 */ \
- \
- mrs r11, ELR_hyp; /* ELR_hyp is return address. */\
- str r11, [sp, #UREGS_pc]; \
- \
- str lr, [sp, #UREGS_lr]; \
- \
- add r11, sp, #UREGS_kernel_sizeof+4; \
- str r11, [sp, #UREGS_sp]; \
- \
- mrc CP32(r11, HSR); /* Save exception syndrome */ \
- str r11, [sp, #UREGS_hsr]; \
- \
- mrs r11, SPSR_hyp; \
- str r11, [sp, #UREGS_cpsr]; \
- and r11, #PSR_MODE_MASK; \
- cmp r11, #PSR_MODE_HYP; \
- blne save_guest_regs
-
save_guest_regs:
#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
/*
@@ -52,7 +31,7 @@ save_guest_regs:
ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
str r11, [sp, #UREGS_sp]
SAVE_ONE_BANKED(SP_usr)
- /* LR_usr is the same physical register as lr and is saved in SAVE_ALL */
+ /* LR_usr is the same physical register as lr and is saved by the caller */
SAVE_BANKED(svc)
SAVE_BANKED(abt)
SAVE_BANKED(und)
@@ -131,7 +110,28 @@ skip_check:
* interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
*/
.macro vector trap, iflags
- SAVE_ALL
+ /* Save registers in the stack */
+ sub sp, #(UREGS_SP_usr - UREGS_sp) /* SP, LR, SPSR, PC */
+ push {r0-r12} /* Save R0-R12 */
+ mrs r11, ELR_hyp /* ELR_hyp is return address */
+ str r11, [sp, #UREGS_pc]
+
+ str lr, [sp, #UREGS_lr]
+
+ add r11, sp, #(UREGS_kernel_sizeof + 4)
+
+ str r11, [sp, #UREGS_sp]
+
+ mrc CP32(r11, HSR) /* Save exception syndrome */
+ str r11, [sp, #UREGS_hsr]
+
+ mrs r11, SPSR_hyp
+ str r11, [sp, #UREGS_cpsr]
+ and r11, #PSR_MODE_MASK
+ cmp r11, #PSR_MODE_HYP
+ blne save_guest_regs
+
+ /* We are ready to handle the trap, setup the registers and jump. */
cpsie \iflags
adr lr, return_from_trap
mov r0, sp
--
2.11.0
["xsa303/0003-xen-arm32-Don-t-blindly-unmask-interrupts-on-trap-wi.patch" (application/octet-stream)]
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
["xsa303/0004-xen-arm64-Don-t-blindly-unmask-interrupts-on-trap-wi.patch" (application/octet-stream)]
From c6d290ce157a044dec417fdda8db71e41a37d744 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Mon, 7 Oct 2019 18:10:56 +0100
Subject: [PATCH 4/4] xen/arm64: Don't blindly unmask interrupts on trap
without a change of level
Some of the traps without a change of the level (i.e. hypervisor ->
hypervisor) will unmask interrupts regardless the state of them in the
interrupted context.
One of the consequences is IRQ will be unmasked when receiving a
synchronous exception (used by WARN*()). 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:
- hyp_error: All the interrupts are now kept masked. SError should
be pretty rare and if ever happen then we most likely want to
avoid any other interrupts to be generated. The potential main
"caller" is during virtual SError synchronization on the exit
path from the guest (see check_pending_vserror).
- hyp_sync: The interrupts state is inherited from the interrupted
context.
- hyp_irq: All the interrupts but IRQ state are inherited from the
interrupted context. IRQ is kept masked.
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/arm64/entry.S | 47 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 2d9a2713a1..3e41ba65b6 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -188,24 +188,63 @@ hyp_error_invalid:
entry hyp=1
invalid BAD_ERROR
+/*
+ * SError received while running in the hypervisor mode.
+ *
+ * Technically, we could unmask the IRQ if it were unmasked in the
+ * interrupted context. However, this require to check the PSTATE. For
+ * simplicity, as SError should be rare and potentially fatal,
+ * all interrupts are kept masked.
+ */
hyp_error:
entry hyp=1
- msr daifclr, #2
mov x0, sp
bl do_trap_hyp_serror
exit hyp=1
-/* Traps taken in Current EL with SP_ELx */
+/*
+ * Synchronous exception received while running in the hypervisor mode.
+ *
+ * While the exception could 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.
+ */
hyp_sync:
entry hyp=1
- msr daifclr, #6
+
+ /* Inherit interrupts */
+ mrs x0, SPSR_el2
+ and x0, x0, #(PSR_DBG_MASK | PSR_ABT_MASK | PSR_IRQ_MASK | PSR_FIQ_MASK)
+ msr daif, x0
+
mov x0, sp
bl do_trap_hyp_sync
exit hyp=1
+/*
+ * IRQ received while running in the hypervisor mode.
+ *
+ * While the exception could be executed with all the interrupts but IRQ
+ * unmasked, the interrupted context may have purposefully masked some
+ * of them. So we want to inherit the state from the interrupt context
+ * and keep IRQ masked.
+ *
+ * XXX: We may want to consider an ordering between interrupts (e.g. if
+ * SError are masked, then IRQ should be masked too). However, this
+ * would require some rework in some paths (e.g. panic, livepatch) to
+ * ensure the ordering is enforced everywhere.
+ */
hyp_irq:
entry hyp=1
- msr daifclr, #4
+
+ /* Inherit D, A, F interrupts and keep I masked */
+ mrs x0, SPSR_el2
+ mov x1, #(PSR_DBG_MASK | PSR_ABT_MASK | PSR_FIQ_MASK)
+ and x0, x0, x1
+ orr x0, x0, #PSR_IRQ_MASK
+ msr daif, x0
+
mov x0, sp
bl do_trap_irq
exit hyp=1
--
2.11.0
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic