xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
@ 2021-03-13 16:06 Julien Grall
  2021-03-13 16:06 ` [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files Julien Grall
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Julien Grall @ 2021-03-13 16:06 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

Hi all,

Last year, Arm released a whitepaper about a new category of speculation.
(see [1] and [2]). In short, a processor may be able to speculate past
some of the unconditional control flow instructions (e.g eret, smc, br).

In some of the cases, the registers will contain values controlled by
the guest. While there is no known gadget afterwards, we still want to
prevent any leakage in the future.

The mitigation is planned in two parts:
   1) Arm provided patches for both GCC and LLVM to add speculation barrier
   and remove problematic code sequence.
   2) Inspection of assembly code and call to higher level (e.g smc in our case).

I still haven't looked at 1) and how to mitigate properly Arm32 (see
patch #1) and SMC call. So this issue is not fully addressed.

Note that the ERET instruction was already addressed as part of XSA-312.

Cheers,

[1] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability
[2] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability/downloads/straight-line-speculation

Julien Grall (2):
  xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly
    files
  xen/arm64: Place a speculation barrier following an ret instruction

 xen/arch/arm/Makefile                |  2 +-
 xen/arch/arm/arm32/entry.S           |  2 +-
 xen/arch/arm/arm32/head.S            |  1 -
 xen/arch/arm/arm32/lib/lib1funcs.S   |  1 +
 xen/arch/arm/arm32/proc-v7.S         |  1 -
 xen/arch/arm/arm64/debug-cadence.inc |  1 -
 xen/arch/arm/arm64/debug-pl011.inc   |  2 --
 xen/arch/arm/arm64/entry.S           |  2 --
 xen/arch/arm/arm64/head.S            |  2 --
 xen/arch/arm/arm64/smc.S             |  3 ---
 xen/include/asm-arm/arm64/macros.h   |  6 ++++++
 xen/include/asm-arm/config.h         |  6 ++++++
 xen/include/asm-arm/macros.h         | 18 +++++++++---------
 13 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.17.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files
  2021-03-13 16:06 [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
@ 2021-03-13 16:06 ` Julien Grall
  2021-03-17 14:38   ` Bertrand Marquis
  2021-03-13 16:06 ` [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction Julien Grall
  2021-03-15 13:32 ` [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Bertrand Marquis
  2 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-13 16:06 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

In a follow-up patch we may want to automatically replace some
mnemonics (such as ret) with a different sequence.

To ensure all the assembly files will include asm/macros.h it is best to
automatically include it on single assembly. This can be done via
config.h.

It was necessary to include a few more headers as dependency:
  - <asm/asm_defns.h> to define sizeof_*
  - <xen/page-size.h> which is already a latent issue given STACK_ORDER
  rely on PAGE_SIZE.

Unfortunately the build system will use -D__ASSEMBLY__ when generating
the linker script. A new option -D__LINKER__ is introduceed and used for
the linker script to avoid including headers (such as asm/macros.h) that
may not be compatible with the syntax.

Lastly, take the opportunity to remove both asm/asm-offsets.h and
asm/macros.h from the various assembly files as they are now
automagically included.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/Makefile                | 2 +-
 xen/arch/arm/arm32/entry.S           | 1 -
 xen/arch/arm/arm32/head.S            | 1 -
 xen/arch/arm/arm32/proc-v7.S         | 1 -
 xen/arch/arm/arm64/debug-cadence.inc | 1 -
 xen/arch/arm/arm64/debug-pl011.inc   | 2 --
 xen/arch/arm/arm64/entry.S           | 2 --
 xen/arch/arm/arm64/head.S            | 2 --
 xen/arch/arm/arm64/smc.S             | 3 ---
 xen/include/asm-arm/config.h         | 6 ++++++
 10 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 16e6523e2cc6..9ffc3f771c51 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -135,7 +135,7 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
 	$(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $<
 
 xen.lds: xen.lds.S
-	$(CPP) -P $(a_flags) -MQ $@ -o $@ $<
+	$(CPP) -P $(a_flags) -D__LINKER__ -MQ $@ -o $@ $<
 
 dtb.o: $(CONFIG_DTB_FILE)
 
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index b228d44b190c..f2f1bc7a3158 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -1,4 +1,3 @@
-#include <asm/asm_defns.h>
 #include <asm/sysregs.h>
 #include <asm/regs.h>
 #include <asm/alternative.h>
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c404fa973e9b..9084023a6ed9 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -18,7 +18,6 @@
  */
 
 #include <asm/page.h>
-#include <asm/asm_defns.h>
 #include <asm/early_printk.h>
 
 #define ZIMAGE_MAGIC_NUMBER 0x016f2818
diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S
index 46bfc7a9074c..1efde2d72da0 100644
--- a/xen/arch/arm/arm32/proc-v7.S
+++ b/xen/arch/arm/arm32/proc-v7.S
@@ -17,7 +17,6 @@
  * GNU General Public License for more details.
  */
 
-#include <asm/asm_defns.h>
 #include <asm/arm32/processor.h>
 #include <asm/sysregs.h>
 
diff --git a/xen/arch/arm/arm64/debug-cadence.inc b/xen/arch/arm/arm64/debug-cadence.inc
index 7df0abe4756f..0b6f2e094e18 100644
--- a/xen/arch/arm/arm64/debug-cadence.inc
+++ b/xen/arch/arm/arm64/debug-cadence.inc
@@ -17,7 +17,6 @@
  * GNU General Public License for more details.
  */
 
-#include <asm/asm_defns.h>
 #include <asm/cadence-uart.h>
 
 /*
diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
index 385deff49b1b..1928a2e3ffbb 100644
--- a/xen/arch/arm/arm64/debug-pl011.inc
+++ b/xen/arch/arm/arm64/debug-pl011.inc
@@ -16,8 +16,6 @@
  * GNU General Public License for more details.
  */
 
-#include <asm/asm_defns.h>
-
 /*
  * PL011 UART initialization
  * xb: register which containts the UART base address
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 175ea2981e72..ab9a65fc1475 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -1,6 +1,4 @@
-#include <asm/asm_defns.h>
 #include <asm/current.h>
-#include <asm/macros.h>
 #include <asm/regs.h>
 #include <asm/alternative.h>
 #include <asm/smccc.h>
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 5d44667bd89d..fa7a3ffd2926 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -21,11 +21,9 @@
  */
 
 #include <asm/page.h>
-#include <asm/asm_defns.h>
 #include <asm/early_printk.h>
 #include <efi/efierr.h>
 #include <asm/arm64/efibind.h>
-#include <asm/arm64/macros.h>
 
 #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
index b0752be57e8f..91bae62dd4d2 100644
--- a/xen/arch/arm/arm64/smc.S
+++ b/xen/arch/arm/arm64/smc.S
@@ -13,9 +13,6 @@
  * GNU General Public License for more details.
  */
 
-#include <asm/asm_defns.h>
-#include <asm/macros.h>
-
 /*
  * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
  *                          register_t a3, register_t a4, register_t a5,
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 5c10c755db46..51273b9db1fc 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -69,6 +69,7 @@
 #endif
 
 #include <xen/const.h>
+#include <xen/page-size.h>
 
 /*
  * Common ARM32 and ARM64 layout:
@@ -190,6 +191,11 @@ extern unsigned long frametable_virt_end;
 #define watchdog_disable() ((void)0)
 #define watchdog_enable()  ((void)0)
 
+#if defined(__ASSEMBLY__) && !defined(__LINKER__)
+#include <asm/asm-offsets.h>
+#include <asm/macros.h>
+#endif
+
 #endif /* __ARM_CONFIG_H__ */
 /*
  * Local variables:
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-13 16:06 [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
  2021-03-13 16:06 ` [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files Julien Grall
@ 2021-03-13 16:06 ` Julien Grall
  2021-03-17 14:56   ` Bertrand Marquis
  2021-03-15 13:32 ` [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Bertrand Marquis
  2 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-13 16:06 UTC (permalink / raw)
  To: xen-devel
  Cc: bertrand.marquis, Julien Grall, Stefano Stabellini, Julien Grall,
	Volodymyr Babchuk

From: Julien Grall <jgrall@amazon.com>

Some CPUs can speculate past a RET instruction and potentially perform
speculative accesses to memory before processing the return.

There is no known gadget available after the RET instruction today.
However some of the registers (such as in check_pending_guest_serror())
may contain a value provided by the guest.

In order to harden the code, it would be better to add a speculation
barrier after each RET instruction. The performance impact is meant to
be negligeable as the speculation barrier is not meant to be
architecturally executed.

Rather than manually inserting a speculation barrier, use a macro
which overrides the mnemonic RET and replace with RET + SB. We need to
use the opcode for RET to prevent any macro recursion.

This patch is only covering the assembly code. C code would need to be
covered separately using the compiler support.

This is part of the work to mitigate straight-line speculation.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

It is not clear to me whether Armv7 (we don't officially support 32-bit
hypervisor on Armv8) is also affected by straight-line speculation.

But the mitigation is a lot messier because opcode can be optionally
executed. So this Arm32 is left alone for now.

    Changes in v2:
        - Use a macro rather than inserting the speculation barrier
        manually
        - Remove mitigation for arm32
---
 xen/arch/arm/arm32/entry.S         |  1 +
 xen/arch/arm/arm32/lib/lib1funcs.S |  1 +
 xen/include/asm-arm/arm64/macros.h |  6 ++++++
 xen/include/asm-arm/macros.h       | 18 +++++++++---------
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index f2f1bc7a3158..d0a066484f13 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -441,6 +441,7 @@ ENTRY(__context_switch)
 
         add     r4, r1, #VCPU_arch_saved_context
         ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return */
+        sb
 
 /*
  * Local variables:
diff --git a/xen/arch/arm/arm32/lib/lib1funcs.S b/xen/arch/arm/arm32/lib/lib1funcs.S
index f1278bd6c139..8c33ffbbcc4c 100644
--- a/xen/arch/arm/arm32/lib/lib1funcs.S
+++ b/xen/arch/arm/arm32/lib/lib1funcs.S
@@ -382,5 +382,6 @@ UNWIND(.save {lr})
 	bl	__div0
 	mov	r0, #0			@ About as wrong as it could be.
 	ldr	pc, [sp], #8
+	sb
 UNWIND(.fnend)
 ENDPROC(Ldiv0)
diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
index f981b4f43e84..4614394b3dd5 100644
--- a/xen/include/asm-arm/arm64/macros.h
+++ b/xen/include/asm-arm/arm64/macros.h
@@ -21,6 +21,12 @@
     ldr     \dst, [\dst, \tmp]
     .endm
 
+    .macro  ret
+        // ret opcode
+        .inst 0xd65f03c0
+        sb
+    .endm
+
 /*
  * Register aliases.
  */
diff --git a/xen/include/asm-arm/macros.h b/xen/include/asm-arm/macros.h
index 4833671f4ced..1aa373760f98 100644
--- a/xen/include/asm-arm/macros.h
+++ b/xen/include/asm-arm/macros.h
@@ -5,6 +5,15 @@
 # error "This file should only be included in assembly file"
 #endif
 
+    /*
+     * Speculative barrier
+     * XXX: Add support for the 'sb' instruction
+     */
+    .macro sb
+    dsb nsh
+    isb
+    .endm
+
 #if defined (CONFIG_ARM_32)
 # include <asm/arm32/macros.h>
 #elif defined(CONFIG_ARM_64)
@@ -20,13 +29,4 @@
     .endr
     .endm
 
-    /*
-     * Speculative barrier
-     * XXX: Add support for the 'sb' instruction
-     */
-    .macro sb
-    dsb nsh
-    isb
-    .endm
-
 #endif /* __ASM_ARM_MACROS_H */
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
  2021-03-13 16:06 [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
  2021-03-13 16:06 ` [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files Julien Grall
  2021-03-13 16:06 ` [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction Julien Grall
@ 2021-03-15 13:32 ` Bertrand Marquis
  2021-03-16 15:27   ` Julien Grall
  2 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-15 13:32 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> Last year, Arm released a whitepaper about a new category of speculation.
> (see [1] and [2]). In short, a processor may be able to speculate past
> some of the unconditional control flow instructions (e.g eret, smc, br).
> 
> In some of the cases, the registers will contain values controlled by
> the guest. While there is no known gadget afterwards, we still want to
> prevent any leakage in the future.
> 
> The mitigation is planned in two parts:
>   1) Arm provided patches for both GCC and LLVM to add speculation barrier
>   and remove problematic code sequence.
>   2) Inspection of assembly code and call to higher level (e.g smc in our case).
> 
> I still haven't looked at 1) and how to mitigate properly Arm32 (see
> patch #1) and SMC call. So this issue is not fully addressed.
> 
> Note that the ERET instruction was already addressed as part of XSA-312.

On my tests, this serie is breaking the arm64 build:
| aarch64-poky-linux-ld --sysroot=/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/recipe-sysroot         -EL  --fix-cortex-a53-843419 --fix-cortex-a53-843419 -r -o built_in.o memcpy.o memcmp.o memmove.o memset.o memchr.o clear_page.o bitops.o find_next_bit.o strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
| arm64/head.S: Assembler messages:
| arm64/head.S:305: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot CPU booting -\r\n")'
| arm64/head.S:331: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
| arm64/head.S:365: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- CPU ")'
| make[6]: Leaving directory '/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/arch/arm/arm64/lib'
| arm64/head.S:367: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " booting -\r\n")'
| arm64/head.S:398: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
| arm64/head.S:412: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Current EL ")'
| arm64/head.S:415: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " -\r\n")'
| arm64/head.S:424: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Xen must be entered in NS EL2 mode -\r\n")'
| arm64/head.S:425: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Please update the bootloader -\r\n")'
| arm64/head.S:441: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Zero BSS -\r\n")'
| arm64/head.S:459: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Initialize CPU -\r\n")'
| arm64/head.S:654: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Unable to build boot page tables - virt and phys addresses clash. -\r\n")'
| arm64/head.S:666: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Turning on paging -\r\n")'
| arm64/head.S:800: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot failed -\r\n")'
| arm64/head.S:848: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- UART enabled -\r\n")'
| {standard input}: Error: local label `"98" (instance number 1 of a fb label)' is not defined
| /home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/Rules.mk:204: recipe for target 'arm64/head.o' failed

This was done adding your 2 patches on top of current staging.

Cheers
Bertrand

> 
> Cheers,
> 
> [1] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability
> [2] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability/downloads/straight-line-speculation
> 
> Julien Grall (2):
>  xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly
>    files
>  xen/arm64: Place a speculation barrier following an ret instruction
> 
> xen/arch/arm/Makefile                |  2 +-
> xen/arch/arm/arm32/entry.S           |  2 +-
> xen/arch/arm/arm32/head.S            |  1 -
> xen/arch/arm/arm32/lib/lib1funcs.S   |  1 +
> xen/arch/arm/arm32/proc-v7.S         |  1 -
> xen/arch/arm/arm64/debug-cadence.inc |  1 -
> xen/arch/arm/arm64/debug-pl011.inc   |  2 --
> xen/arch/arm/arm64/entry.S           |  2 --
> xen/arch/arm/arm64/head.S            |  2 --
> xen/arch/arm/arm64/smc.S             |  3 ---
> xen/include/asm-arm/arm64/macros.h   |  6 ++++++
> xen/include/asm-arm/config.h         |  6 ++++++
> xen/include/asm-arm/macros.h         | 18 +++++++++---------
> 13 files changed, 24 insertions(+), 23 deletions(-)
> 
> -- 
> 2.17.1
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
  2021-03-15 13:32 ` [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Bertrand Marquis
@ 2021-03-16 15:27   ` Julien Grall
  2021-03-16 17:16     ` Bertrand Marquis
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-16 15:27 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk



On 15/03/2021 13:32, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Hi all,
>>
>> Last year, Arm released a whitepaper about a new category of speculation.
>> (see [1] and [2]). In short, a processor may be able to speculate past
>> some of the unconditional control flow instructions (e.g eret, smc, br).
>>
>> In some of the cases, the registers will contain values controlled by
>> the guest. While there is no known gadget afterwards, we still want to
>> prevent any leakage in the future.
>>
>> The mitigation is planned in two parts:
>>    1) Arm provided patches for both GCC and LLVM to add speculation barrier
>>    and remove problematic code sequence.
>>    2) Inspection of assembly code and call to higher level (e.g smc in our case).
>>
>> I still haven't looked at 1) and how to mitigate properly Arm32 (see
>> patch #1) and SMC call. So this issue is not fully addressed.
>>
>> Note that the ERET instruction was already addressed as part of XSA-312.
> 
> On my tests, this serie is breaking the arm64 build:
> | aarch64-poky-linux-ld --sysroot=/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/recipe-sysroot         -EL  --fix-cortex-a53-843419 --fix-cortex-a53-843419 -r -o built_in.o memcpy.o memcmp.o memmove.o memset.o memchr.o clear_page.o bitops.o find_next_bit.o strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o

I can't see any build failure with the following GCC:

42sh> aarch64-linux-gnu-gcc
aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

AFAICT, there is also no compilation issue reported by gitlab:

https://gitlab.com/xen-project/patchew/xen/-/pipelines/269989894

What's the version of your compiler? Do you have steps to reproduce your 
setup?

> | arm64/head.S: Assembler messages:
> | arm64/head.S:305: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot CPU booting -\r\n")'

This is strange, the code should use RODATA_STR() but here it is in 
lower case. Can you check in your tree whether there some instance of 
the lower case version?

If not, this may just be GAS printing in lower cases.

> | arm64/head.S:331: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
> | arm64/head.S:365: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- CPU ")'
> | make[6]: Leaving directory '/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/arch/arm/arm64/lib'
> | arm64/head.S:367: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " booting -\r\n")'
> | arm64/head.S:398: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
> | arm64/head.S:412: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Current EL ")'
> | arm64/head.S:415: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " -\r\n")'
> | arm64/head.S:424: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Xen must be entered in NS EL2 mode -\r\n")'
> | arm64/head.S:425: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Please update the bootloader -\r\n")'
> | arm64/head.S:441: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Zero BSS -\r\n")'
> | arm64/head.S:459: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Initialize CPU -\r\n")'
> | arm64/head.S:654: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Unable to build boot page tables - virt and phys addresses clash. -\r\n")'
> | arm64/head.S:666: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Turning on paging -\r\n")'
> | arm64/head.S:800: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot failed -\r\n")'
> | arm64/head.S:848: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- UART enabled -\r\n")'
> | {standard input}: Error: local label `"98" (instance number 1 of a fb label)' is not defined
> | /home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/Rules.mk:204: recipe for target 'arm64/head.o' failed
> 
> This was done adding your 2 patches on top of current staging.
> 
> Cheers
> Bertrand
> 
>>
>> Cheers,
>>
>> [1] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability
>> [2] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability/downloads/straight-line-speculation
>>
>> Julien Grall (2):
>>   xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly
>>     files
>>   xen/arm64: Place a speculation barrier following an ret instruction
>>
>> xen/arch/arm/Makefile                |  2 +-
>> xen/arch/arm/arm32/entry.S           |  2 +-
>> xen/arch/arm/arm32/head.S            |  1 -
>> xen/arch/arm/arm32/lib/lib1funcs.S   |  1 +
>> xen/arch/arm/arm32/proc-v7.S         |  1 -
>> xen/arch/arm/arm64/debug-cadence.inc |  1 -
>> xen/arch/arm/arm64/debug-pl011.inc   |  2 --
>> xen/arch/arm/arm64/entry.S           |  2 --
>> xen/arch/arm/arm64/head.S            |  2 --
>> xen/arch/arm/arm64/smc.S             |  3 ---
>> xen/include/asm-arm/arm64/macros.h   |  6 ++++++
>> xen/include/asm-arm/config.h         |  6 ++++++
>> xen/include/asm-arm/macros.h         | 18 +++++++++---------
>> 13 files changed, 24 insertions(+), 23 deletions(-)
>>
>> -- 
>> 2.17.1
>>
> 

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
  2021-03-16 15:27   ` Julien Grall
@ 2021-03-16 17:16     ` Bertrand Marquis
  2021-03-17 11:20       ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-16 17:16 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 16 Mar 2021, at 15:27, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 15/03/2021 13:32, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> Hi all,
>>> 
>>> Last year, Arm released a whitepaper about a new category of speculation.
>>> (see [1] and [2]). In short, a processor may be able to speculate past
>>> some of the unconditional control flow instructions (e.g eret, smc, br).
>>> 
>>> In some of the cases, the registers will contain values controlled by
>>> the guest. While there is no known gadget afterwards, we still want to
>>> prevent any leakage in the future.
>>> 
>>> The mitigation is planned in two parts:
>>>   1) Arm provided patches for both GCC and LLVM to add speculation barrier
>>>   and remove problematic code sequence.
>>>   2) Inspection of assembly code and call to higher level (e.g smc in our case).
>>> 
>>> I still haven't looked at 1) and how to mitigate properly Arm32 (see
>>> patch #1) and SMC call. So this issue is not fully addressed.
>>> 
>>> Note that the ERET instruction was already addressed as part of XSA-312.
>> On my tests, this serie is breaking the arm64 build:
>> | aarch64-poky-linux-ld --sysroot=/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/recipe-sysroot         -EL  --fix-cortex-a53-843419 --fix-cortex-a53-843419 -r -o built_in.o memcpy.o memcmp.o memmove.o memset.o memchr.o clear_page.o bitops.o find_next_bit.o strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
> 
> I can't see any build failure with the following GCC:
> 
> 42sh> aarch64-linux-gnu-gcc
> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
> Copyright (C) 2017 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions.  There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> AFAICT, there is also no compilation issue reported by gitlab:
> 
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/269989894
> 
> What's the version of your compiler? Do you have steps to reproduce your setup?

You need to have earlyprintk enabled
I am using gcc 7.5.0:
aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0

one configuration triggering the issue is using the default .config with the following items added:
CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS=y
CONFIG_DEBUG_LOCK_PROFILE=y
CONFIG_PERF_COUNTERS=y
CONFIG_PERF_ARRAYS=y
CONFIG_DEVICE_TREE_DEBUG=y
CONFIG_DEBUG_TRACE=y
CONFIG_EARLY_PRINTK_JUNO=y
CONFIG_EARLY_UART_PL011=y
CONFIG_EARLY_PRINTK=y
CONFIG_EARLY_UART_BASE_ADDRESS=0x7ff80000
CONFIG_EARLY_UART_PL011_BAUD_RATE=115200
CONFIG_EARLY_UART_INIT=y
CONFIG_EARLY_PRINTK_INC="debug-pl011.inc”

> 
>> | arm64/head.S: Assembler messages:
>> | arm64/head.S:305: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot CPU booting -\r\n")'
> 
> This is strange, the code should use RODATA_STR() but here it is in lower case. Can you check in your tree whether there some instance of the lower case version?

I have no instance of rodata_str in lower case.

> 
> If not, this may just be GAS printing in lower cases.

it probably is then.

If you need help on this i can try to dig on that a bit later this week (thursday or friday).

Cheers
Bertrand

> 
>> | arm64/head.S:331: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
>> | arm64/head.S:365: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- CPU ")'
>> | make[6]: Leaving directory '/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/arch/arm/arm64/lib'
>> | arm64/head.S:367: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " booting -\r\n")'
>> | arm64/head.S:398: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Ready -\r\n")'
>> | arm64/head.S:412: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Current EL ")'
>> | arm64/head.S:415: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, " -\r\n")'
>> | arm64/head.S:424: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Xen must be entered in NS EL2 mode -\r\n")'
>> | arm64/head.S:425: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Please update the bootloader -\r\n")'
>> | arm64/head.S:441: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Zero BSS -\r\n")'
>> | arm64/head.S:459: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Initialize CPU -\r\n")'
>> | arm64/head.S:654: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Unable to build boot page tables - virt and phys addresses clash. -\r\n")'
>> | arm64/head.S:666: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Turning on paging -\r\n")'
>> | arm64/head.S:800: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- Boot failed -\r\n")'
>> | arm64/head.S:848: Error: unknown mnemonic `rodata_str' -- `rodata_str(98, "- UART enabled -\r\n")'
>> | {standard input}: Error: local label `"98" (instance number 1 of a fb label)' is not defined
>> | /home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/local-xen/xen/xen/Rules.mk:204: recipe for target 'arm64/head.o' failed
>> This was done adding your 2 patches on top of current staging.
>> Cheers
>> Bertrand
>>> 
>>> Cheers,
>>> 
>>> [1] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability
>>> [2] https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability/downloads/straight-line-speculation
>>> 
>>> Julien Grall (2):
>>>  xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly
>>>    files
>>>  xen/arm64: Place a speculation barrier following an ret instruction
>>> 
>>> xen/arch/arm/Makefile                |  2 +-
>>> xen/arch/arm/arm32/entry.S           |  2 +-
>>> xen/arch/arm/arm32/head.S            |  1 -
>>> xen/arch/arm/arm32/lib/lib1funcs.S   |  1 +
>>> xen/arch/arm/arm32/proc-v7.S         |  1 -
>>> xen/arch/arm/arm64/debug-cadence.inc |  1 -
>>> xen/arch/arm/arm64/debug-pl011.inc   |  2 --
>>> xen/arch/arm/arm64/entry.S           |  2 --
>>> xen/arch/arm/arm64/head.S            |  2 --
>>> xen/arch/arm/arm64/smc.S             |  3 ---
>>> xen/include/asm-arm/arm64/macros.h   |  6 ++++++
>>> xen/include/asm-arm/config.h         |  6 ++++++
>>> xen/include/asm-arm/macros.h         | 18 +++++++++---------
>>> 13 files changed, 24 insertions(+), 23 deletions(-)
>>> 
>>> -- 
>>> 2.17.1
>>> 
> 
> -- 
> Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
  2021-03-16 17:16     ` Bertrand Marquis
@ 2021-03-17 11:20       ` Julien Grall
  2021-03-17 14:04         ` Bertrand Marquis
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-17 11:20 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: Xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk



On 16/03/2021 17:16, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 16 Mar 2021, at 15:27, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 15/03/2021 13:32, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Hi all,
>>>>
>>>> Last year, Arm released a whitepaper about a new category of speculation.
>>>> (see [1] and [2]). In short, a processor may be able to speculate past
>>>> some of the unconditional control flow instructions (e.g eret, smc, br).
>>>>
>>>> In some of the cases, the registers will contain values controlled by
>>>> the guest. While there is no known gadget afterwards, we still want to
>>>> prevent any leakage in the future.
>>>>
>>>> The mitigation is planned in two parts:
>>>>    1) Arm provided patches for both GCC and LLVM to add speculation barrier
>>>>    and remove problematic code sequence.
>>>>    2) Inspection of assembly code and call to higher level (e.g smc in our case).
>>>>
>>>> I still haven't looked at 1) and how to mitigate properly Arm32 (see
>>>> patch #1) and SMC call. So this issue is not fully addressed.
>>>>
>>>> Note that the ERET instruction was already addressed as part of XSA-312.
>>> On my tests, this serie is breaking the arm64 build:
>>> | aarch64-poky-linux-ld --sysroot=/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/recipe-sysroot         -EL  --fix-cortex-a53-843419 --fix-cortex-a53-843419 -r -o built_in.o memcpy.o memcmp.o memmove.o memset.o memchr.o clear_page.o bitops.o find_next_bit.o strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
>>
>> I can't see any build failure with the following GCC:
>>
>> 42sh> aarch64-linux-gnu-gcc
>> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
>> Copyright (C) 2017 Free Software Foundation, Inc.
>> This is free software; see the source for copying conditions.  There is NO
>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>
>> AFAICT, there is also no compilation issue reported by gitlab:
>>
>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/269989894
>>
>> What's the version of your compiler? Do you have steps to reproduce your setup?
> 
> You need to have earlyprintk enabled
> I am using gcc 7.5.0:
> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
> 
> one configuration triggering the issue is using the default .config with the following items added:
> CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS=y
> CONFIG_DEBUG_LOCK_PROFILE=y
> CONFIG_PERF_COUNTERS=y
> CONFIG_PERF_ARRAYS=y
> CONFIG_DEVICE_TREE_DEBUG=y
> CONFIG_DEBUG_TRACE=y
> CONFIG_EARLY_PRINTK_JUNO=y
> CONFIG_EARLY_UART_PL011=y
> CONFIG_EARLY_PRINTK=y
> CONFIG_EARLY_UART_BASE_ADDRESS=0x7ff80000
> CONFIG_EARLY_UART_PL011_BAUD_RATE=115200
> CONFIG_EARLY_UART_INIT=y
> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc”

Thanks for providing the .config. I managed to reproduce it. So I 
removed "asm_defns.h" everywhere but forgot to include it in the 
"config.h" :/.

This small change fixed the error:

diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index 51273b9db1fc..c7b77912013e 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -192,7 +192,7 @@ extern unsigned long frametable_virt_end;
  #define watchdog_enable()  ((void)0)

  #if defined(__ASSEMBLY__) && !defined(__LINKER__)
-#include <asm/asm-offsets.h>
+#include <asm/asm_defns.h>
  #include <asm/macros.h>
  #endif

Would you still be happy to review the series before I send a v3?

Cheers,

-- 
Julien Grall


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation
  2021-03-17 11:20       ` Julien Grall
@ 2021-03-17 14:04         ` Bertrand Marquis
  0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-17 14:04 UTC (permalink / raw)
  To: Julien Grall
  Cc: Xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 17 Mar 2021, at 11:20, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/03/2021 17:16, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 16 Mar 2021, at 15:27, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 15/03/2021 13:32, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>> 
>>>>> Hi all,
>>>>> 
>>>>> Last year, Arm released a whitepaper about a new category of speculation.
>>>>> (see [1] and [2]). In short, a processor may be able to speculate past
>>>>> some of the unconditional control flow instructions (e.g eret, smc, br).
>>>>> 
>>>>> In some of the cases, the registers will contain values controlled by
>>>>> the guest. While there is no known gadget afterwards, we still want to
>>>>> prevent any leakage in the future.
>>>>> 
>>>>> The mitigation is planned in two parts:
>>>>>   1) Arm provided patches for both GCC and LLVM to add speculation barrier
>>>>>   and remove problematic code sequence.
>>>>>   2) Inspection of assembly code and call to higher level (e.g smc in our case).
>>>>> 
>>>>> I still haven't looked at 1) and how to mitigate properly Arm32 (see
>>>>> patch #1) and SMC call. So this issue is not fully addressed.
>>>>> 
>>>>> Note that the ERET instruction was already addressed as part of XSA-312.
>>>> On my tests, this serie is breaking the arm64 build:
>>>> | aarch64-poky-linux-ld --sysroot=/home/bermar01/Development/xen-dev/build/profile-fvp-base.prj/tmp/work/fvp_base-poky-linux/xen/4.15+git1-r0/recipe-sysroot         -EL  --fix-cortex-a53-843419 --fix-cortex-a53-843419 -r -o built_in.o memcpy.o memcmp.o memmove.o memset.o memchr.o clear_page.o bitops.o find_next_bit.o strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
>>> 
>>> I can't see any build failure with the following GCC:
>>> 
>>> 42sh> aarch64-linux-gnu-gcc
>>> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
>>> Copyright (C) 2017 Free Software Foundation, Inc.
>>> This is free software; see the source for copying conditions.  There is NO
>>> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>> 
>>> AFAICT, there is also no compilation issue reported by gitlab:
>>> 
>>> https://gitlab.com/xen-project/patchew/xen/-/pipelines/269989894
>>> 
>>> What's the version of your compiler? Do you have steps to reproduce your setup?
>> You need to have earlyprintk enabled
>> I am using gcc 7.5.0:
>> aarch64-linux-gnu-gcc (Ubuntu/Linaro 7.5.0-3ubuntu1~18.04) 7.5.0
>> one configuration triggering the issue is using the default .config with the following items added:
>> CONFIG_SUPPRESS_DUPLICATE_SYMBOL_WARNINGS=y
>> CONFIG_DEBUG_LOCK_PROFILE=y
>> CONFIG_PERF_COUNTERS=y
>> CONFIG_PERF_ARRAYS=y
>> CONFIG_DEVICE_TREE_DEBUG=y
>> CONFIG_DEBUG_TRACE=y
>> CONFIG_EARLY_PRINTK_JUNO=y
>> CONFIG_EARLY_UART_PL011=y
>> CONFIG_EARLY_PRINTK=y
>> CONFIG_EARLY_UART_BASE_ADDRESS=0x7ff80000
>> CONFIG_EARLY_UART_PL011_BAUD_RATE=115200
>> CONFIG_EARLY_UART_INIT=y
>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc”
> 
> Thanks for providing the .config. I managed to reproduce it. So I removed "asm_defns.h" everywhere but forgot to include it in the "config.h" :/.
> 
> This small change fixed the error:
> 
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 51273b9db1fc..c7b77912013e 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -192,7 +192,7 @@ extern unsigned long frametable_virt_end;
> #define watchdog_enable()  ((void)0)
> 
> #if defined(__ASSEMBLY__) && !defined(__LINKER__)
> -#include <asm/asm-offsets.h>
> +#include <asm/asm_defns.h>
> #include <asm/macros.h>
> #endif
> 
> Would you still be happy to review the series before I send a v3?

Sure,

I will restart my tests with this change now and review the v2 once passed.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files
  2021-03-13 16:06 ` [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files Julien Grall
@ 2021-03-17 14:38   ` Bertrand Marquis
  2021-03-20 12:09     ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-17 14:38 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,


> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> In a follow-up patch we may want to automatically replace some
> mnemonics (such as ret) with a different sequence.
> 
> To ensure all the assembly files will include asm/macros.h it is best to
> automatically include it on single assembly. This can be done via
> config.h.
> 
> It was necessary to include a few more headers as dependency:
>  - <asm/asm_defns.h> to define sizeof_*
>  - <xen/page-size.h> which is already a latent issue given STACK_ORDER
>  rely on PAGE_SIZE.
> 
> Unfortunately the build system will use -D__ASSEMBLY__ when generating
> the linker script. A new option -D__LINKER__ is introduceed and used for
> the linker script to avoid including headers (such as asm/macros.h) that
> may not be compatible with the syntax.
> 
> Lastly, take the opportunity to remove both asm/asm-offsets.h and
> asm/macros.h from the various assembly files as they are now
> automagically included.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Everything is now building :-)

I could not find a better then your define as filtering out or undefining __ASSEMBLY__
is actually not working.

So with the fix from offset to defns:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/Makefile                | 2 +-
> xen/arch/arm/arm32/entry.S           | 1 -
> xen/arch/arm/arm32/head.S            | 1 -
> xen/arch/arm/arm32/proc-v7.S         | 1 -
> xen/arch/arm/arm64/debug-cadence.inc | 1 -
> xen/arch/arm/arm64/debug-pl011.inc   | 2 --
> xen/arch/arm/arm64/entry.S           | 2 --
> xen/arch/arm/arm64/head.S            | 2 --
> xen/arch/arm/arm64/smc.S             | 3 ---
> xen/include/asm-arm/config.h         | 6 ++++++
> 10 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 16e6523e2cc6..9ffc3f771c51 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -135,7 +135,7 @@ asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
> 	$(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $<
> 
> xen.lds: xen.lds.S
> -	$(CPP) -P $(a_flags) -MQ $@ -o $@ $<
> +	$(CPP) -P $(a_flags) -D__LINKER__ -MQ $@ -o $@ $<
> 
> dtb.o: $(CONFIG_DTB_FILE)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index b228d44b190c..f2f1bc7a3158 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -1,4 +1,3 @@
> -#include <asm/asm_defns.h>
> #include <asm/sysregs.h>
> #include <asm/regs.h>
> #include <asm/alternative.h>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index c404fa973e9b..9084023a6ed9 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -18,7 +18,6 @@
>  */
> 
> #include <asm/page.h>
> -#include <asm/asm_defns.h>
> #include <asm/early_printk.h>
> 
> #define ZIMAGE_MAGIC_NUMBER 0x016f2818
> diff --git a/xen/arch/arm/arm32/proc-v7.S b/xen/arch/arm/arm32/proc-v7.S
> index 46bfc7a9074c..1efde2d72da0 100644
> --- a/xen/arch/arm/arm32/proc-v7.S
> +++ b/xen/arch/arm/arm32/proc-v7.S
> @@ -17,7 +17,6 @@
>  * GNU General Public License for more details.
>  */
> 
> -#include <asm/asm_defns.h>
> #include <asm/arm32/processor.h>
> #include <asm/sysregs.h>
> 
> diff --git a/xen/arch/arm/arm64/debug-cadence.inc b/xen/arch/arm/arm64/debug-cadence.inc
> index 7df0abe4756f..0b6f2e094e18 100644
> --- a/xen/arch/arm/arm64/debug-cadence.inc
> +++ b/xen/arch/arm/arm64/debug-cadence.inc
> @@ -17,7 +17,6 @@
>  * GNU General Public License for more details.
>  */
> 
> -#include <asm/asm_defns.h>
> #include <asm/cadence-uart.h>
> 
> /*
> diff --git a/xen/arch/arm/arm64/debug-pl011.inc b/xen/arch/arm/arm64/debug-pl011.inc
> index 385deff49b1b..1928a2e3ffbb 100644
> --- a/xen/arch/arm/arm64/debug-pl011.inc
> +++ b/xen/arch/arm/arm64/debug-pl011.inc
> @@ -16,8 +16,6 @@
>  * GNU General Public License for more details.
>  */
> 
> -#include <asm/asm_defns.h>
> -
> /*
>  * PL011 UART initialization
>  * xb: register which containts the UART base address
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 175ea2981e72..ab9a65fc1475 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -1,6 +1,4 @@
> -#include <asm/asm_defns.h>
> #include <asm/current.h>
> -#include <asm/macros.h>
> #include <asm/regs.h>
> #include <asm/alternative.h>
> #include <asm/smccc.h>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 5d44667bd89d..fa7a3ffd2926 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -21,11 +21,9 @@
>  */
> 
> #include <asm/page.h>
> -#include <asm/asm_defns.h>
> #include <asm/early_printk.h>
> #include <efi/efierr.h>
> #include <asm/arm64/efibind.h>
> -#include <asm/arm64/macros.h>
> 
> #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
> #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
> diff --git a/xen/arch/arm/arm64/smc.S b/xen/arch/arm/arm64/smc.S
> index b0752be57e8f..91bae62dd4d2 100644
> --- a/xen/arch/arm/arm64/smc.S
> +++ b/xen/arch/arm/arm64/smc.S
> @@ -13,9 +13,6 @@
>  * GNU General Public License for more details.
>  */
> 
> -#include <asm/asm_defns.h>
> -#include <asm/macros.h>
> -
> /*
>  * void __arm_smccc_1_0_smc(register_t a0, register_t a1, register_t a2,
>  *                          register_t a3, register_t a4, register_t a5,
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index 5c10c755db46..51273b9db1fc 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -69,6 +69,7 @@
> #endif
> 
> #include <xen/const.h>
> +#include <xen/page-size.h>
> 
> /*
>  * Common ARM32 and ARM64 layout:
> @@ -190,6 +191,11 @@ extern unsigned long frametable_virt_end;
> #define watchdog_disable() ((void)0)
> #define watchdog_enable()  ((void)0)
> 
> +#if defined(__ASSEMBLY__) && !defined(__LINKER__)
> +#include <asm/asm-offsets.h>
> +#include <asm/macros.h>
> +#endif
> +
> #endif /* __ARM_CONFIG_H__ */
> /*
>  * Local variables:
> -- 
> 2.17.1
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-13 16:06 ` [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction Julien Grall
@ 2021-03-17 14:56   ` Bertrand Marquis
  2021-03-20 13:13     ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-17 14:56 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk

Hi Julien,

> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Some CPUs can speculate past a RET instruction and potentially perform
> speculative accesses to memory before processing the return.
> 
> There is no known gadget available after the RET instruction today.
> However some of the registers (such as in check_pending_guest_serror())
> may contain a value provided by the guest.
> 
> In order to harden the code, it would be better to add a speculation
> barrier after each RET instruction. The performance impact is meant to
> be negligeable as the speculation barrier is not meant to be
> architecturally executed.
> 
> Rather than manually inserting a speculation barrier, use a macro
> which overrides the mnemonic RET and replace with RET + SB. We need to
> use the opcode for RET to prevent any macro recursion.
> 
> This patch is only covering the assembly code. C code would need to be
> covered separately using the compiler support.
> 
> This is part of the work to mitigate straight-line speculation.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

The macro solution is definitely a great improvement compared to v1 and I could
confirm the presence of the sb in the generated code.

I also think that the mitigation on arm32/v7 would be messy to do.
Shall we mark v7/aarch32 as not security supported ?

Apart from this global question (which does not need to be answered in this serie):

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand


> 
> ---
> 
> It is not clear to me whether Armv7 (we don't officially support 32-bit
> hypervisor on Armv8) is also affected by straight-line speculation.
> 
> But the mitigation is a lot messier because opcode can be optionally
> executed. So this Arm32 is left alone for now.
> 
>    Changes in v2:
>        - Use a macro rather than inserting the speculation barrier
>        manually
>        - Remove mitigation for arm32
> ---
> xen/arch/arm/arm32/entry.S         |  1 +
> xen/arch/arm/arm32/lib/lib1funcs.S |  1 +
> xen/include/asm-arm/arm64/macros.h |  6 ++++++
> xen/include/asm-arm/macros.h       | 18 +++++++++---------
> 4 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
> index f2f1bc7a3158..d0a066484f13 100644
> --- a/xen/arch/arm/arm32/entry.S
> +++ b/xen/arch/arm/arm32/entry.S
> @@ -441,6 +441,7 @@ ENTRY(__context_switch)
> 
>         add     r4, r1, #VCPU_arch_saved_context
>         ldmia   r4, {r4 - sl, fp, sp, pc}       /* Load registers and return */
> +        sb
> 
> /*
>  * Local variables:
> diff --git a/xen/arch/arm/arm32/lib/lib1funcs.S b/xen/arch/arm/arm32/lib/lib1funcs.S
> index f1278bd6c139..8c33ffbbcc4c 100644
> --- a/xen/arch/arm/arm32/lib/lib1funcs.S
> +++ b/xen/arch/arm/arm32/lib/lib1funcs.S
> @@ -382,5 +382,6 @@ UNWIND(.save {lr})
> 	bl	__div0
> 	mov	r0, #0			@ About as wrong as it could be.
> 	ldr	pc, [sp], #8
> +	sb
> UNWIND(.fnend)
> ENDPROC(Ldiv0)
> diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
> index f981b4f43e84..4614394b3dd5 100644
> --- a/xen/include/asm-arm/arm64/macros.h
> +++ b/xen/include/asm-arm/arm64/macros.h
> @@ -21,6 +21,12 @@
>     ldr     \dst, [\dst, \tmp]
>     .endm
> 
> +    .macro  ret
> +        // ret opcode
> +        .inst 0xd65f03c0
> +        sb
> +    .endm
> +
> /*
>  * Register aliases.
>  */
> diff --git a/xen/include/asm-arm/macros.h b/xen/include/asm-arm/macros.h
> index 4833671f4ced..1aa373760f98 100644
> --- a/xen/include/asm-arm/macros.h
> +++ b/xen/include/asm-arm/macros.h
> @@ -5,6 +5,15 @@
> # error "This file should only be included in assembly file"
> #endif
> 
> +    /*
> +     * Speculative barrier
> +     * XXX: Add support for the 'sb' instruction
> +     */
> +    .macro sb
> +    dsb nsh
> +    isb
> +    .endm
> +
> #if defined (CONFIG_ARM_32)
> # include <asm/arm32/macros.h>
> #elif defined(CONFIG_ARM_64)
> @@ -20,13 +29,4 @@
>     .endr
>     .endm
> 
> -    /*
> -     * Speculative barrier
> -     * XXX: Add support for the 'sb' instruction
> -     */
> -    .macro sb
> -    dsb nsh
> -    isb
> -    .endm
> -
> #endif /* __ASM_ARM_MACROS_H */
> -- 
> 2.17.1
> 



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files
  2021-03-17 14:38   ` Bertrand Marquis
@ 2021-03-20 12:09     ` Julien Grall
  0 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2021-03-20 12:09 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk



On 17/03/2021 14:38, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> In a follow-up patch we may want to automatically replace some
>> mnemonics (such as ret) with a different sequence.
>>
>> To ensure all the assembly files will include asm/macros.h it is best to
>> automatically include it on single assembly. This can be done via
>> config.h.
>>
>> It was necessary to include a few more headers as dependency:
>>   - <asm/asm_defns.h> to define sizeof_*
>>   - <xen/page-size.h> which is already a latent issue given STACK_ORDER
>>   rely on PAGE_SIZE.
>>
>> Unfortunately the build system will use -D__ASSEMBLY__ when generating
>> the linker script. A new option -D__LINKER__ is introduceed and used for
>> the linker script to avoid including headers (such as asm/macros.h) that
>> may not be compatible with the syntax.
>>
>> Lastly, take the opportunity to remove both asm/asm-offsets.h and
>> asm/macros.h from the various assembly files as they are now
>> automagically included.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Everything is now building :-)
> 
> I could not find a better then your define as filtering out or undefining __ASSEMBLY__
> is actually not working.

Yes, unfortunately the linker is also relying on __ASSEMBLY__ for a few 
macros and to also remove the definitions of structure/function from 
headers that can be included either in C or assembly.

-D__LINKER__ was the best option I could come up with.

> 
> So with the fix from offset to defns:
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks! I will resend a new version with the fix fold in this patch.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-17 14:56   ` Bertrand Marquis
@ 2021-03-20 13:13     ` Julien Grall
  2021-03-26 11:13       ` Bertrand Marquis
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-20 13:13 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Xen.org security team

(+ Security)


On 17/03/2021 14:56, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Some CPUs can speculate past a RET instruction and potentially perform
>> speculative accesses to memory before processing the return.
>>
>> There is no known gadget available after the RET instruction today.
>> However some of the registers (such as in check_pending_guest_serror())
>> may contain a value provided by the guest.
>>
>> In order to harden the code, it would be better to add a speculation
>> barrier after each RET instruction. The performance impact is meant to
>> be negligeable as the speculation barrier is not meant to be
>> architecturally executed.
>>
>> Rather than manually inserting a speculation barrier, use a macro
>> which overrides the mnemonic RET and replace with RET + SB. We need to
>> use the opcode for RET to prevent any macro recursion.
>>
>> This patch is only covering the assembly code. C code would need to be
>> covered separately using the compiler support.
>>
>> This is part of the work to mitigate straight-line speculation.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> The macro solution is definitely a great improvement compared to v1 and I could
> confirm the presence of the sb in the generated code.

Thanks for testing! It is indeed a lot nicer and less error-prone. We 
can thansk Jan for the idea as he originally introduced it on x86 :).

> 
> I also think that the mitigation on arm32/v7 would be messy to do.

It is messy but not impossible :). Some of the assembly function could 
be rewritten in C to take advantage of the compiler mitigations.

I went through the paper again today. Straight-line mitigation only 
refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.

A recent submission to LLVM seems to suggest that Armv7 and AArch32 
Armv8 is also affected [2].

So I think we only need to care of unconditional return instruction 
(e.g. mov pc, lr).

For conditional return instructions, they would be treated as spectre v2 
which we already mitigate.

> Shall we mark v7/aarch32 as not security supported ?
This would have consequence beyond just speculation. There might be 
processor out which are not affected by straight-line speculation and we 
would not issue any security update for them. So I am not in favor with 
this approach.

What we could consider is mentioning in SUPPORT.MD that speculation 
attack using Straight-Line speculation are not security support on Arm 
(the 64-bit is not fully mitigated).

Cheers,

[1] https://reviews.llvm.org/D92395

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-20 13:13     ` Julien Grall
@ 2021-03-26 11:13       ` Bertrand Marquis
  2021-03-26 11:56         ` Julien Grall
  0 siblings, 1 reply; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-26 11:13 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Xen.org security team

Hi Julien,

(sorry for the delay)

> On 20 Mar 2021, at 13:13, Julien Grall <julien@xen.org> wrote:
> 
> (+ Security)
> 
> 
> On 17/03/2021 14:56, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>> 
>>> From: Julien Grall <jgrall@amazon.com>
>>> 
>>> Some CPUs can speculate past a RET instruction and potentially perform
>>> speculative accesses to memory before processing the return.
>>> 
>>> There is no known gadget available after the RET instruction today.
>>> However some of the registers (such as in check_pending_guest_serror())
>>> may contain a value provided by the guest.
>>> 
>>> In order to harden the code, it would be better to add a speculation
>>> barrier after each RET instruction. The performance impact is meant to
>>> be negligeable as the speculation barrier is not meant to be
>>> architecturally executed.
>>> 
>>> Rather than manually inserting a speculation barrier, use a macro
>>> which overrides the mnemonic RET and replace with RET + SB. We need to
>>> use the opcode for RET to prevent any macro recursion.
>>> 
>>> This patch is only covering the assembly code. C code would need to be
>>> covered separately using the compiler support.
>>> 
>>> This is part of the work to mitigate straight-line speculation.
>>> 
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> The macro solution is definitely a great improvement compared to v1 and I could
>> confirm the presence of the sb in the generated code.
> 
> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
> 
>> I also think that the mitigation on arm32/v7 would be messy to do.
> 
> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
> 
> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
> 
> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].

Thanks for the pointer :-)

> 
> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
> 
> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.

That would be a good idea but that would mean lots of invasive changes on armv7 and this is not the mostly tested architecture with Xen.
Anyway I am happy to help reviewing this if it is done.

> 
>> Shall we mark v7/aarch32 as not security supported ?
> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
> 
> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).

Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.

Anyway I agree with the principle to do this.

Cheers
Bertrand

> 
> Cheers,
> 
> [1] https://reviews.llvm.org/D92395
> 
> -- 
> Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-26 11:13       ` Bertrand Marquis
@ 2021-03-26 11:56         ` Julien Grall
  2021-03-26 13:07           ` Bertrand Marquis
  0 siblings, 1 reply; 15+ messages in thread
From: Julien Grall @ 2021-03-26 11:56 UTC (permalink / raw)
  To: Bertrand Marquis
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Xen.org security team



On 26/03/2021 11:13, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 20 Mar 2021, at 13:13, Julien Grall <julien@xen.org> wrote:
>>
>> (+ Security)
>>
>>
>> On 17/03/2021 14:56, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Some CPUs can speculate past a RET instruction and potentially perform
>>>> speculative accesses to memory before processing the return.
>>>>
>>>> There is no known gadget available after the RET instruction today.
>>>> However some of the registers (such as in check_pending_guest_serror())
>>>> may contain a value provided by the guest.
>>>>
>>>> In order to harden the code, it would be better to add a speculation
>>>> barrier after each RET instruction. The performance impact is meant to
>>>> be negligeable as the speculation barrier is not meant to be
>>>> architecturally executed.
>>>>
>>>> Rather than manually inserting a speculation barrier, use a macro
>>>> which overrides the mnemonic RET and replace with RET + SB. We need to
>>>> use the opcode for RET to prevent any macro recursion.
>>>>
>>>> This patch is only covering the assembly code. C code would need to be
>>>> covered separately using the compiler support.
>>>>
>>>> This is part of the work to mitigate straight-line speculation.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> The macro solution is definitely a great improvement compared to v1 and I could
>>> confirm the presence of the sb in the generated code.
>>
>> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
>>
>>> I also think that the mitigation on arm32/v7 would be messy to do.
>>
>> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
>>
>> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
>>
>> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].
> 
> Thanks for the pointer :-)
> 
>>
>> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
>>
>> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.
> 
> That would be a good idea but that would mean lots of invasive changes on armv7 and
It is not quite clear which change you think is invasive... The change 
for adding a barrier after all unconditional return instruction is 
pretty straight-forward.

Regarding conditional return instructions, then is nothing to do for 
straight-line speculation.

> this is not the mostly tested architecture with Xen.
To me this looks very subjective, how do you define "mostly tested"?

 From Xen Project perspective, we run the same test suite on arm64 and 
arm32 multiple time daily. I couldn't say the same for some of the Arm 
drivers in the tree.

> Anyway I am happy to help reviewing this if it is done.
> 
>>
>>> Shall we mark v7/aarch32 as not security supported ?
>> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
>>
>> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).
> 
> Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.

I am open for the wording :).

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction
  2021-03-26 11:56         ` Julien Grall
@ 2021-03-26 13:07           ` Bertrand Marquis
  0 siblings, 0 replies; 15+ messages in thread
From: Bertrand Marquis @ 2021-03-26 13:07 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Volodymyr Babchuk,
	Xen.org security team

Hi,

> On 26 Mar 2021, at 11:56, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 26/03/2021 11:13, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 20 Mar 2021, at 13:13, Julien Grall <julien@xen.org> wrote:
>>> 
>>> (+ Security)
>>> 
>>> 
>>> On 17/03/2021 14:56, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 13 Mar 2021, at 16:06, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>> 
>>>>> Some CPUs can speculate past a RET instruction and potentially perform
>>>>> speculative accesses to memory before processing the return.
>>>>> 
>>>>> There is no known gadget available after the RET instruction today.
>>>>> However some of the registers (such as in check_pending_guest_serror())
>>>>> may contain a value provided by the guest.
>>>>> 
>>>>> In order to harden the code, it would be better to add a speculation
>>>>> barrier after each RET instruction. The performance impact is meant to
>>>>> be negligeable as the speculation barrier is not meant to be
>>>>> architecturally executed.
>>>>> 
>>>>> Rather than manually inserting a speculation barrier, use a macro
>>>>> which overrides the mnemonic RET and replace with RET + SB. We need to
>>>>> use the opcode for RET to prevent any macro recursion.
>>>>> 
>>>>> This patch is only covering the assembly code. C code would need to be
>>>>> covered separately using the compiler support.
>>>>> 
>>>>> This is part of the work to mitigate straight-line speculation.
>>>>> 
>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>> The macro solution is definitely a great improvement compared to v1 and I could
>>>> confirm the presence of the sb in the generated code.
>>> 
>>> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
>>> 
>>>> I also think that the mitigation on arm32/v7 would be messy to do.
>>> 
>>> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
>>> 
>>> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
>>> 
>>> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].
>> Thanks for the pointer :-)
>>> 
>>> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
>>> 
>>> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.
>> That would be a good idea but that would mean lots of invasive changes on armv7 and
> It is not quite clear which change you think is invasive... The change for adding a barrier after all unconditional return instruction is pretty straight-forward.
> 
> Regarding conditional return instructions, then is nothing to do for straight-line speculation.
> 
>> this is not the mostly tested architecture with Xen.
> To me this looks very subjective, how do you define "mostly tested"?
> 
> From Xen Project perspective, we run the same test suite on arm64 and arm32 multiple time daily. I couldn't say the same for some of the Arm drivers in the tree.

All together i just want to say that I have no testing capacities for arm32 (in hardware and persons) and the “user base” for it might not be huge (but I might be wrong here).

If you think that there is enough testing for it available or other might be able to test it then no problem for me, I will help on the review side.

Cheers
Bertrand

> 
>> Anyway I am happy to help reviewing this if it is done.
>>> 
>>>> Shall we mark v7/aarch32 as not security supported ?
>>> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
>>> 
>>> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).
>> Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.
> 
> I am open for the wording :).
> 
> Cheers,
> 
> -- 
> Julien Grall
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-03-26 13:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-13 16:06 [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Julien Grall
2021-03-13 16:06 ` [PATCH for-next v2 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files Julien Grall
2021-03-17 14:38   ` Bertrand Marquis
2021-03-20 12:09     ` Julien Grall
2021-03-13 16:06 ` [PATCH for-next v2 2/2] xen/arm64: Place a speculation barrier following an ret instruction Julien Grall
2021-03-17 14:56   ` Bertrand Marquis
2021-03-20 13:13     ` Julien Grall
2021-03-26 11:13       ` Bertrand Marquis
2021-03-26 11:56         ` Julien Grall
2021-03-26 13:07           ` Bertrand Marquis
2021-03-15 13:32 ` [PATCH for-next v2 0/2] xen/arm: Mitigate straight-line speculation Bertrand Marquis
2021-03-16 15:27   ` Julien Grall
2021-03-16 17:16     ` Bertrand Marquis
2021-03-17 11:20       ` Julien Grall
2021-03-17 14:04         ` Bertrand Marquis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).