[for-next,v2,2/2] xen/arm64: Place a speculation barrier following an ret instruction

Commit Message

Julien Grall March 13, 2021, 4:06 p.m. UTC

From: Julien Grall <[e-mail]>

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 <[e-mail]>

---

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.

But the mitigation is a lot messier because opcode can be optionally
executed. So this Arm32 is left alone for now.

    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(-)

Comments

Bertrand Marquis March 17, 2021, 2:56 p.m. UTC | #1

Hi Julien,

> On 13 Mar 2021, at 16:06, Julien Grall <[e-mail]> wrote:
> 
> From: Julien Grall <[e-mail]>
> 
> 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 <[e-mail]>

The macro solution is definitely a great improvement compared to v1 and I could
confirm the presence of the sb in the generated code.

I also think that the mitigation on arm32/v7 would be messy to do.
Shall we mark v7/aarch32 as not security supported ?

Apart from this global question (which does not need to be answered in this serie):

Reviewed-by: Bertrand Marquis <[e-mail]>

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.
> 
> But the mitigation is a lot messier because opcode can be optionally
> executed. So this Arm32 is left alone for now.
> 
>    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
>

Julien Grall March 20, 2021, 1:13 p.m. UTC | #2

(+ Security)


On 17/03/2021 14:56, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

> 
>> On 13 Mar 2021, at 16:06, Julien Grall <[e-mail]> wrote:
>>
>> From: Julien Grall <[e-mail]>
>>
>> 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 <[e-mail]>
> 
> The macro solution is definitely a great improvement compared to v1 and I could
> confirm the presence of the sb in the generated code.

Thanks for testing! It is indeed a lot nicer and less error-prone. We 
can thansk Jan for the idea as he originally introduced it on x86 :).

> 
> I also think that the mitigation on arm32/v7 would be messy to do.

It is messy but not impossible :). Some of the assembly function could 
be rewritten in C to take advantage of the compiler mitigations.

I went through the paper again today. Straight-line mitigation only 
refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.

A recent submission to LLVM seems to suggest that Armv7 and AArch32 
Armv8 is also affected [2].

So I think we only need to care of unconditional return instruction 
(e.g. mov pc, lr).

For conditional return instructions, they would be treated as spectre v2 
which we already mitigate.

> Shall we mark v7/aarch32 as not security supported ?
This would have consequence beyond just speculation. There might be 
processor out which are not affected by straight-line speculation and we 
would not issue any security update for them. So I am not in favor with 
this approach.

What we could consider is mentioning in SUPPORT.MD that speculation 
attack using Straight-Line speculation are not security support on Arm 
(the 64-bit is not fully mitigated).

Cheers,

[1] https://reviews.llvm.org/D92395

Bertrand Marquis March 26, 2021, 11:13 a.m. UTC | #3

Hi Julien,

(sorry for the delay)

> On 20 Mar 2021, at 13:13, Julien Grall <[e-mail]> wrote:
> 
> (+ Security)
> 
> 
> On 17/03/2021 14:56, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 13 Mar 2021, at 16:06, Julien Grall <[e-mail]> wrote:
>>> 
>>> From: Julien Grall <[e-mail]>
>>> 
>>> 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 <[e-mail]>
>> The macro solution is definitely a great improvement compared to v1 and I could
>> confirm the presence of the sb in the generated code.
> 
> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
> 
>> I also think that the mitigation on arm32/v7 would be messy to do.
> 
> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
> 
> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
> 
> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].

Thanks for the pointer :-)

> 
> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
> 
> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.

That would be a good idea but that would mean lots of invasive changes on armv7 and this is not the mostly tested architecture with Xen.
Anyway I am happy to help reviewing this if it is done.

> 
>> Shall we mark v7/aarch32 as not security supported ?
> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
> 
> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).

Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.

Anyway I agree with the principle to do this.

Cheers
Bertrand

> 
> Cheers,
> 
> [1] https://reviews.llvm.org/D92395
> 
> -- 
> Julien Grall

Julien Grall March 26, 2021, 11:56 a.m. UTC | #4

On 26/03/2021 11:13, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 20 Mar 2021, at 13:13, Julien Grall <[e-mail]> wrote:
>>
>> (+ Security)
>>
>>
>> On 17/03/2021 14:56, Bertrand Marquis wrote:
>>> Hi Julien,
>>
>> Hi Bertrand,
>>
>>>> On 13 Mar 2021, at 16:06, Julien Grall <[e-mail]> wrote:
>>>>
>>>> From: Julien Grall <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>> The macro solution is definitely a great improvement compared to v1 and I could
>>> confirm the presence of the sb in the generated code.
>>
>> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
>>
>>> I also think that the mitigation on arm32/v7 would be messy to do.
>>
>> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
>>
>> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
>>
>> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].
> 
> Thanks for the pointer :-)
> 
>>
>> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
>>
>> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.
> 
> That would be a good idea but that would mean lots of invasive changes on armv7 and
It is not quite clear which change you think is invasive... The change 
for adding a barrier after all unconditional return instruction is 
pretty straight-forward.

Regarding conditional return instructions, then is nothing to do for 
straight-line speculation.

> this is not the mostly tested architecture with Xen.
To me this looks very subjective, how do you define "mostly tested"?

 From Xen Project perspective, we run the same test suite on arm64 and 
arm32 multiple time daily. I couldn't say the same for some of the Arm 
drivers in the tree.

> Anyway I am happy to help reviewing this if it is done.
> 
>>
>>> Shall we mark v7/aarch32 as not security supported ?
>> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
>>
>> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).
> 
> Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.

I am open for the wording :).

Cheers,

Bertrand Marquis March 26, 2021, 1:07 p.m. UTC | #5

Hi,

> On 26 Mar 2021, at 11:56, Julien Grall <[e-mail]> wrote:
> 
> 
> 
> On 26/03/2021 11:13, Bertrand Marquis wrote:
>> Hi Julien,
> 
> Hi Bertrand,
> 
>>> On 20 Mar 2021, at 13:13, Julien Grall <[e-mail]> wrote:
>>> 
>>> (+ Security)
>>> 
>>> 
>>> On 17/03/2021 14:56, Bertrand Marquis wrote:
>>>> Hi Julien,
>>> 
>>> Hi Bertrand,
>>> 
>>>>> On 13 Mar 2021, at 16:06, Julien Grall <[e-mail]> wrote:
>>>>> 
>>>>> From: Julien Grall <[e-mail]>
>>>>> 
>>>>> 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 <[e-mail]>
>>>> The macro solution is definitely a great improvement compared to v1 and I could
>>>> confirm the presence of the sb in the generated code.
>>> 
>>> Thanks for testing! It is indeed a lot nicer and less error-prone. We can thansk Jan for the idea as he originally introduced it on x86 :).
>>> 
>>>> I also think that the mitigation on arm32/v7 would be messy to do.
>>> 
>>> It is messy but not impossible :). Some of the assembly function could be rewritten in C to take advantage of the compiler mitigations.
>>> 
>>> I went through the paper again today. Straight-line mitigation only refers to unconditional control flow change (e.g. RET) on AArch64 Armv8.
>>> 
>>> A recent submission to LLVM seems to suggest that Armv7 and AArch32 Armv8 is also affected [2].
>> Thanks for the pointer :-)
>>> 
>>> So I think we only need to care of unconditional return instruction (e.g. mov pc, lr).
>>> 
>>> For conditional return instructions, they would be treated as spectre v2 which we already mitigate.
>> That would be a good idea but that would mean lots of invasive changes on armv7 and
> It is not quite clear which change you think is invasive... The change for adding a barrier after all unconditional return instruction is pretty straight-forward.
> 
> Regarding conditional return instructions, then is nothing to do for straight-line speculation.
> 
>> this is not the mostly tested architecture with Xen.
> To me this looks very subjective, how do you define "mostly tested"?
> 
> From Xen Project perspective, we run the same test suite on arm64 and arm32 multiple time daily. I couldn't say the same for some of the Arm drivers in the tree.

All together i just want to say that I have no testing capacities for arm32 (in hardware and persons) and the “user base” for it might not be huge (but I might be wrong here).

If you think that there is enough testing for it available or other might be able to test it then no problem for me, I will help on the review side.

Cheers
Bertrand

> 
>> Anyway I am happy to help reviewing this if it is done.
>>> 
>>>> Shall we mark v7/aarch32 as not security supported ?
>>> This would have consequence beyond just speculation. There might be processor out which are not affected by straight-line speculation and we would not issue any security update for them. So I am not in favor with this approach.
>>> 
>>> What we could consider is mentioning in SUPPORT.MD that speculation attack using Straight-Line speculation are not security support on Arm (the 64-bit is not fully mitigated).
>> Weird to say “not security supported” maybe saying not mitigated by Xen would be more clear.
> 
> I am open for the wording :).
> 
> Cheers,
> 
> -- 
> Julien Grall
>

Patch

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 */