Page MenuHomeFreeBSD

Implement mitigation for Spectre Version 2 attacks on ARMv7.
ClosedPublic

Authored by mmel on Jan 16 2018, 6:16 AM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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
326 ↗(On Diff #38031)

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.

329 ↗(On Diff #38031)

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...

334 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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

sys/arm/arm/cpuinfo.c
375 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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

sys/arm/arm/swtch-v6.S
159 ↗(On Diff #38031)

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

sys/arm/arm/trap-v6.c
326 ↗(On Diff #38031)

I will do it.

329 ↗(On Diff #38031)

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

334 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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 ↗(On Diff #38031)

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 ↗(On Diff #38031)

"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
334 ↗(On Diff #38031)

excellent!

sys/arm/include/pcpu.h
67 ↗(On Diff #38031)

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

Addressed all objections.

sys/arm/arm/genassym.c
140 ↗(On Diff #38031)

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

sys/arm/arm/genassym.c
140 ↗(On Diff #38031)

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.