Cve 2017 5715 2 #2055

jenswi-linaro · 2018-01-08T13:25:32Z

This pull request is done on top of #2048 to avoid unnecessary conflicts when finally merging everything. The commit to review here is core: arm32: thread: invalidate branch predictor.

jforissier

With or without my comment addressed, and for "core: arm32: thread: invalidate branch predictor" only:
Reviewed-by: Jerome Forissier <[email protected]>

jforissier · 2018-01-08T14:11:11Z

core/arch/arm/kernel/thread_a32.S

+	 * The idea is to form a specific bit pattern in the lowest three
+	 * bits of SP depending on which entry in the vector we enter via.
+	 * This is done by adding 1 to SP in each entry but the last.
+	 */


I think this deserves a few words to explain that the BPIALL has to be executed before any branch instruction is executed, hence the slightly convoluted code. I know I could/should have made the same comment for the sm commit ;-)

I'll add something.

minor stuff: s/doesn't/does not/ , s/it's/it is/.

Sentence seems wrong to me: It doesn't matter if it's a conditional or an unconditional branch speculation can still occur..

Perhaps it's a missing comma. How about:
It doesn't matter if it's a conditional or an unconditional branch, speculation can still occur.

jforissier · 2018-01-08T14:13:54Z

What about 64-bits?

jenswi-linaro · 2018-01-08T14:24:15Z

You're right I forgot about the 64-bit stuff. I'll add another patch here taking care of that.

jenswi-linaro · 2018-01-08T14:34:18Z

Hmm, the 64-bit branch predictor invalidation is a bit more complicated. I'll take that in a separate pull request.

jenswi-linaro · 2018-01-08T14:48:33Z

Commit updated, tag applied to core: arm32: thread: invalidate branch predictor only.

etienne-lms · 2018-01-08T15:21:39Z

core/arch/arm/include/mm/core_mmu.h

@@ -74,6 +77,11 @@
 #define CFG_TEE_RAM_VA_SIZE		CORE_MMU_PGDIR_SIZE
 #endif

+#ifdef CFG_WITH_LPAE
+#define CORE_MMU_L1_TBL_OFFSET		(CFG_TEE_CORE_NB_CORE * 4 * 8)


* 4 * 8 looks like magic, especially compared to how NUM_L1_ENTRIES is defined in core_mmu_lpae.c. Deserves a bit of explanation: why 4 64bit wide mmu descriptors ?

Regarding the armv7, the comment is straightforward: we need a full L1 table.

Please make these comments in #2048 instead

etienne-lms · 2018-01-08T15:22:05Z

core/arch/arm/kernel/thread.c

+
+	v = (vaddr_t)thread_vect_table;
+	thread_user_kcode_va = ROUNDDOWN(v, CORE_MMU_USER_CODE_SIZE);
+	thread_user_kcode_size = CORE_MMU_USER_CODE_SIZE * 2;


why 2 regions (likely pages)?

Please make these comments in #2048 instead

jenswi-linaro · 2018-01-09T14:06:15Z

core/arch/arm/kernel/thread_a32.S

@@ -748,6 +748,56 @@ END_FUNC thread_unwind_user_mode
 FUNC thread_vect_table , :
 UNWIND(	.fnstart)
 UNWIND(	.cantunwind)
+#ifdef CFG_CORE_WORKAROUND_SPECTRE_BP


I'm wondering if we shouldn't use a different config option here as we're protecting against a different attack and also the defense will be quite expensive in AArch64.

Agreed. CFG_CORE_WORKAROUND_SPECTRE_BP_SEC maybe? (rationale: still Spectre branch prediction attack but entirely within secure world hence _SEC).

Sounds good

jenswi-linaro · 2018-01-09T14:41:56Z

Commit core: arm32: thread: invalidate branch predictor updated and rebased on top of #2048

jforissier · 2018-01-09T14:51:30Z

Fine with me

danh-arm · 2018-01-11T10:42:55Z

We've discovered that the BPIALL workaround is not effective in invalidating the branch predictor for Cortex-A15. An alternative workaround is described in the updated TF security advisory:

https://github.com/ARM-software/arm-trusted-firmware/wiki/Arm-Trusted-Firmware-Security-Advisory-TFV-6

That will affect your implementation here. Note we plan to push an example implementation of this in our SP_MIN payload in the coming days.

Also, it would be good if this workaround was not applied to unaffected CPUs (e.g. Cortex-A32, Cortex-A7 and Cortex-A5).

jbech-linaro · 2018-01-12T08:35:07Z

We've discovered that the BPIALL ...
... That will affect your implementation here. Note we plan to push an example implementation of this in our SP_MIN payload in the coming days.

Thanks for letting us know Dan, this means it's worth for us wait a bit before merging this.

Also, it would be good if this workaround was not applied to unaffected CPUs (e.g. Cortex-A32, Cortex-A7 and Cortex-A5).

Agree!

jforissier · 2018-01-12T08:39:16Z

Sounds like we may have to use runtime detection in order to install the proper exception vector. Otherwise, we can't properly support mixed configurations like big.LITTLE.

etienne-lms · 2018-01-12T11:09:48Z

Can't we do something about it using the core/arch/arm/cpus/xxx.mk?
Current status is the patches are default enable but the platform can force their disabling.

jenswi-linaro · 2018-01-12T12:00:28Z

Rebased on master, updated for runtime configuration.

jforissier

With the movs fixed:

Reviewed-by: Jerome Forissier <[email protected]>
Tested-by: Jerome Forissier <[email protected]> (HiKey)
Tested-by: Jerome Forissier <[email protected]> (HiKey960)

jforissier · 2018-01-12T13:12:41Z

core/arch/arm/kernel/thread_a32.S

+	cmpne	r2, r3
+	movne	r3, #CORTEX_A75_PART_NUM
+	cmpne	r2, r3
+	ldreq	r0, =exception_vector_bpial


s/bpial/bpiall/

jforissier · 2018-01-12T13:16:36Z

core/arch/arm/kernel/thread_a32.S

+	ldreq	r0, =exception_vector_a15
+	beq	2f
+#endif
+1:	ldr	r0, =thread_vect_table


Minor thing: now we have two wordings, "exception vector" and "thread vector table". I'd rather s/thread_vect_table/exception_vector/ but that's probably out of scope for this PR.

Yes, let's take that cleanup in a later PR.

jforissier · 2018-01-12T13:35:21Z

core/arch/arm/kernel/thread_a32.S

+	cmp_spsr_user_mode r0
+	bne	1f
+	/* Invalidate the branch predictor for the current processor. */
+	write_iciallu


Could you add a note to emphasise that BPIALL doesn't fit here and ICIALLU is indeed required? I'm referring to the statement: "Note that the BPIALL instruction is not effective in invalidating the branch predictor on Cortex-A15. For that CPU, set ACTLR[0] to 1 during early processor initialisation, and invalidate the branch predictor by performing an ICIALLU instruction." from the Arm-TF page https://github.com/ARM-software/arm-trusted-firmware/wiki/Arm-Trusted-Firmware-Security-Advisory-TFV-6#variant-2-cve-2017-5715.

jforissier · 2018-01-12T14:12:52Z

core/arch/arm/kernel/thread_a32.S

+	ubfx	r2, r1, #MIDR_PRIMARY_PART_NUM_SHIFT, \
+			#MIDR_PRIMARY_PART_NUM_WIDTH
+
+	mov	r3, #CORTEX_A8_PART_NUM


core/arch/arm/kernel/thread_a32.S: Assembler messages: core/arch/arm/kernel/thread_a32.S:477: Error: invalid constant (c08) after fixup core/arch/arm/kernel/thread_a32.S:479: Error: invalid constant (c09) after fixup core/arch/arm/kernel/thread_a32.S:481: Error: invalid constant (c0e) after fixup core/arch/arm/kernel/thread_a32.S:483: Error: invalid constant (d07) after fixup core/arch/arm/kernel/thread_a32.S:485: Error: invalid constant (d08) after fixup core/arch/arm/kernel/thread_a32.S:487: Error: invalid constant (d09) after fixup core/arch/arm/kernel/thread_a32.S:489: Error: invalid constant (d0a) after fixup core/arch/arm/kernel/thread_a32.S:494: Error: invalid constant (c0f) after fixup

s/mov/movw/ and s/movne/movwne/ fixes the issues

jenswi-linaro · 2018-01-15T08:17:25Z

Updated as requested, rebased, squashed and tags applied.

jenswi-linaro · 2018-01-15T09:13:00Z

Pushed updates (commit message and a spell error in a comment) for two checkpatch warnings.
One checkpatch warning for a long line remains, I don't want to break the url.

jenswi-linaro · 2018-01-15T11:05:03Z

Please wait with merging this, this PR requires ACTLR[0] set to 1 for Cortex-A15 platforms. I'm preparing another PR to take care of that.

jforissier · 2018-01-15T11:54:10Z

@jenswi-linaro sure

jenswi-linaro · 2018-01-15T11:56:07Z

Needs #2071

With CFG_CORE_UNMAP_CORE_AT_EL0=y the exception vector is updated to use the minimal kernel mapping during user space execution. With this patch vbar is updated relative to previous value in vbar to allow different exception vectors for different cpu types. Reviewed-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]>

If build with CFG_CORE_WORKAROUND_SPECTRE_BP_SEC=y invalidate branch predictor on all secure world exceptions. Fixes CVE-2017-5715 Tested-by: Jerome Forissier <[email protected]> (HiKey) Tested-by: Jerome Forissier <[email protected]> (HiKey960) Reviewed-by: Jerome Forissier <[email protected]> Signed-off-by: Jens Wiklander <[email protected]>

jenswi-linaro · 2018-01-16T12:42:30Z

Rebased on master

jforissier approved these changes Jan 8, 2018

View reviewed changes

jenswi-linaro force-pushed the cve-2017-5715-2 branch from 58effc5 to c7f8f75 Compare January 8, 2018 14:47

etienne-lms reviewed Jan 8, 2018

View reviewed changes

jenswi-linaro commented Jan 9, 2018

View reviewed changes

jenswi-linaro force-pushed the cve-2017-5715-2 branch from c7f8f75 to e985cb1 Compare January 9, 2018 14:40

jforissier mentioned this pull request Jan 12, 2018

core: arm32: sm: invalidate branch predictor #2047

Merged

jenswi-linaro force-pushed the cve-2017-5715-2 branch from e985cb1 to d63a923 Compare January 12, 2018 11:59

jforissier reviewed Jan 12, 2018

View reviewed changes

jenswi-linaro force-pushed the cve-2017-5715-2 branch from 2fd6261 to 0f2d789 Compare January 15, 2018 08:17

jenswi-linaro mentioned this pull request Jan 15, 2018

Secure monitor spectre updates #2065

Merged

jenswi-linaro force-pushed the cve-2017-5715-2 branch from 0f2d789 to 41c381c Compare January 15, 2018 09:12

jenswi-linaro mentioned this pull request Jan 15, 2018

Actlr update #2071

Merged

jenswi-linaro added 2 commits January 16, 2018 13:40

jenswi-linaro force-pushed the cve-2017-5715-2 branch from 41c381c to 7d1a49c Compare January 16, 2018 12:42

jforissier merged commit 4051194 into OP-TEE:master Jan 16, 2018

jenswi-linaro deleted the cve-2017-5715-2 branch January 16, 2018 14:30

Cve 2017 5715 2 #2055

Cve 2017 5715 2 #2055

Conversation

jenswi-linaro commented Jan 8, 2018 • edited Loading

jforissier left a comment • 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

jforissier commented Jan 8, 2018

jenswi-linaro commented Jan 8, 2018

jenswi-linaro 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

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 9, 2018

jforissier commented Jan 9, 2018

danh-arm commented Jan 11, 2018 • edited Loading

jbech-linaro commented Jan 12, 2018

jforissier commented Jan 12, 2018

etienne-lms commented Jan 12, 2018

jenswi-linaro commented Jan 12, 2018

jforissier left a 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

jenswi-linaro commented Jan 15, 2018

jenswi-linaro commented Jan 15, 2018

jenswi-linaro commented Jan 15, 2018

jforissier commented Jan 15, 2018

jenswi-linaro commented Jan 15, 2018

jenswi-linaro commented Jan 16, 2018

jenswi-linaro commented Jan 8, 2018 •

edited

Loading

jforissier left a comment •

edited

Loading

danh-arm commented Jan 11, 2018 •

edited

Loading