[PATCH v3 0/2] xen/arm: Mitigate straight-line speculation

[PATCH v3 0/2] xen/arm: Mitigate straight-line speculation

[Julien Grall]

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

[PATCH v3 1/2] xen/arm: Include asm/asm-offsets.h and asm/macros.h on every assembly files

[Julien Grall]

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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

---
    Changes in v3:
        - Include <asm/asm_defns.h> rather than <asm/asm-offsets.h>
        - Add Bertrand's reviewed-by tag

    Changes in v2:
        - Patch added
---
 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 46e6a95fec98..ca75f1040dcc 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -134,7 +134,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: $(patsubst "%",%,$(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 50f019ed98ea..7178865f48c3 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 f38a8dfca7dc..aa1f88c76498 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..c7b77912013e 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_defns.h>
+#include <asm/macros.h>
+#endif
+
 #endif /* __ARM_CONFIG_H__ */
 /*
  * Local variables:
-- 
2.17.1

[PATCH v3 2/2] xen/arm64: Place a speculation barrier following an ret instruction

[Julien Grall]

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>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.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.
The LLVM website suggests it is: https://reviews.llvm.org/D92395

For now only focus on arm64.

    Changes in v3:
        -  Add Bertrand's reviewed-by

    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

Re: [PATCH v3 2/2] xen/arm64: Place a speculation barrier following an ret instruction

[Stefano Stabellini]

On Thu, 1 Apr 2021, Julien Grall 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>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.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.
> The LLVM website suggests it is: https://reviews.llvm.org/D92395
> 
> For now only focus on arm64.
> 
>     Changes in v3:
>         -  Add Bertrand's reviewed-by
> 
>     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

This series is very nice Julien! You can add my acked-by to both patches
and also commit them.

One minor request: could you please replace this comment with

/* ret opcode */

on commit?  // is not part of the coding style.


> +        .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
> 

Re: [PATCH v3 2/2] xen/arm64: Place a speculation barrier following an ret instruction

[Julien Grall]

Hi Stefano,

On 14/04/2021 00:03, Stefano Stabellini wrote:
> On Thu, 1 Apr 2021, Julien Grall 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>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.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.
>> The LLVM website suggests it is: https://reviews.llvm.org/D92395
>>
>> For now only focus on arm64.
>>
>>      Changes in v3:
>>          -  Add Bertrand's reviewed-by
>>
>>      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
> 
> This series is very nice Julien! You can add my acked-by to both patches
> and also commit them.

I have realized that I left a couple of arm32 specific code in the 
patch. I will commit the first one and respin this one.

> 
> One minor request: could you please replace this comment with
> 
> /* ret opcode */
> 
> on commit?  // is not part of the coding style.

I have modified it in the new version.

Cheers,

-- 
Julien Grall