| Message ID | 20210418180334.7829-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] xen/arm64: Place a speculation barrier following an ret instruction | expand |
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 >
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>
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,
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 */