Page MenuHomeFreeBSD

Implement mitigation for Spectre Version 2 attacks on ARMv7.
ClosedPublic

Authored by mmel on Jan 16 2018, 6:16 AM.
Tags
None
Referenced Files
F105856121: D13931.diff
Sat, Dec 21, 4:24 PM
Unknown Object (File)
Fri, Dec 20, 2:55 PM
Unknown Object (File)
Wed, Dec 4, 7:34 AM
Unknown Object (File)
Wed, Nov 27, 4:16 PM
Unknown Object (File)
Nov 19 2024, 5:55 PM
Unknown Object (File)
Nov 8 2024, 12:31 AM
Unknown Object (File)
Oct 4 2024, 10:54 AM
Unknown Object (File)
Oct 3 2024, 11:14 AM
Subscribers

Details

Summary

Similarly as we already do for arm64, for mitigation is necessary to
flush branch predictor when we:

  • do task switch
  • receive prefetch abort on non-userspace address

The user can disable this mitigation by setting 'machdep.disable_bp_hardening'
sysctl variable, or it can check actual system status by reading
'machdep.spectre_v2_safe'

The situation is complicated by fact that:

  • for Cortex-A8, the BPIALL instruction is effectively NOP until the IBE bit in ACTLR is set.
  • for Cortex-A15, the BPIALL is always NOP. The branch predictor can be only flushed by doing ICIALLU with special bit (Enable invalidates of BTB) set in ACTLR.

Since access to the ACTLR register is locked to secure monitor/firmware on
most boards, they will also need update of firmware / U-boot.
In worst case, when secure monitor is on-chip ROM (e.g. PandaBoard),
the board is unfixable.

MFC after: 2 weeks


Out of records:
There is still too much noise and inconsistent information on air.
Mainly:

  • Why is branch predictor flush necessary only for prefetch

translation/permission faults? By theory, it should be done for each user to
system transition. It's unclear if this 'otimalization' was selected by ARM by
analyzis of actual Linux code or if is determined by real branch predictor
implementation for affected cores.

  • The Arm Trusted Firmware Security Advisory TFV 6

https://github.com/ARM-software/arm-trusted-firmware/wiki/ARM-Trusted-Firmware-Security-Advisory-TFV-6
notes:
"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."

but ARM TRM for Cortex-A15 declares this:

Branch predictor requires flushing on:
• enabling or disabling the MMU
• writing new data to instruction locations
• writing new mappings to the translation tables.
• any change to the TTBR0, TTBR1, or TTBCR registers without a
corresponding change to the FCSE ProcessID or ContextID.

These two information are in direct conflict, I think.

Anyway, Linux have similar patch, designed by ARM employees, so I cannot do
much more that trust them.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mmel retitled this revision from Implement BP hardening as mitigation for Spectre Version 2 attacks on ARMv7 platforms. to Implement mitigation for Spectre Version 2 attacks on ARMv7..Jan 16 2018, 7:51 AM
mmel edited the summary of this revision. (Show Details)

Looks good to me, modulo one real problem that likely is a stray line noted in the assembler. Most of my concerns are questions and helping me connect all the dots that I have.

sys/arm/arm/cpuinfo.c
375

What about !ARM implementors? Do we know of any that FreeBSD runs on? Or are all the armv7 CPUs that we have Cortex Ax?

487–492

It's a shame that smp_rendezvous_cpus doesn't work on single core case... But fixing that is beyond the scope of this code review...

sys/arm/arm/genassym.c
140

do we need to publish the kind of BP hardening in use via a sysctl in case there's issues down the road?

sys/arm/arm/swtch-v6.S
159

We do this above conditional and then do it again unconditionally? I'm not sure I understand the logic here, and this differs from what I'd expect reading the ARM TR on this.

sys/arm/arm/trap-v6.c
340

I think this comment needs some more explanation.

"Apply BP hardening by flushing the branch prediction cache for prefaults on kernel addresses."

might be a good start.

343

why would this info be per-cpu? Seems like it could be rolled up into the IF statement as well if it were just a global...

348

The above differs from the assembler as I pointed out. This is what I'd expect here. I seem to recall reading that we need to do this invalidation as early as possible. Is that true for this case? If so, can we move this block earlier in the function with a comment to that effect?

sys/arm/include/pcpu.h
67

will the original ACTLR be different for different CPUs? Is that why the hardening kind is per-cpu?

sys/arm/arm/cpuinfo.c
375

Imho, we supports Marvell PJ4B cores and Qualcomm Krait cores.
Krait cores are known to be affected by Spectre v1,v2 but status of PJ4B is unknown yet.

sys/arm/arm/genassym.c
140

Hard to say, I have not any strict opinion about this

sys/arm/arm/swtch-v6.S
159

Oups. I forget to remove original "mcr CP15_BPIALL" line. Thanks.

sys/arm/arm/trap-v6.c
340

I will do it.

343

I plan to add support for multicluster SoC in "near" future. So yes, each cluster can have unique cores (big.LITTLE for example)

348

This is exact point of my confusion. Logic tells me that BP must be flushed before first indirect branch is taken (simply, before mistrained BP can be used). And this is not what this code does (nor original linux version).

I'm very afraid that this is done specifically for some real (but not published) attack vector based on Linux code.

Anyway, I will move (and comment) this block.

sys/arm/include/pcpu.h
67

The original ACTRL may be different on each CPU (but we can expect that these values are same for all CPUs in single cluster). But main reason to have this in PCPU is support for multi-cluster SoCs.

sys/arm/arm/cpuinfo.c
375

Do we need to print warnings for those cores? Eg "Qualcomm Krait cores are known (or believed) to be vulnerable to speculative branch attacks, no mitigation exists yet." ?

452

"Would it make sense to add "It is vulnerable to speculative branch attacks." after "unsecure" (which I think should be "insecure" since that reads better to my ear)?

sys/arm/arm/trap-v6.c
348

excellent!

sys/arm/include/pcpu.h
67

I'd forgotten cases like BIG.little. This makes sense.

Addressed all objections.

sys/arm/arm/genassym.c
140

IMO no, but would be nice to have a printf in verbose boot.

sys/arm/arm/genassym.c
140

OK, I agree (And it's easy :) ) Is something like "CPU(%d) BP hardening kind: (not necessary | BPIALL | ICIALLU)" acceptable?

  • print actual mitigation variant if bootversode
  • slightly restructure code to make additional vendors or CPUs addition easier
This revision was not accepted when it landed; it landed in state Needs Review.Jan 27 2018, 11:19 AM
This revision was automatically updated to reflect the committed changes.