[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hi Stefano,

On 14/04/2021 00:03, Stefano Stabellini wrote:
On Thu, 1 Apr 2021, Julien Grall wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>

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

---

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.