| Message ID | 20210401164444.20377-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Mitigate straight-line speculation | expand |
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 >
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,
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 */