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

[PATCH v4] 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.

Note that the definition of the macros sb needs to be moved earlier in
asm-arm/macros.h so it can be used by the new macro.

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.
The LLVM website suggests it is: https://reviews.llvm.org/D92395

For now only focus on arm64.

    Changes in v4:
        - Remove Bertand's reviewed-by
        - Use /* ... */ rather than // for comments
        - Remove arm32 changes
        - Explain why the macro sb is moved around

    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/include/asm-arm/arm64/macros.h |  6 ++++++
 xen/include/asm-arm/macros.h       | 18 +++++++++---------
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
index f981b4f43e84..5ad66efd6ba4 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 v4] xen/arm64: Place a speculation barrier following an ret instruction

[Bertrand Marquis]

Hi Julien,

> On 18 Apr 2021, at 19:03, 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.
> 
> Note that the definition of the macros sb needs to be moved earlier in
> asm-arm/macros.h so it can be used by the new macro.
> 
> 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>

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.
> The LLVM website suggests it is: https://reviews.llvm.org/D92395
> 
> For now only focus on arm64.
> 
>    Changes in v4:
>        - Remove Bertand's reviewed-by
>        - Use /* ... */ rather than // for comments
>        - Remove arm32 changes
>        - Explain why the macro sb is moved around
> 
>    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/include/asm-arm/arm64/macros.h |  6 ++++++
> xen/include/asm-arm/macros.h       | 18 +++++++++---------
> 2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/asm-arm/arm64/macros.h b/xen/include/asm-arm/arm64/macros.h
> index f981b4f43e84..5ad66efd6ba4 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 v4] xen/arm64: Place a speculation barrier following an ret instruction

[Stefano Stabellini]

On Mon, 19 Apr 2021, Bertrand Marquis wrote:
> Hi Julien,
> 
> > On 18 Apr 2021, at 19:03, 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.
> > 
> > Note that the definition of the macros sb needs to be moved earlier in
> > asm-arm/macros.h so it can be used by the new macro.
> > 
> > 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>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Julien Grall]

Hi,

On 19/04/2021 19:24, Stefano Stabellini wrote:
> On Mon, 19 Apr 2021, Bertrand Marquis wrote:
>> Hi Julien,
>>
>>> On 18 Apr 2021, at 19:03, 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.
>>>
>>> Note that the definition of the macros sb needs to be moved earlier in
>>> asm-arm/macros.h so it can be used by the new macro.
>>>
>>> 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>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks both! I have committed the patch.

Cheers,

-- 
Julien Grall