Page MenuHomeFreeBSD

TSX Asynchronous Abort Mitigation
Needs ReviewPublic

Authored by scottl on Nov 14 2019, 5:04 PM.

Details

Summary

Mitigation for TAA involves either turning off TSX or turning on the
VERW mitigation used for MDS. Some CPUs will also be self-mitigation
for TAA and require no software workaround.

  • If the CPU advertised TAA_NO, then no mitigation is needed
  • If the CPU advertises TSX_CTRL, then TSX can be turned off
  • Otherwise, mitigation requires VERW.

The control knobs for this are similar to mds_disable. Because the
VERW mitigation is already implemented in mds_disable, turning it
on for TAA will also turn it on for MDS. This means that the
hw.tsx_disable sysctl/tunable can override the hw.mds_disable
setting, even if the MDS_NO flag is present.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 27549
Build 25771: arc lint + arc unit

Event Timeline

That said, would be it more straightforward to add logic to hw_mds_recalculate() instead, to enable the workaround if either of mds_disable or tsx_disable is forced. I mean, get rid of hw_tsx_recalculate(), check TAA in hw_mds_recalculate.

sys/amd64/amd64/machdep.c
1781

This fetch must be done early enough so it must occur in hammer_time().

sys/x86/x86/cpu_machdep.c
1144

yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as well.

1159

I think you need to check the existing value of hw_mds_disable. If it is non-zero, do not touch it. If it is zero, setting it to 1 (VERW) might not work simply because the right microcode is not loaded. 3 (auto) might be more reasonable value.

Complete the TAA mitigation. Code is tested with and without the
microcode update. Has not been tested to see if it actually stops
the POC attacks.

Forgot to set the TSX MSR on all CPUs, not just the one that's currently
running. Fixed.

sys/x86/x86/cpu_machdep.c
1159

My intent was to assume that hw_mds_recalculate() would determine the correct state. If this code asks for VERW but that can't be done, then it'll just fail anyways.

Remove an unncessary XXX comment

Add some bootverbose messages for when things don't behave.
Track MDS state and synchronize more closely with it.

sys/amd64/amd64/machdep.c
1781

we should really use positive sense sysctls

sys/amd64/amd64/machdep.c
1781

Agreed, but I propose that we do a sweep and rename all of the nearby and related knobs. For now, hw.tsx_disable is consistent with the others, which I think it important.

PR241988 for a PR for the sense issue with the existing sysctl

sys/amd64/amd64/machdep.c
1781

It's not exactly, because mds_disable is actually enabling a workaround - we could rename it mds_workaround_enable and the values the user sets it to do not change.

If tsx_disable becomes tsx_enable the setting also has to change.

Anyhow, I didn't object (enough) when the previous wrong sense sysctls went in so I won't object now to more wrong ones.

My suggestion is to move ssb, mds, tsx, and probably others from _hw to something like _hw.x86.mitigations. Then rename each with a name that's not enable or disable. We would still have compat OIDs, and I think that the values could remain the same. That's work for a future time, though.

sys/x86/x86/cpu_machdep.c
1204

maybe /* CPU is not susceptible to TAA */

1206

/* CPU is susceptible to TAA and has the ability to disable TSX */

1298

I'm not sure that "inactive" is the right name for this, should probably be "TSX not disabled"?

1304

Just "VERW"?

sys/x86/x86/cpu_machdep.c
1244

hw_mds_disable is user-controlled, and user settings should be honored IMO.

This is one of the reason why I proposed to merge taa and mds recalculation functions.

sys/amd64/amd64/machdep.c
1781

I think the name here needs to be changed to taa_disable. The intention is to mitigate the TSX Async Abort vulnerability, not necessarily just general purpose TSX feature control.

You also need to reset the taa state after resume, at least if it was set to disable TSX. See how ibrs and sbb are reset in acpi_sleep_machdep(). Or you need to re-apply TSX disabling command after resume.

sys/x86/x86/cpu_machdep.c
1234

Why use ints and implicit convertions ? We have true/false constant symbols in C99.

sys/amd64/amd64/machdep.c
1781

Indeed, but is it not usable as a general TSX disable control? I.e., if in the future another problem is found with TSX we could still use this to turn it off?

sys/x86/x86/cpu_machdep.c
1244

How would it work if a user specified VERW for TAA and SW for MDS? Maybe these can be merged in the future, but for now I would like to keep them separate, and enforce that TAA is a superset of MDS, as stated in the review summary.

scottl added inline comments.
sys/x86/x86/cpu_machdep.c
1206

What you've described are two states that can be mutually exclusive.

scottl added inline comments.
sys/amd64/amd64/machdep.c
1781

hw.x86.mitigations.taa is my proposal. 0 = no mitigation, 1 = TSX-disable, 2 = VERW, 3 = auto

sys/x86/x86/cpu_machdep.c
1244

In this case you need somehow merge them, and this is easier to do in the merged function.

But I think that you should either not allow that fine control over taa (e.g. only provide disabled/mds/tsx variants of mitigation).

Or even better, further along lines of merged mds/taa handling, do not introduce separate knob to taa. Consider taa action as continuation of mds. For instance, auto would mean: if both MDS_NO and TAA_NO, do nothing. If MDS requires VERW, _or_ TAA requires VERW, it is VERW. If VERW is not available, it is microarch-specific code sequences.

sys/x86/x86/cpu_machdep.c
1244

I agree that this maybe could be better, but trying to handle MDS_NO, TAA_NO, TSX_CTRL feature bits (not to mention the MDS and RTM/HLE presence bits) as well as VERW vs software mitigation, is a very complex logic table. I'd like to keep it more simple for now, and consider combining in the future.

sys/x86/x86/cpu_machdep.c
1206

But this case here (where we set tsx_need = TSX_TAA_DISABLE) is exactly that - the CPU is susceptible to TAA, and can turn off TSX.

Move the sysctls and tunables to the new machdep.mitigations
tree. Rename the code in accordance, and rename the sysctls
themselves to have neutral wording.

scottl added inline comments.
sys/x86/x86/cpu_machdep.c
1298

I think that this is handled by the OID renaming

I'm cool with the sysctl/tunable naming.. Don't know about TAA enough to comment on the rest :)

This revision is now accepted and ready to land.Nov 15 2019, 11:35 PM

Move taa into its own sysctl node, machdep.mitigations.taa.(enable|state)

This revision now requires review to proceed.Nov 15 2019, 11:45 PM