core: arm32: sm: invalidate branch predictor #2047

jenswi-linaro · 2018-01-04T07:03:26Z

If built with internal secure monitor, invalidates branch predictor
on non-secure entry.
If built for usage with ARM-TF, invalidates branch predictor on
entry.

Fixes CVE-2017-5715

Signed-off-by: Jens Wiklander [email protected]

jforissier · 2018-01-04T15:58:35Z

core/arch/arm/sm/sm_a32.S

@@ -166,6 +166,10 @@ UNWIND(	.cantunwind)
 	b	.sm_exit

 .smc_from_nsec:
+	/* Invalidate the branch predictor. */
+	write_bpiallis


Why Inner Shareable? Wouldn't write_bpiall be enough?

You're right, I'll fix.

This is late to apply the fix as the BTB speculation can occur on the unconditional branch at Line 258. BPIALL needs to be done prior to any branch instruction in Secure world. Its not easy to do this but we have an upcoming patch in ARM TF which will demonstrate this.

@soby-mathew , so you are saying this should be done before actually doing any jump from the vector table. Will this also affect the thread vector table? I'd guess that it doesn't affect that, agree?

Interesting, does it involve multiple vectors?

@jenswi-linaro, the way we do it is to have ORR (Or ADD) instruction at each vector entry to encode a bit pattern into an available General Purpose register. We use the sp for this and ensure the stack/whatever sp is pointing to is aligned so that the LSB can be used for the bit pattern encoding. Then the BPIALL is done and the bit pattern is extracted to get the vector entry. Finally we jump to the correct handler corresponding to the vector entry.

If the sp is pointing to address that is naturally aligned to 256 bytes then ORR can be used to encode the bit pattern (one bit per vector entry). Else ADD can be used to encode values from 0 to 7 for each vector entry and the sp needs to point to an address that is 8 byte aligned.

That's the general idea.

Aha, clever. Thanks, I'll do that.

All branches are subject to prediction on some Arm CPUs.

ARM Technical Reference Manual for A57 on predicted instructions:

6.5.1 Predicted and non-predicted instructions
This section describes the instructions that the processor predicts.

Unless otherwise specified, the list applies to A32, T32, and A64 instructions. As a general rule, the branch prediction hardware predicts all branch instructions regardless of the addressing mode, including:

Conditional branches.

Unconditional branches.

Indirect branches.

Branches that switch between ARM and Thumb states.

PC destination data processing operations.

BXJ, because of the inclusion of the trivial Jazelle implementation, this degenerates to a BX instruction. There is no BXJ instruction in A64.

However, the following branch instructions are not predicted:

AArch32 instructions with the S suffix are not predicted because they are typically used when returning from exceptions and have side effects that can change privilege mode and Security state.

All mode or Exception level changing instructions.

In Thumb state, you can make a branch that is normally encoded as unconditional conditional by including an If-Then (IT) block. It is then treated as a normal conditional branch.

see http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0488h/way1382448710808.html

[oops - missed that Soby had already mentioned this in his comment]

Jens - we're also testing a _version that has less constraint on the SP alignment and only needs 3 bits of SP to encode the vector before invalidating the branch predictor.

@athoelke thanks for the reference, quite helpful.

I suppose this section will need some rework however:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0488h/way1382448710808.html
;-)

jforissier · 2018-01-04T16:22:02Z

core/arch/arm/kernel/thread_a32.S

+	.macro maybe_invalidate_bp
+#ifdef CFG_WITH_ARM_TRUSTED_FW
+	/* Invalidate the branch predictor. */
+	write_bpiallis


Why is BP invalidate needed at all in this file when CFG_WITH_ARM_TRUSTED_FW=y? Isn't EL3 supposed to take care of it if needed? (I'm referring to ARM-TF pull request 1214).

I was asked to do it like this, I guess this was before ARM-software/arm-trusted-firmware#1214 existed.
@jbech-linaro, do you know if this is still needed?

I think @jforissier is right here, we don't need to do it if it is done by Arm-TF. We need to make the branch invalidate when we are running our own monitor (and therefore responsible for the switch).

OK, let's drop this.

jenswi-linaro · 2018-01-04T17:20:49Z

Update
I'll update the commit message when squashing.

jbech-linaro · 2018-01-04T17:49:56Z

Reviewed-by: Joakim Bech <[email protected]>

jforissier · 2018-01-05T07:42:48Z

What about sm_fiq_entry?

jenswi-linaro · 2018-01-05T10:16:21Z

@jforissier, I'll fix sm_fiq_entry() too.

jenswi-linaro · 2018-01-05T12:59:17Z

Update

soby-mathew

Looks good. The only comment being introducing a build flag so to apply this only for affected platforms ?

jenswi-linaro · 2018-01-05T15:19:41Z

Update, add config flag default enabled for this workaround.

jforissier · 2018-01-05T15:29:39Z

core/arch/arm/arm.mk

@@ -31,6 +31,12 @@ platform-hard-float-enabled := y
 endif
 endif

+# Adds protection against CVE-2017-5715 also know as Spectre
+# (https://spectreattack.com)
+# See also https://developer.arm.com/-/media/Files/pdf/Cache_Speculation_Side-channels.pdf?revision=966364ce-10aa-4580-8431-7e4ed42fb90b&la=en


You may shorten the URL, the parameters are useless

jenswi-linaro · 2018-01-05T15:32:24Z

Update

jforissier · 2018-01-05T16:03:25Z

Reviewed-by: Jerome Forissier <[email protected]>

jbech-linaro · 2018-01-08T10:06:56Z

Reviewed-by: Joakim Bech <[email protected]>

jforissier · 2018-01-08T10:30:10Z

What about entry via svc? I would think that path should be protected, too.

If build with secure monitor and CFG_CORE_WORKAROUND_SPECTRE_BP=y invalidate branch predictor on non-secure entry. Fixes CVE-2017-5715 Reviewed-by: Jerome Forissier <[email protected]> Reviewed-by: Joakim Bech <[email protected]> Signed-off-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2018-01-08T10:33:25Z

I'll create another PR for svc

jforissier · 2018-01-08T10:34:10Z

OK, sounds good.

jenswi-linaro · 2018-01-08T10:34:24Z

Rebased, squashed and tags applied.

jforissier · 2018-01-12T09:22:13Z

core/arch/arm/sm/sm_a32.S

+	nop			/* 0:000 FIQ			*/
+
+	/* Invalidate the branch predictor for the current processor. */
+	write_bpiall


Will this work on Cortex-A15?
#2055 (comment)

As far as I understand, not as intended at least.

BPIALL is not effective in invalidating the branch predictor on A15. An ICALLU with the ACTLR[0] set to 1 need to be done on Cortex-A15. The ARM TF advisory has been updated with this info. This would also mean the fix needs to detect the CPU type at runtime and apply the appropriate workaround.

Addressed in #2065

jenswi-linaro mentioned this pull request Jan 4, 2018

Cve 2017 5754 #2048

Merged

jforissier reviewed Jan 4, 2018

View reviewed changes

soby-mathew reviewed Jan 5, 2018

View reviewed changes

jforissier reviewed Jan 5, 2018

View reviewed changes

jenswi-linaro force-pushed the cve-2017-5715 branch from 2a9d642 to 0b77e33 Compare January 8, 2018 10:33

jforissier merged commit 3bc90f3 into OP-TEE:master Jan 8, 2018

jenswi-linaro deleted the cve-2017-5715 branch January 8, 2018 12:43

jforissier changed the title ~~core: arm32: invalidate branch predictor~~ core: arm32: sm: invalidate branch predictor Jan 12, 2018

jforissier reviewed Jan 12, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: arm32: sm: invalidate branch predictor #2047

core: arm32: sm: invalidate branch predictor #2047

jenswi-linaro commented Jan 4, 2018

jforissier Jan 4, 2018

jenswi-linaro Jan 4, 2018

soby-mathew Jan 4, 2018

jbech-linaro Jan 4, 2018

jenswi-linaro Jan 4, 2018

soby-mathew Jan 5, 2018

jenswi-linaro Jan 5, 2018

athoelke Jan 5, 2018

athoelke Jan 5, 2018 •

edited

Loading

jforissier Jan 5, 2018

jforissier Jan 4, 2018

jenswi-linaro Jan 4, 2018

jbech-linaro Jan 4, 2018

jenswi-linaro Jan 4, 2018

jenswi-linaro commented Jan 4, 2018

jbech-linaro commented Jan 4, 2018

jforissier commented Jan 5, 2018

jenswi-linaro commented Jan 5, 2018

jenswi-linaro commented Jan 5, 2018

soby-mathew left a comment

jenswi-linaro commented Jan 5, 2018

jforissier Jan 5, 2018

jenswi-linaro commented Jan 5, 2018

jforissier commented Jan 5, 2018

jbech-linaro commented Jan 8, 2018

jforissier commented Jan 8, 2018

jenswi-linaro commented Jan 8, 2018

jforissier commented Jan 8, 2018

jenswi-linaro commented Jan 8, 2018

jforissier Jan 12, 2018

jenswi-linaro Jan 12, 2018

soby-mathew Jan 12, 2018

jenswi-linaro Jan 12, 2018

core: arm32: sm: invalidate branch predictor #2047

core: arm32: sm: invalidate branch predictor #2047

Conversation

jenswi-linaro commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athoelke Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Jan 4, 2018

jbech-linaro commented Jan 4, 2018

jforissier commented Jan 5, 2018

jenswi-linaro commented Jan 5, 2018

jenswi-linaro commented Jan 5, 2018

soby-mathew left a comment

Choose a reason for hiding this comment

jenswi-linaro commented Jan 5, 2018

Choose a reason for hiding this comment

jenswi-linaro commented Jan 5, 2018

jforissier commented Jan 5, 2018

jbech-linaro commented Jan 8, 2018

jforissier commented Jan 8, 2018

jenswi-linaro commented Jan 8, 2018

jforissier commented Jan 8, 2018

jenswi-linaro commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athoelke Jan 5, 2018 •

edited

Loading