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

scottl created this revision.Nov 14 2019, 5:04 PM
scottl updated this revision to Diff 64334.Nov 14 2019, 5:22 PM

Small updates

kib added a comment.Nov 14 2019, 7:38 PM

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
1782

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

sys/x86/x86/cpu_machdep.c
1145

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

1160

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.

scottl updated this revision to Diff 64365.Nov 15 2019, 12:12 PM

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.

scottl updated this revision to Diff 64366.Nov 15 2019, 12:40 PM

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

scottl marked an inline comment as done.Nov 15 2019, 5:28 PM
scottl added inline comments.Nov 15 2019, 5:32 PM
sys/x86/x86/cpu_machdep.c
1160

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.

scottl updated this revision to Diff 64384.Nov 15 2019, 5:47 PM

Remove an unncessary XXX comment

scottl marked an inline comment as done.Nov 15 2019, 5:48 PM
scottl updated this revision to Diff 64393.Nov 15 2019, 7:23 PM

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

emaste added inline comments.Nov 15 2019, 7:44 PM
sys/amd64/amd64/machdep.c
1782

we should really use positive sense sysctls

scottl added inline comments.Nov 15 2019, 7:50 PM
sys/amd64/amd64/machdep.c
1782

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.

emaste added a comment.EditedNov 15 2019, 7:51 PM

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

emaste added inline comments.Nov 15 2019, 7:53 PM
sys/amd64/amd64/machdep.c
1782

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.

emaste added inline comments.Nov 15 2019, 8:09 PM
sys/x86/x86/cpu_machdep.c
1205

maybe /* CPU is not susceptible to TAA */

1207

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

1299

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

1305

Just "VERW"?

kib added inline comments.Nov 15 2019, 8:10 PM
sys/x86/x86/cpu_machdep.c
1245

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.

scottph added inline comments.Nov 15 2019, 8:12 PM
sys/amd64/amd64/machdep.c
1782

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.

kib added a comment.Nov 15 2019, 8:16 PM

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
1235

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

emaste added inline comments.Nov 15 2019, 8:20 PM
sys/amd64/amd64/machdep.c
1782

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?

scottl added inline comments.Nov 15 2019, 8:28 PM
sys/x86/x86/cpu_machdep.c
1245

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 updated this revision to Diff 64396.Nov 15 2019, 8:37 PM

Address several comments

scottl marked 4 inline comments as done.Nov 15 2019, 8:39 PM
scottl added inline comments.
sys/x86/x86/cpu_machdep.c
1207

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

scottl marked an inline comment as done.Nov 15 2019, 8:40 PM
scottl added inline comments.
sys/amd64/amd64/machdep.c
1782

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

kib added inline comments.Nov 15 2019, 8:44 PM
sys/x86/x86/cpu_machdep.c
1245

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.

scottl added inline comments.Nov 15 2019, 9:13 PM
sys/x86/x86/cpu_machdep.c
1245

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.

emaste added inline comments.Nov 15 2019, 9:16 PM
sys/x86/x86/cpu_machdep.c
1207

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.

scottl updated this revision to Diff 64405.Nov 15 2019, 11:10 PM

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 marked an inline comment as done.Nov 15 2019, 11:12 PM
scottl added inline comments.
sys/x86/x86/cpu_machdep.c
1299

I think that this is handled by the OID renaming

imp accepted this revision.Nov 15 2019, 11:35 PM

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
scottl updated this revision to Diff 64406.Nov 15 2019, 11:45 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